초급 개발자가 해보는 리팩토링

이완희·2023년 5월 21일
0

개요

저는 자바 1년차 개발자로써 풍부한 경험과 뛰어난 식견이 있지 않습니다.

코딩을 하면서 늘 “어떻게 해야 더 효율으로 구현할 수 있을까?”라는 의문을 품었습니다. 코드를 구현할 당시에는 “이쁘고 효율적으로 코딩하자”라는 마인드보단 “에러 없이 동작하는 코딩을 하자”란 마인드가 더 강했습니다. 물론 테스트 코드를 만들고 코드 리뷰를 하면서 보다 좋은 방향으로 개선했지만 큰 흐름에 있어서는 문제점들이 있었습니다. 개선은 하겠지만 무조건적인 정답은 없으며 3년뒤에 이 글을 보게 된다면 수정사항은 여전히 많을 것입니다. 그럼에도 일단 시도해보고 하나씩 바꿔나가겠습니다. 리팩토링과 코드 컨벤션 가이드의 내용도 얼추 포함되어 있습니다.

클린 코드와 블로그들, 그리고 제 견해가 잔뜩 들어간 리팩토링을 해보겠습니다.

실제 코드를 통한 리팩토링

리팩토링을 하기 앞서 테스트코드가 잘 구현되어있는지 잘 살펴보자.

리팩토링을 하기 전이나 이후에나 사용자가 변함을 알게되면 안된다.

성능이 느려졌거나, 동일한 Request를 보내는데 Error가 발생한다면 오히려 리팩토링이 독이 되었다.

그렇다면 리팩토링을 하고나서 통합 테스트를 꼼꼼히 해봐야할까? 물론 이것도 권장되지만 시간절약을 위해선 Repository, Service, API Layer에서 다양한 Unit Test가 작성되어야 한다. 그래야 변경점을 바로바로 알아낼 수 있다. 반드시 충분한 테스트코드가 작성되었는지 살펴보자.

주석

주석은 의미가 있어야 한다. 메소드의 정보나 개발자의 의도를 잘 적는다면 좋은 주석이다.

반대로 변수나 메소드를 사용하지 않는고 주석처리, 의미를 전달하지 못하고 구구절절 설명하는 주석은 나쁜주석이다. 나쁜 주석을 제거하면 라인수는 줄어서 코드를 보기가 더 쉬워지고, 좋은 주석을 추가하면 다른 개발자가 본인에게 “근데 이건 왜 이렇게 적었어요?”라고 묻는일은 없을 것이다

/**
 *운행구분에 따라 조정율을 반환한다.
 *왕복(A) or 2회전(B) or 3회전(C) or 4회전(Y)이고 왕복 페어가DB에 저장되어있으면 해당 요율을 따른다. (승인 시 처리)
 *터미널 입고(E) or직간선(T)이면 요율테이블에서 조회한다.
 *왕복(A) or정규편도(D) or T/S운행(H)이면 조정율을100으로 반환한다.
 * 2회전(B)이고 거리가100이상이면75를, 100미만이면65를 반환한다.
 * 3회전(C) or 4회전(Y)이고 거리가100이상이면65를, 100미만이면55를 반환한다.
 */
public int retrieveAdjustmentRatio(RouteCharge rc) {
    String routePlanTypeCd = rc.getRoutePlanType();
    if ("E".equals(routePlanTypeCd) || "T".equals(routePlanTypeCd)) {
        return this.findCostRate(rc.getDepartureTmlCd(), rc.getArrivalTmlCd(), rc.getCorporationRegNo(), rc.getDrivingDt());
    } else if ("A".equals(routePlanTypeCd) || "D".equals(routePlanTypeCd) || "H".equals(routePlanTypeCd)) {
        return 100;
    } else if ("B".equals(routePlanTypeCd)) {
        return rc.getDistance() >= 100 ? 75 : 65;
    } else if ("C".equals(routePlanTypeCd) || "Y".equals(routePlanTypeCd)) {
        return rc.getDistance() >= 100 ? 65 : 55;
    }
    return rc.getAdjustmentRatio();
}

비지니스 로직을 충분히 설명한 주석이다.

    //      처움부터 주석 처리되어 있었음
//        log.info("costRateId: {}", costRateId);

“처음부터 주석 처리되어 있었음”이란 문장이 왜 필요할까? 추후에 누가 봤을 때 저 주석으로 어떠한 정보도 얻기 쉽지 않다.

// private final ObjectMapper mapper;

삭제 하지 않는 메소드나 변수는 주석처리 하지말고 지우자. 언젠가 또 써야할지도 모른다고 해도 걱정할 필욘 없다. 우린 git을 쓰니까 형상관리를 통해 지운 코드를 복원할 수 있다.

네이밍

변수나 메소드는 적절한 네이밍을 갖춰야 한다.

한 눈에 목적과 의도등을 파악할 수 있어야 한다.

private void checkCorporationRegNoIsValid(...)

사업자등록번호를 체크하고 inValid하다면 Exception을 발생하는 메소드이다. 약간 아쉽다고 느껴지는게 isValid면 true/false일테니 boolean으로 반환해야 하지 않을까 싶다. Exception을 발생할거면 메소드 명을

checkCoprotationRegNoElseThrow 등으로 바꿨어도 괜찮았을지도?

Optional<CostRate> findFirstByDepartureTerritoryCdAndArrivalTerritoryCdAndCorporationRegNoAndApplyStartDtLessThanEqualOrderByApplyStartDtDesc(
        String departureTerritoryCd, String arrivalTerritoryCd, String corporationRegNo, String applyStartDt);

무려 이게 한 메소드입니다. JPA의 기능을 너무 충실히 이행하다보면 저런 메소드도 만들수 있게 됩니다. 수능 영어 해석하듯 끊어 읽으면 어떤 메소드인지는 알수 있겠지만 메소드명이 너무 길어서 라인 가독성도 해치는 문제가 있습니다. 이는 Nativae Query를 사용하여 해결했습니다.

@Query(value = "SELECT * FROM CostRate " +
        "WHERE departure_territory_cd = :departureTerritoryCd " +
        "AND arrival_territory_cd = :arrivalTerritoryCd " +
        "AND corporation_reg_no = :corporationRegNo " +
        "AND apply_start_dt <= :applyStartDt " +
        "ORDER BY apply_start_dt DESC " +
        "LIMIT 1", nativeQuery = true)
Optional<CostRate> findValidCostRate(String departureTerritoryCd, String arrivalTerritoryCd, String corporationRegNo, String applyStartDt);

혼자 알고리즘 문제를 풀때에는 x1, a, b같은 의미없는 변수명을 써도 될지는 몰라도 협업하는 프로젝트에선 의미없는 변수명 사용을 지양해야 합니다. 메소드와 마찬가지로 변수명도 너무 길지는 않되, 개발자의 의도를 잘 담아야 합니다.

적절한 어노테이션 사용

@Setter(AccessLevel.NONE)을 사용하자

VO, DTO, Entity등 @Data를 사용하는 POJO객체들은 @Setter가 사용된다. AccessLevel.NONE을 사용해야 하는 이유는 불변성 유지와 보안 강화이다. 해당 필드가 아닌 외부에서 호출하여 무분별하게 값을 수정한다면 이는 데이터가 오염된다. 반드시 해당 필드내에서만 데이터를 변경하도록 하자.

public void updateDistance(int distance) {
    this.distance = distance;
}

@Data를 그대로 사용해도 괜찮을까?

@Data는 @Getter, @Setter, @RequiredArgsConstructor, @ToString, @EqualAndHashCode, @Value를 포함한다. @Setter의 설정값을 바꿔야 하는 이유는 위에서 설명했다. 다만 사용하지도 않는 Annotation을 일일히 호출하는게 괜찮을지는 다시 생각해보자.

@AllArgsConstructor은 두 개의 같은 타입 인스턴스 멤버를 선언한 상황에서 개발자가 선언된 인스턴스 멤버의 순서를 바꾸면 lombok이 개발자도 모르게 생성자의 파라미터 순서를 필드 선언 순서에 따라 변경하게 되므로 입력된 값이 잘못 들어가는 상황이 발생하게 된다.

너무 길지 않은 메소드 구현

메소드는 잘 읽혀야 합니다. 한눈에 봤을 때 “대체 이게 뭐야…?”생각이 들면 안되겠죠. 바로 파악하기가 쉽지는 않더라도 라인마다 어떤 역할을 하는지는 알수 있어야 합니다.

@Transactional
public CommonCommandResponseEx save(SaveRoutePlanRequest request) {
    CommonCommandResponseEx<CommonRoutePlan, Integer> response = new CommonCommandResponseEx<>();
    List<CommonRoutePlan> requestPlans = request.getPlans();
    List<Long> deletedIds = request.getDeletedRouteIds().stream().map(Long::parseLong).collect(Collectors.toList());
    List<StopoverSaveRequest> stopoverDetails = request.getStopover();
    List<Long> deletedStopoverIds = request.getDeletedStopoverIds();
    Map<String, Long> uuidMap = new HashMap<>();

    this.deleteIdList(deletedIds);
    this.deleteStopover(deletedStopoverIds);

    //TODO:전체 리팩토링
List<CommonRoutePlan> insertReq = requestPlans.stream().filter(CommonRoutePlan.Specification::isStsInsert).collect(Collectors.toList());
    List<CommonRoutePlan> updateReq = requestPlans.stream().filter(CommonRoutePlan.Specification::isStsUpdate).collect(Collectors.toList());

    //TODO리팩토링.임시조치: invalidTruckingNo가 발생하면 해당 노선번호 전체에 대해 처리하지 않고 메시지를 던진다.
// 중복 편명이 있는 노선번호는 제외
    removeInvalidTruckingNoAndIncludedRouteNumber(response, insertReq, updateReq);
    // 리팩토링 대상 종료
    // 노선번호 채번 및 할당
    setRouteNumberToInsertReq(insertReq);
    // routeGroupId
    Map<String, String> routeGroupIdMap = this.createUUIDMap(request);

    // 같은 노선에 편명 '없음'이 두개이상 들어가면 All Fail 하기 위해 2개 이상인 것을 list로 담아오는 메서드입니다.
    List<Integer> duplicatedList = this.getDuplicatedNoneTruckingNo(insertReq, updateReq, routeGroupIdMap);

    if (!commonUtils.isListEmpty(duplicatedList)) {
        return response.addAllFail(duplicatedList, PreparedMessages.CAN_NOT_REGISTER_NONE_TRUCKING_NO_BOTH_DRIVING);
    } else {
        insertReq.forEach(req -> {
            try {
                // 저장 전 유효성 체크
                scheduledRoutePlanSpecification.preflightSave(insertReq, req);
                vendorDomainSpec.isCorpRegNoNullWithOutSelfDrivingElseThrow(req.getVehicleVendorNm(), req.getCorporationRegNo());

                String routeGroupId = routeGroupIdMap.get(req.getDrivingDt() + req.getRouteNumber() + req.getRoutePlanType());
                int distance = routePlanDomainService.findDistance(req.getTsCd(), req.getDepartureTmlCd(), req.getArrivalTmlCd());
                req.setRouteGroupIdAndDistance(routeGroupId, distance);

                ScheduledRoutePlan rp = scheduledRoutePlanMapper.toEntity(req);
                ScheduledRoutePlan scheduledRoutePlan = scheduledRoutePlanRepository.save(rp);
                drivingRepository.save(Driving.fromScheduledRoutePlan(scheduledRoutePlan, CommonMessage.B2C.getValue()));
                CommonRoutePlan reqResult = scheduledRoutePlanMapper.toDto(scheduledRoutePlan);
                reqResult.setRowId(req.getRowId());
                uuidMap.put(req.getExbStopoverUuid(), scheduledRoutePlan.getRoutePlanId());
                reqResult.setExbStopoverUuid("");
                response.addSuccess(reqResult);
            } catch (InvalidSpecException e) {
                response.addFail(req.getRowId(), e.getPreparedMessage());
            }
        });

        Map<Long, CommonRoutePlan> paramContextMap = this.toMap(updateReq);
        scheduledRoutePlanRepository.findByRoutePlanIdIn(Lists.newArrayList(paramContextMap.keySet()))
                .forEach(o -> {
                    int rowId = paramContextMap.get(o.getRoutePlanId()).getRowId();

                    if (vendorDomainSpec.isCorpRegNoNullWithOutSelfDriving(o.getVehicleVendorNm(), o.getCorporationRegNo())) {
                        response.addFail(rowId, PreparedMessages.CORPORATION_REG_NO_IS_NULL);
                        paramContextMap.remove(o.getRoutePlanId());
                        return;
                    }

                    //삭제된 데이터는 제외
                    if (scheduledRoutePlanSpecification.isScheduleDeleted(o)) {
                        response.addFail(rowId, PreparedMessages.DELETED_DATA_EXIST_IN_TARGET);
                        paramContextMap.remove(o.getRoutePlanId());
                        return;
                    }

                    // 운행상태가 '완료'면 제외
                    boolean isDrivingStatusStandby = drivingDomainSpecification.isStandby(drivingRepository.findById(Driving.createScheduledRoutePlanIdToDrivingId(o.getRoutePlanId())));
                    if (!isDrivingStatusStandby) {
                        response.addFail(rowId, PreparedMessages.CANNOT_MODIFY_DRIVING_STATUS_DONE);
                        paramContextMap.remove(o.getRoutePlanId());
                        return;
                    }

                    //그 이외 케이스는 업데이트
                    o.edit(scheduledRoutePlanMapper.toEntity(paramContextMap.get(o.getRoutePlanId())));
                    int distance = routePlanDomainService.findDistance(o.getTsCd(), o.getDepartureTmlCd(), o.getArrivalTmlCd());
                    o.updateDistance(distance);
                    drivingRepository.save(Driving.fromScheduledRoutePlan(o, CommonMessage.B2C.getValue()));
                    hasRowIdSuccessRes(response, paramContextMap, o);
                    if (o.getDistance() == 0) {
                        response.addExtra(rowId, PreparedMessages.DISTANCE_MUST_NOT_ZERO);
                    }
                    paramContextMap.remove(o.getRoutePlanId());
                });

        // 경유지 저장하는 부분(insert)
        stopoverDomainService.saveScheduledStopover(stopoverDetails, uuidMap);

        response.addAllFail(this.toRowIdCollection(paramContextMap), PreparedMessages.NOT_EXIST_ID_WARN);
        return response;
    }
}

이게 무려 한 메소드입니다. 무려 98줄이네요. 좋은 메소드는 30줄이 넘어가지 않는단 얘기가 있습니다.

if (!commonUtils.isListEmpty(duplicatedList)){…} else{…}부터는 간선계획을 Insert하고 그 아래 메소드는 간선계획을 update하고 있으므로 두 부분을 메소드로 따로 빼면 되겠네요.

@Transactional
public CommonCommandResponseEx save(SaveRoutePlanRequest request) {
    CommonCommandResponseEx<CommonRoutePlan, Integer> response = new CommonCommandResponseEx<>();
    List<CommonRoutePlan> requestPlans = request.getPlans();
    List<Long> deletedIds = request.getDeletedRouteIds().stream().map(Long::parseLong).collect(Collectors.toList());
    List<StopoverSaveRequest> stopoverDetails = request.getStopover();
    List<Long> deletedStopoverIds = request.getDeletedStopoverIds();
    Map<String, Long> uuidMap = new HashMap<>();

    this.deleteIdList(deletedIds);
    this.deleteStopover(deletedStopoverIds);

    removeInvalidTruckingNoAndIncludedRouteNumber(response, requestPlans);
    setRouteNumberToInsertReq(requestPlans);
    routeGroupIdMap = createUUIDMap(request);

    processInsertRequests(response, requestPlans, routeGroupIdMap, uuidMap);
    processUpdateRequests(response, requestPlans, routeGroupIdMap);

    stopoverDomainService.saveScheduledStopover(stopoverDetails, uuidMap);

    response.addAllFail(this.toRowIdCollection(paramContextMap), PreparedMessages.NOT_EXIST_ID_WARN);
    return response;
}

아까보다 훨씬 짧아졌고 로직은 변한게 없습니다. processInsertRequests와 processUpdateRequests의 메소드는 굳이 적지 않을게요!

위처럼 메소드가 너무 길어질때는 메소드 분리작업이 필수적이라 생각합니다. 너무 길지 않더라도 기능이 명확하게 구현되어 있다면 과감하게 분리해봐요.

메소드 뿐만 아니라 Input Parameter가 너무 많아서 메소드가 길어질 수도 있습니다. 저는 Input전용 객체를 만들고 Builder패턴을 이용하여 코드를 간결하게 했습니다.

도메인이 책임을 다하게 하자

간선 프로덕트에서 터미널은 매우 다양한 도메인에서 중요한 정보로 사용됩니다. 운임을 계산할때도, 거리를 조회할때도, 편명을 생성할때에도 사용됩니다. 운임 도메인은 터미널 Repository를 호출하여 조회하고, 거리 도메인에서도 조회하고 심지어 편명은 조회뿐만 아니라 UPDATE작업도 수행합니다. 잠깐 이게 맞나?

DDD에서는 도메인 간의 의존성을 최소화하고, 도메인 간의 경계를 명확하게 구분 짓는 것이 중요합니다. 그래서 위 방법은 DDD 설계를 위반합니다. 도메인이 외부에서 조회되고 변경된다면 도메인의 역할을 제대로 못하고 있습니다. 그렇다면 방법은 터미널 Service Layer에서 조회 또는 수정하는 메소드를 작성하고 운임등의 도메인은 이를 호출하기만 하면 됩니다. 다만 타 서비스를 호출하게 된다면 순환참조 오류가 발생할 가능성도 생기게 되므로 조심해야 합니다. 이는 Sequence Diagram등을 작성한다면 어렵지 않게 해결할 수 있습니다.

중복되는 메소드는 공통 메소드로 만들자

객체지향의 장점 중 하나는 “재사용성”입니다. 부모 클래스에서 구현한 메소드를 상속받으면 자식 클래스에서도 그대로 사용 가능합니다. 재사용성을 활용하지 못하면 비슷하게 생긴 코드들이 여기저기 퍼져있어서 찾기 힘듭니다. 그것만이 문제가 되지는 않습니다. 만일 여러군데 퍼져있는 메소드가 잘못 되었거나 요구사항이 변경되어 코드를 수정해야 한다면? 전체 찾기를 통해서 바꾼다 하더라도 문제가 생길 수 밖에 없습니다.

public static BigDecimal nvl(final BigDecimal bigDecimal) {
    return bigDecimal == null ? BigDecimal.ZERO: bigDecimal;
}

RouteCharge(운임) 도메인에서 사용하는 nvl메소드 입니다. 초기 메소드 구현시 Input Parameter가 BigDecimal인 메소드는 RouteCharge에서만 사용하기에 문제 없었습니다. 하지만 Settlement(정산), EtcRouteCharege(기타운임)등에서도 nvl을 사용해야 할 경우가 생겼습니다. 그렇다면 Settlement에서 RouteCharge 도메인에 있는 nvl을 참조하는게 맞을까요? 도메인에서 직접 도메인을 호출하는건 올바르지 않다고 생각합니다.

그러므로 여러 도메인에서 호출하는 메소드는 CommonUtils 에서 구현해줍시다. CommonUtils은 메소드를 static하게 생성하여 인스턴스를 생성하지 않더라도 메소드를 호출 가능합니다.

다음으론 상속을 통한 코드 재상용성을 높인 경우입니다.

public Specification<Driving> withCommonSearch(CommonSearchRequest request) {
    return ((root, query, builder) -> {
        List<Predicate> predicates = new ArrayList<>();

        if (!Strings.isNullOrEmpty(request.getDrivingDt())) {
            predicates.add(builder.equal(root.get("drivingDt"), request.getDrivingDt()));
        }

        // Append default conditions to Predicate collection
        List<String> terminals;

        if ("local".equals(request.getTmlType().name())) {
            terminals = this.ofcOrgCdByHierarchy(request.getRegionBranchCd(), request.getLocalBranchCd());
        } else {
            terminals = this.tmlCdByHierarchy(request.getRegionBranchCd(), request.getLocalBranchCd(), request.getTmlCd());
        }

        if (terminals != null) {
            terminals.addAll(etcTerminalSearchHelper.searchEtcTml(terminals));
        }

        withSearchCondition(request, root, predicates, terminals);

        predicates.addAll(withNotReady(root, builder));
        predicates.add(withNonScheduled(root, builder));
        query.orderBy(withApproveOrder(root, builder));

        if (GrantedOrgContextHelper.isVE()) {
            predicates.add(builder.like(root.get("vehicleVendorNm"), "%" + grantedOrganizationContext.seekOrgNm() + "%"));
        }

        predicateCustTypeCd(request.getSearchType(), root, builder, predicates);

        // Append default conditions to Predicate collection
        predicates.addAll(withUnDeleted(root, builder));
        return builder.and(predicates.toArray(new Predicate[0]));
    });
}

DrivingJpaSpecification의 CommonSearch입니다. if (GrantedOrgContextHelper.isVE()) {…}는 여러 ~JpaSpecification에서 사용되고 있습니다. 지금은 간선사명이 조회조건이지만 간선사번호로 요구사항이 바뀔경우 변경할 곳이 많으므로 오류가 발생할 확률도 높아집니다. 따라서 DrivingJpaSpecification

CommonJpaSpecification을 상속받도록 해줍시다. 이후에 메소드를 구현하면 됩니다.

protected void predicateVE(Root<?> root, CriteriaBuilder builder, List<Predicate> predicates) {
    if (GrantedOrgContextHelper.isVE()) {
        predicates.add(builder.like(root.get("vehicleVendorNm"), "%" + grantedOrganizationContext.seekOrgNm() + "%"));
    }
}

코드도 훨씬 간결해지고 변화에도 대응하기 좋은 코드가 탄생했습니다!

TODO

현재로썬 실력이 부족하여 구현하지 못한 기술등입니다. 더욱 실력을 갈아서 더 멋진 프로덕트를 만들겠습니다.

  • 싱글톤 패턴과 같은 디자인 패턴 활용
  • 보다 객체지향적인 코드 구현
  • DDD에 충실한 아키텍쳐 설계
profile
인생을 재밌게, 자유롭게

0개의 댓글