[핏츠유 리펙토링1] 가독성과 유지보수성 개선

 

배경

그동안 빠른 프로덕트 출시를 위해 개발 속도를 우선시해 왔고, 이 과정에서 유지보수성과 성능을 저해하는 코드가 누적되었습니다. 해서 앞으로 주제를 정해 점진적으로 리펙토링을 진행하며 배운 내용과 변경 사항들을 공유하려고 합니다.

첫번째 주제는 친숙하지만 어렵고 또 협업의 관점에가 가장 중요할 수 있는 '가독성과 유지보수성 개선'입니다.

 

변수 및 메서드 네이밍 규칙

• 번거롭고 길더라도 그 의미를 파악할 수 있게 풀어서 설명하도록 함이 협업에 유리합니다.
• 리스트(List), 맵(Map) 등의 자료구조가 포함된 경우, 이를 surfix 로 사용하여 자료형을 쉽게 유추할 수 있도록 하는 편입니다.

그 도메인 안에서 누구나 직관적으로 이해가 되는 줄임말이라면 쓸 수 있지만 개인적으로 선호하진 않습니다

•  ex) btn(button), cnt(count),  lat/long(latitude longitude)등등

 

상수 :  매직 넘버 지양

매직넘버는 많이 경계하는 편입니다. 나중에 코드를 되돌아 봤을때 제가 작성한 값에 의미를 이해할 수 없던 적도 많았습니다.

다만 아래 첫번째 유형에서 클래스크기가 크고 메서드 내부에서만 사용되는 값이라면 메서드 내부에 정의해보는 것도 고려해볼 수 있을 것 같습니다

상수 유형	선언 위치	예시
특정 클래스에서만 사용되는 상수	해당 클래스 내부 private static final	private static final int MAX_RETRY_COUNT = 5;
여러 곳에서 공통으로 사용되는 상수	별도 Constants 클래스	public static final String BASE_URL = "https://api.example.com";
ENUM 값과 관련된 상수	해당 enum 내에 정의	public enum UserRole { ADMIN, USER }
설정 값 (환경 변수로 변경 가능성 있음)	application.properties / .yml / @Value	@Value("${app.max-retry}") private int maxRetry;

 

메서드 쪼개기

개인적으로 메서드는 쪼갤 수록 좋다라는 말에는 완전히 동의하기는 어렵습니다.

결국 메서드를 쪼개는 것도 가독성과 유지 보수에 유리하기 위함인데 두줄짜리 코드를 한줄씩 두개의 매서드로 관리하는 것은 본래 취지를 해친다고 생각합니다.

따라서 메서드 기능 구현 차원에서 작성해놓고 한 메서드 안에서 여러 역할이 수행되고 있고 한눈에 이해가 되지 않을 정도로 간단한 코드가 아니라면 분리해나 가는 편입니다.

 

아래 예시 코드에서는 티겟을 저장 한다 라는 메서드 안에 코드 생성, 티켓 생성, 티켓 셋팅, 티켓 저장 들의 역할이 혼재 되어 있어 가독성이 떨어집니다.

@Transactional
public Long saveTicket(Long userId, Long trainerId, int totalCount, LocalDate startAt, LocalDate endAt, String centerName, String centerTel, Integer price, String otherInfo) {
    String seed = "시드 값";
    SecureRandom random = new SecureRandom();
    StringBuilder codeStringBuilder = new StringBuilder();
    while (true) {
        for (int i = 0; i < MAX_TICKET_CODE_LENGTH; i++) {
            int index = random.nextInt(seed.length());
            codeStringBuilder.append(seed.charAt(index));
        }
        if (courseTicketRepository.existByCode(codeStringBuilder.toString())) {
            codeStringBuilder = new StringBuilder();
        } else {
            break;
        }
    }

    CourseTicket ticket = new CourseTicket(userId, trainerId, totalCount, startAt, endAt, codeStringBuilder.toString());
    ticket.setPrice(price);
    ticket.setCenter(centerName, centerTel);
    ticket.setOtherInfo(otherInfo);
    return courseTicketRepository.save(ticket);
}

 

수정된 코드 안에서는 티켓코드를 생성하는 코드와 티켓을 세팅하는 코드를 다른 메서드로 정의함으로써 메서드 별 역할이 좀 더 명확해졌고 이에 따라 가독성이 증가합니다. 

public Long saveTicket(Long userId, Long trainerId, int totalCount, LocalDate startAt, LocalDate endAt, String centerName, String centerTel, Integer price, String otherInfo) {
    String ticketCode = generateUniqueTicketCode();

	CourseTicket ticket = new CourseTicket(userId, trainerId, totalCount, startAt, endAt, ticketCode);
    ticket.updateTickerDetails(price, centerName, centerTel, otherInfo);
    
    return courseTicketRepository.save(ticket);
}

private String generateUniqueTicketCode() {
    String seed = "시드 값";
    SecureRandom random = new SecureRandom();
    final int MAX_ATTEMPTS = 10;

    for (int attempt = 0; attempt < MAX_ATTEMPTS; attempt++) {
        String code = random.ints(MAX_TICKET_CODE_LENGTH, 0, seed.length())
                .mapToObj(seed::charAt)
                .map(String::valueOf)
                .collect(Collectors.joining());

        if (!courseTicketRepository.existByCode(code)) {
            return code;
        }
    }

    throw new IllegalStateException("유효한 티켓 코드를 생성할 수 없습니다.");
}

 

코드의 격이 맞아졌다고 표한 할 수도 있을 것 같습니다.(이전 회사 시니어 분에 표현이었는데 와닿아서 기억이 오래남네요. 코드의 격이란 생각해보면 추상화 레벨이라고 이해해도 좋을 것 같습니다!) 이전에 티켓 코드를 만드는 과정의 코드에서 메서를 통해 반환하는 코드로 격이 올라갔고 saveTicket 안에 다른 코드들과 그 격이 비슷해졌기에 코드를 읽는데 이질감이 느껴지지 않는 것 같습니다.

 

 

중첩 조건문 단순화

중첩 조건문의 경우 깊이나 코드 길이가 늘어 날 수록 복잡도가 크게 늘어납니다.

특히 코드의 흐름을 한눈에 파악하기가 어렵고 조건이 많아질 수록 실수할 가능성이 높습니다.

 

 

EarlyReturn

불필요한 중첩을 줄이고 조건에 부합하거나 만족되지 않으면 바로 리턴하는 방식입니다.

미리 탈출이 가능한 조건에 대한 처리가 우선적으로 수행되기때문에 해당 조건에 대해서 불필요하게 코드를 끝까지 읽지 않아도 된다는 장점을 갖습니다. 성능적 이점은 덤입니다.

 

그러나 EarlyReturn 에도 단점은 존재합니다. 탈출 영역이 여럿생김으로써 코드 복잡도가 올라갈 수 있습니다.
EarlyReturn 단점에 대해서는 최하단에 첨부 한 링크들을 참고 해봐도 좋을 것 같습니다.

 

 

[수정전]

기존에 "트레이너가 특정 회원에 대한 설명이 있는 경우" 를 확인하려면 위에 모든 분기 처리를 확인해야 한다는 점에서 가독성이 떨어졌습니다. (그래도 주석으로 각각에 대한 설명이 있어 다행이다.) 또한 어떤 케이스가 되든 리턴이 최하단에 위치하기에 코드를 끝까지 읽어봐야 한다는 제약이 존재합니다. 

private TrainerDescriptionResult getTrainerDescriptionResult(Long traineeId, Long workoutId, Long trainerId) {
    List<String> descriptions;
    WorkoutDescriptionType workoutDescriptionType;
    List<WorkoutCustomDescription> specificDescriptions = workoutCustomDescRepository.findByCreatorIdAndTargetUserIdAndWorkoutId(trainerId, traineeId, workoutId);

    if (specificDescriptions.isEmpty()) { // 트레이너가 설정한 특정 회원에 대한 설명이 없는 경우
        List<WorkoutCustomDescription> defaultDescriptions = workoutCustomDescRepository.findDefaultDescriptionByCreatorIdAndWorkoutId(trainerId, workoutId);
        if (!defaultDescriptions.isEmpty()) { // 트레이너가 설정한 기본 설명이 있는 경우
            descriptions = defaultDescriptions.stream().map(WorkoutCustomDescription::getDescription).toList();
            workoutDescriptionType = WorkoutDescriptionType.COMMON;
        } else { // 트레이너가 아무 운동법도 설정하지 않은 경우
            workoutDescriptionType = WorkoutDescriptionType.COMMON;
            if (WorkoutCategory.isCustomWorkout(workoutId)) {
                descriptions = Collections.emptyList();
            } else {
                WorkoutCategory workoutCategory = WorkoutCategory.fromCode(workoutId.intValue());
                descriptions = workoutCategory.getSteps();
                workoutDescriptionType = WorkoutDescriptionType.FITSYOU;
            }
        }
    } else { // 트레이너가 설정한 특정 회원에 대한 설명이 있을 경우
        descriptions = specificDescriptions.stream().map(WorkoutCustomDescription::getDescription).toList();
        workoutDescriptionType = WorkoutDescriptionType.SPECIFIC;
    }
    return new TrainerDescriptionResult(descriptions, workoutDescriptionType);
}

 

[1차 수정] : EarlyReturn 적용

 

1차 수정후에는 각 케이스별로 대응 코드를 바로 찾을 수 있어 가독성이 증가합니다.

또한 각각의 케이스 별로 바로 바로 리턴 하기때문에 한 케이스만 확인해야한다면 불필요하게 아래 코드를 읽지 않아도 되고

만약 전체 메서드를 검사하는 관점에서도 각 케이스는 이미 리턴되었기에 더이상 고려 대상이 아니기에 아래 남은 케이스에 더욱 집중할 수도 있게 되었습니다.

 

그러나 해당 메서드는 많은 역할이 혼재되어 있다는 점에서 가독성이 떨어집니다. 현재 코드에는 어떤 케이스인지 판단하는 코드, 결과를 만들어내는 코드가 존재합니다.  또한 각 케이스를 판단하는 코드는 주석이 없다면 무엇을 뜻하는지도 인지하기가 어렵습니다.

private TrainerDescriptionResult getTrainerDescriptionResult(Long traineeId, Long workoutId, Long trainerId) {
    List<WorkoutCustomDescription> specificDescriptions =
            workoutCustomDescRepository.findByCreatorIdAndTargetUserIdAndWorkoutId(trainerId, traineeId, workoutId);

    // 특정 회원에 대한 설명이 있는 경우
    if (!specificDescriptions.isEmpty()) {
        return new TrainerDescriptionResult(
                specificDescriptions.stream().map(WorkoutCustomDescription::getDescription).toList(),
                WorkoutDescriptionType.SPECIFIC
        );
    }

    // 특정 회원에 대한 설명이 없는 경우, 기본 설명 조회
    List<WorkoutCustomDescription> defaultDescriptions =
            workoutCustomDescRepository.findDefaultDescriptionByCreatorIdAndWorkoutId(trainerId, workoutId);

    if (!defaultDescriptions.isEmpty()) {
        return new TrainerDescriptionResult(
                defaultDescriptions.stream().map(WorkoutCustomDescription::getDescription).toList(),
                WorkoutDescriptionType.COMMON
        );
    }

    // 기본 설명도 없는 경우
    if (WorkoutCategory.isCustomWorkout(workoutId)) {
        return new TrainerDescriptionResult(Collections.emptyList(), WorkoutDescriptionType.COMMON);
    }

    WorkoutCategory workoutCategory = WorkoutCategory.fromCode(workoutId.intValue());
    return new TrainerDescriptionResult(workoutCategory.getSteps(), WorkoutDescriptionType.FITSYOU);
}

 

[2차 수정] : 메서드 분리

판단하는 로직이 함수로 정의 되면서 해당 케이스가 무엇을 의미하는지 주석없이도 알 수 있게되었습니다.

또 케이스들을 어떤 순서로 검사해야하는지도 흐름이 명확히 보이게 되었습니다. 

private TrainerDescriptionResult getTrainerDescriptionResult(Long traineeId, Long workoutId, Long trainerId) {
    if (hasSpecificDescription(trainerId, traineeId, workoutId)) {
        return getSpecificDescription(trainerId, traineeId, workoutId);
    }

    if (hasDefaultDescription(trainerId, workoutId)) {
        return getDefaultDescription(trainerId, workoutId);
    }

    return getFallbackDescription(workoutId);
}

/**
 * 특정 회원에 대한 설명이 있는지 확인
 */
private boolean hasSpecificDescription(Long trainerId, Long traineeId, Long workoutId) {
    return !workoutCustomDescRepository.findByCreatorIdAndTargetUserIdAndWorkoutId(trainerId, traineeId, workoutId).isEmpty();
}

/**
 * 특정 회원에 대한 설명을 가져옴
 */
private TrainerDescriptionResult getSpecificDescription(Long trainerId, Long traineeId, Long workoutId) {
    List<String> descriptions = workoutCustomDescRepository.findByCreatorIdAndTargetUserIdAndWorkoutId(trainerId, traineeId, workoutId)
            .stream().map(WorkoutCustomDescription::getDescription).toList();
    return new TrainerDescriptionResult(descriptions, WorkoutDescriptionType.SPECIFIC);
}

/**
 * 기본 설명이 있는지 확인
 */
private boolean hasDefaultDescription(Long trainerId, Long workoutId) {
    return !workoutCustomDescRepository.findDefaultDescriptionByCreatorIdAndWorkoutId(trainerId, workoutId).isEmpty();
}

/**
 * 기본 설명을 가져옴
 */
private TrainerDescriptionResult getDefaultDescription(Long trainerId, Long workoutId) {
    List<String> descriptions = workoutCustomDescRepository.findDefaultDescriptionByCreatorIdAndWorkoutId(trainerId, workoutId)
            .stream().map(WorkoutCustomDescription::getDescription).toList();
    return new TrainerDescriptionResult(descriptions, WorkoutDescriptionType.COMMON);
}

/**
 * 기본 설명도 없을 때의 처리 (커스텀 운동 여부에 따라 다름)
 */
private TrainerDescriptionResult getFallbackDescription(Long workoutId) {
    if (WorkoutCategory.isCustomWorkout(workoutId)) {
        return new TrainerDescriptionResult(Collections.emptyList(), WorkoutDescriptionType.COMMON);
    }
    WorkoutCategory workoutCategory = WorkoutCategory.fromCode(workoutId.intValue());
    return new TrainerDescriptionResult(workoutCategory.getSteps(), WorkoutDescriptionType.FITSYOU);
}

 

 

Enum 

중첩 조건에 대해서 그 케이스가 많고 내용을 많이 담고 있다면 Enum 화하여 관리하는 것이 좋을 수 있습니다.

가독성도 올라가고 관리 포인트도 한정되어 유지보수에도 유리합니다.

또 enum 으로 관리할 경우 enum 메서드를 활용함으로써 객체지향적인 코드를 작성할 수도 있습니다(이 부분은 객체지향적 리펙토링 부분에서 다룰 예정)

다만 Enum 을 새롭게 정의하는 방법인만큼 꼭 필요한 상황인지에 대한 검토가 필요합니다

 

[수정전]

private InbodyHistoryDto.Spec getBodyFatPercentagePhyscialSpec1(Double percentage, LocalDate birthdate, Gender gender) {
    int age = LocalDate.now().getYear() - birthdate.getYear();
    InbodyHistoryDto.Spec spec;
    if (gender == Gender.MALE) {
        if (age <= 39) {
            if (percentage <= 7) spec = InbodyHistoryDto.Spec.LOWER_THAN_STANDARD;
            else if (isWithInRange(percentage, 8, 19)) spec = InbodyHistoryDto.Spec.STANDARD;
            else spec = InbodyHistoryDto.Spec.HIGHER_THAN_STANDARD;
        } else if (age <= 59) {
            if (percentage <= 10) spec = InbodyHistoryDto.Spec.LOWER_THAN_STANDARD;
            else if (isWithInRange(percentage, 11, 21)) spec = InbodyHistoryDto.Spec.STANDARD;
            else spec = InbodyHistoryDto.Spec.HIGHER_THAN_STANDARD;
        } else {
            if (percentage <= 12) spec = InbodyHistoryDto.Spec.LOWER_THAN_STANDARD;
            else if (isWithInRange(percentage, 13, 24)) spec = InbodyHistoryDto.Spec.STANDARD;
            else spec = InbodyHistoryDto.Spec.HIGHER_THAN_STANDARD;
        }
    } else {
        if (age <= 39) {
            if (percentage <= 20) spec = InbodyHistoryDto.Spec.LOWER_THAN_STANDARD;
            else if (isWithInRange(percentage, 21, 32)) spec = InbodyHistoryDto.Spec.STANDARD;
            else spec = InbodyHistoryDto.Spec.HIGHER_THAN_STANDARD;
        } else if (age <= 59) {
            if (percentage <= 22) spec = InbodyHistoryDto.Spec.LOWER_THAN_STANDARD;
            else if (isWithInRange(percentage, 23, 33)) spec = InbodyHistoryDto.Spec.STANDARD;
            else spec = InbodyHistoryDto.Spec.HIGHER_THAN_STANDARD;
        } else {
            if (percentage <= 23) spec = InbodyHistoryDto.Spec.LOWER_THAN_STANDARD;
            else if (isWithInRange(percentage, 24, 35)) spec = InbodyHistoryDto.Spec.STANDARD;
            else spec = InbodyHistoryDto.Spec.HIGHER_THAN_STANDARD;
        }
    }
    return spec;
}

[수정후]

private InbodyHistoryDto.Spec getBodyFatPercentagePhyscialSpec(Double percentage, LocalDate birthdate, Gender gender) {
    int age = LocalDate.now().getYear() - birthdate.getYear();
    BodyFatStandard standard = BodyFatStandard.getStandard(gender, age);

    if (percentage <= standard.lower) return InbodyHistoryDto.Spec.LOWER_THAN_STANDARD;
    if (isWithInRange(percentage, standard.standardMin, standard.standardMax)) return InbodyHistoryDto.Spec.STANDARD;
    return InbodyHistoryDto.Spec.HIGHER_THAN_STANDARD;
}

private enum BodyFatStandard {
    MALE_39(Gender.MALE, 0, 39, 7, 8, 19),
    MALE_59(Gender.MALE, 40, 59, 10, 11, 21),
    MALE_60(Gender.MALE, 60, Integer.MAX_VALUE, 12, 13, 24),
    FEMALE_39(Gender.FEMALE, 0, 39, 20, 21, 32),
    FEMALE_59(Gender.FEMALE, 40, 59, 22, 23, 33),
    FEMALE_60(Gender.FEMALE, 60, Integer.MAX_VALUE, 23, 24, 35);

    private final Gender gender;
    private final int minAge;
    private final int maxAge;
    private final int lower;
    private final int standardMin;
    private final int standardMax;

    BodyFatStandard(Gender gender, int minAge, int maxAge, int lower, int standardMin, int standardMax) {
        this.gender = gender;
        this.minAge = minAge;
        this.maxAge = maxAge;
        this.lower = lower;
        this.standardMin = standardMin;
        this.standardMax = standardMax;
    }

    public static BodyFatStandard getStandard(Gender gender, int age) {
        return Arrays.stream(values())
                .filter(s -> s.gender == gender && age >= s.minAge && age <= s.maxAge)
                .findFirst()
                .orElseThrow(() -> new IllegalArgumentException("Invalid age range"));
    }
}

 

---

 

EarlyReturn 추가정보

• https://medium.com/swlh/return-early-pattern-3d18a41bba8

Return Early Pattern

 

•  https://thearchivelog.dev/article/are-early-returns-any-good/

Early Return은 과연 좋은가? | 아카이브-로그