8. 깔끔한 코드로 리팩토링 하기

1. 작은 리팩토링

리팩토링은 기존 기능을 그대로 유지하면서 코드의 하부 구조를 변경하는 것을 말한다.

1. 리팩토링의 기회

public class Profile {

   private Map<String,Answer> answers = new HashMap<>();
   // ...

   private int score;
   private String name;

   public Profile(String name) {
      this.name = name;
   }
   
   public String getName() {
      return name;
   }

   public void add(Answer answer) {
      answers.put(answer.getQuestionText(), answer);
   }
   
   public boolean matches(Criteria criteria) {
      score = 0;
      
      boolean kill = false;
      boolean anyMatches = false;
      for (Criterion criterion: criteria) {
         Answer answer = answers.get(
               criterion.getAnswer().getQuestionText());
         boolean match = 
               criterion.getWeight() == Weight.DontCare ||
               answer.match(criterion.getAnswer());
         if (!match && criterion.getWeight() == Weight.MustMatch) {
            kill = true;
         }
         if (match) {
            score += criterion.getWeight().getValue();
         }
         anyMatches |= match;
         // ...
      }
      if (kill)
         return false;
      return anyMatches;
   }

   public int score() {
      return score;
   }

   public List<Answer> classicFind(Predicate<Answer> pred) {
      List<Answer> results = new ArrayList<Answer>();
      for (Answer answer: answers.values())
         if (pred.test(answer))
            results.add(answer);
      return results;
   }
   
   @Override
   public String toString() {
     return name;
   }

   public List<Answer> find(Predicate<Answer> pred) {
      return answers.values().stream()
            .filter(pred)
            .collect(Collectors.toList());
   }
}

Profile 클래스의 matches() 메서드를 보면 조건문이 복잡하다. 따라서 matches() 메서드의 복잡도롤 줄여 코드가 무엇을 담당하는지 이해하기 쉽도록 만들고 싶을 수 있다.

2. 메서드 추출 : 두 번째로 중요한 리팩토링 친구

boolean match = 
    criterion.getWeight() == Weight.DontCare ||
    answer.match(criterion.getAnswer());

이 부분을 별도의 메서드로 추출하여 복잡성을 고립시킨다. 그러면 반복문에는 단순한 선언문만 남게 된다. match 변수는 조건이 답변에 맞는지 여부만 나타낸다.

public boolean matches(Criteria criteria) {
    score = 0;
      
    boolean kill = false;
    boolean anyMatches = false;
    for (Criterion criterion: criteria) {
        Answer answer = answers.get(
            criterion.getAnswer().getQuestionText());
        boolean match = matches(criterion, answer);

        if (!match && criterion.getWeight() == Weight.MustMatch) {
           kill = true;
        }
        if (match) {
           score += criterion.getWeight().getValue();
        }
        anyMatches |= match;
     }
    if (kill)
        return false;
    return anyMatches;
 }

private boolean matches(Criterion criterion, Answer answer) {
    return criterion.getWeight() == Weight.DontCare ||
        answer.match(criterion.getAnswer());
   }

2. 메서드를 위한 더 좋은 집 찾기

새롭게 추출된 matches() 메서드는 Profile 객체와 관련이 없고 Answer 클래스와 Criterion 클래스와 관련이 있다. 따라서 matches() 메서드를 Criterion 클래스로 이동시킨다. Criterion 객체는 이미 Answer 객체들을 알고 있지만, 그 역은 성립하지 않기 때문이다. 즉, Answer 클래스는 Criterion 클래스를 의존하지 않는다. matches() 메서드를 Answer 클래스로 이동했다면 이는 양방향 의존 관계가 된다.

public class Criterion implements Scoreable {
   // ... 
   private Weight weight;
   private Answer answer;
   private int score;

   public Criterion(Answer answer, Weight weight) {
      this.answer = answer;
      this.weight = weight;
   }
   
   public Answer getAnswer() { return answer; }
   public Weight getWeight() { return weight; }
   
   public void setScore(int score) { this.score = score; }
   public int getScore() { return score; }

   public boolean matches(Answer answer) {
      return getWeight() == Weight.DontCare ||
          answer.match(getAnswer());
   }
}

이동한 후에 Profile 클래스의 반복문은 다음과 같다.

for (Criterion criterion: criteria) {
    Answer answer = answers.get(
        criterion.getAnswer().getQuestionText());
    boolean match = criterion.matches(answer);

    if (!match && criterion.getWeight() == Weight.MustMatch) {
        kill = true;
    }
    if (match) {
        score += criterion.getWeight().getValue();
    }
    anyMatches |= match;
}

또한 answer 지역 변수에 할당하는 문장은 꽤 길고 복잡하다.

Answer answer = answers.get(
    criterion.getAnswer().getQuestionText());

위의 코드는 디메테르의 법칙(the Law of Demeter)(요약하면 다른 객체로 전파되는 연쇄적인 메서드 호출을 피해야 함)을 위반하고 깔끔하지도 않다.

이것을 개선하는 첫 번째 단계는 answer 할당문의 우변을 새로운 메서드인 answerMathcing() 메서드로 추출하는 것이다.

public boolean matches(Criteria criteria) {
    score = 0;

    boolean kill = false;
    boolean anyMatches = false;
    for (Criterion criterion: criteria) {
        Answer answer = answerMatching(criterion);
        boolean match = criterion.matches(answer);

        if (!match && criterion.getWeight() == Weight.MustMatch) {
           kill = true;
        }
        if (match) {
           score += criterion.getWeight().getValue();
        }
        anyMatches |= match;
    }
    if (kill)
        return false;
    return anyMatches;
}

private Answer answerMatching(Criterion criterion) {
    return answers.get(criterion.getAnswer().getQuestionText());
}

임시 변수들은 쓰임새가 다양하다. 임시 변수로 값비싼 비용의 계산 값을 캐시에 넣거나 메서드 몸체에서 변경되는 것들을 수집한다. 또한 임시 변수는 코드 의도를 명확하게 하는 데 쓰일 수 있다.

3. 자동 및 수동 리팩토링

여기서 answer 지역 변수는 코드의 명확성을 높이지 않고 한 번만 사용한다. 변수를 제거하고 answerMatching(criterion) 표현을 인라인한다.

for (Criterion criterion: criteria) {
    boolean match = criterion.matches(answerMatching(criterion));

    if (!match && criterion.getWeight() == Weight.MustMatch) {
        kill = true;
    }
    if (match) {
        score += criterion.getWeight().getValue();
    }
    anyMatches |= match;
}

또한 대부분의 IDE는 인라인 리팩토링을 자동화하고 있다.

이제 matches() 메서드의 세부 사항을 제거했기 때문에 이제 고수준의 정책을 쉽게 이해할 수 있다. 따라서 메서드의 핵심 목표를 구별할 수 있다.

매칭되는 조건의 가중치를 합하여 점수를 계산한다.
필수(must-match) 항목이 프로파일 답변과 매칭되지 않으면 false를 반환한다.
그렇지 않고 매칭되는 것이 있으면 true를 반환하고, 매칭되는 것이 없으면 false를 반환한다.

이제 matches() 메서드에서 어떤 프로파일이 매치되는지 여부를 결정하는 부분을 리팩토링한다.

 public boolean matches(Criteria criteria) {
     score = 0;

     boolean kill = false;
     for (Criterion criterion: criteria) {
         boolean match = criterion.matches(answerMatching(criterion));

         if (!match && criterion.getWeight() == Weight.MustMatch) {
             kill = true;
         }
         if (match) {
             score += criterion.getWeight().getValue();
         }
     }
     if (kill)
         return false;
     return anyMatches(criteria);
}

private boolean anyMatches(Criteria criteria) {
    boolean anyMatches = false;
    for (Criterion criterion: criteria)
        anyMatches = criterion.matches(answerMatching(criterion));
    return anyMatches;
}

서로 떨어진 코드들을 새로운 메서드로 추출할 때는 자동화하는 방법이 없기 때문에 수동으로 리팩토링해야 한다. 다음 테스트는 실패한다.

@Test
public void matchAnswersTrueWhenAnyOfMultipleCriteriaMatch() {
    profile.add(answerThereIsRelocation);
    profile.add(answerDoesNotReimburseTuition);
    criteria.add(new Criterion(answerThereIsRelocation, Weight.Important));
    criteria.add(new Criterion(answerReimbursesTuition, Weight.Important));

    boolean matches = profile.matches(criteria);

    assertTrue(matches);
}

이에 대한 해결책은 anyMatches 값을 갱신할 때 복합 할당 연산자(|=)를 사용하는 것이다.

 private boolean anyMatches(Criteria criteria) {
     boolean anyMatches = false;
     for (Criterion criterion: criteria)
         anyMatches |= criterion.matches(answerMatching(criterion));
     return anyMatches;
}

4. 과한 리팩토링?

이와 유사하게 모든 매칭의 전체 가중치를 계산하는 코드를 추출한다.

 public boolean matches(Criteria criteria) {
     calculateScore(criteria);
      
     boolean kill = false;
     for (Criterion criterion: criteria) {
         boolean match = criterion.matches(answerMatching(criterion));
         if (!match && criterion.getWeight() == Weight.MustMatch) {
             kill = true;
         }
    }
    if (kill)
        return false;
    return anyMatches(criteria);
}

private void calculateScore(Criteria criteria) {
    score = 0;
    for (Criterion criterion: criteria) 
        if (criterion.matches(answerMatching(criterion))) 
            score += criterion.getWeight().getValue();
}

마지막으로 매치되지 않는 어떤 필수(must-meet) 조건이 있는지 여부를 결정하는 로직을 추출한다.

public boolean matches(Criteria criteria) {
    calculateScore(criteria);
    if (doesNotMeetAnyMustMatchCriterion(criteria))
        return false;
    return anyMatches(criteria);
}

private boolean doesNotMeetAnyMustMatchCriterion(Criteria criteria) {
    for (Criterion criterion: criteria) {
        boolean match = criterion.matches(answerMatching(criterion));
        if (!match && criterion.getWeight() == Weight.MustMatch) 
            return true;
    }
    return false;
}

이제 새로운 메서드와 반복문이 각각 세 개가 되었다. 이에 따른 성능 시사점과 이득을 분석해보자.

1. 보상 : 명확하고 테스트 가능한 단위들

matches() 메서드는 이제 즉시 이해할 수 있을 정도로 전체 알고리즘이 깔끔하게 정리되었다. 현재 코드는 다음 순서의 알고리즘을 따른다.

주어진 조건에 따라 점수를 계산한다.
프로파일이 어떤 필수 조건에 부합하지 않으면 false를 반환한다.
그렇지 않으면 어떤 조건에 맞는지 여부를 반환한다.

알고리즘에 있는 세 단계의 각 구현 세부 사항은 다음 도우미 메서드인 calculateScore(), doesMeetAnyMustMatchCriterion()과 anyMatches()에 숨겨져 있다. 각 도우미 메서드는 명확하고 고립된 방식으로 잘 표현되어 있다.

2. 성능 염려 : 그러지 않아도 된다

matches() 메서드를 리팩토링한 결과 anyMatches(), calculateScore(), doesNotmeetAnyMustMatchCriterion() 메서드 각각에 criterion 조건에 대한 반복문을 갖게 되었다. 새로운 반복문 세 개로 matches() 메서드는 잠재적으로 실행 시간이 네 배가 되었다.

성능이 즉시 문제되지 않으면 최적화 노력으로 시간을 낭비하기 보다는 코드를 깔끔하게 유지해야 한다. 최적화된 코드는 여러 방면에서 문제의 소지가 있다. 일반적으로 코드 가독성이 낮고 유지 보수 비용이 증가하고 설계 또한 유연하지 않다.

반대로 깔끔한 설계는 성능을 위해 최적화할 때 즉시 대응할 수 있다. 깔끔한 설계는 코드를 이동시킬 수 있는 유연성을 제공하고 다른 알고리즘을 적용하는 데도 수월하다.

성능이 당장 문제된다면 다른 일을 하기 전에 먼저 문제가 얼마나 심각한지 성능을 측정하고 작은 테스트 코드를 만들어 예전 코드의 속도가 어느 정도인지 확인하고, 리팩토링한 코드는 몇 퍼 센트의 성능 저하가 있는지 판단하고 비교한다.

참고

자바와 JUnit을 활용한 실용주의 단위 테스트
https://github.com/gilbutITbook/006814

Author And Source

이 문제에 관하여(8. 깔끔한 코드로 리팩토링 하기), 우리는 이곳에서 더 많은 자료를 발견하고 링크를 클릭하여 보았다 https://velog.io/@jsj3282/8.-깔끔한-코드로-리팩토링-하기

우수한 개발자 콘텐츠 발견에 전념 (Collection and Share based on the CC Protocol.)