리팩토링 12. 반복문 쪼개기

들어가기 전

이 글은 인프런 백기선님의 강의를 복습하며 작성한 글입니다. 

---

리팩토링 12. 반복문 쪼개기 (Split Loop)

• 하나의 반복문 내에서 여러 작업을 동시에 처리하는 코드를 쉽게 찾아볼 수 있음.
  • 이런 코드는 효율적이지만, 읽기 어렵다.
  • 해당 반복문을 수정할 때 여러 작업을 모두 고려하며 코딩을 해야하기 때문이다.
• 반복문을 여러 개로 쪼개고, 각 반복문에서 각 작업을 하도록 하면 유지보수가 쉬워진다. 
• 반복문 쪼개기는 성능 문제를 야기할 수 있지만, '리팩토링'은 '성능 최적화'와 별개의 작업이다.
  • 리팩토링을 마친 이후에 병목 지점인 경우 성능 최적화를 시도해 볼 수 있다. 

 

일반적으로 하나의 반복문 안에서 여러 작업을 동시에 진행하는 경우가 많다. 어차피 루프를 돌고 있으니, 똑같은 루프를 여러 번 만들기 보다는 한번 루프를 돌면서 여러 작업을 하는 것이 더욱 효율적이기 때문이다. 그렇지만 반복문 하나가 길면 반복문 안의 코드를 수정할 때 모든 작업을 고려하며 수정해야 한다는 문제점이 있다.

따라서 반복문안의 코드가 전체 어플리케이션 성능의 병목지점이 아니라면 일단은 반복문을 각각의 작업으로 분리해서 사용하도록 한다. 이후 병목 지점으로 바뀔 경우, 해당 문제를 개선하는 것이 좋다. 

 

---

리팩토링 하기 (Before)

아래 코드를 살펴보자. For문을 리팩토링 할 것이기 때문에 For문을 보자. 현재 For 문 내부에선 두 가지 동작이 수행된다. 

• 각각의 이슈마다 과제를 한 사람을 찾으면서, 과제를 했는지 체크한다.
• 가장 먼저 과제를 제출한 사람을 찾는다.

하나의 반복문 내부에서 두 가지 작업이 진행되는 것을 볼 수 있다. 이번 리팩토링은 한 루프에서 한 가지 작업만 하도록 하는 것이다. 따라서 하나의 루프를 두 개로 쪼개서 작업을 진행하도록 한다. 

1. firstCreatedAt, first 변수들은 가장 먼저 과제를 제출한 사람을 찾을 때 사용된다. 따라서 문장 슬라이드를 통해서 문맥을 맞추어 준다.
2. 이후 2개의 루프로 나눈 후, 각각의 작업을 이동시켜준다.
3. 2개의 루프를 각각의 메서드로 추출해준다. 이 때 메서드의 이름은 정확한 의미를 표현할 수 있도록 수정한다. 

private void print() throws IOException, InterruptedException {
    GHRepository ghRepository = getGhRepository();

    ExecutorService service = Executors.newFixedThreadPool(8);
    CountDownLatch latch = new CountDownLatch(totalNumberOfEvents);

    // 여러 작업이 하나의 루프에서 반복되고 있음.
    // 이걸 그대로 메서드로 빼면 전달해야하는 매개변수 갯수가 증가함. 따라서 위의 병렬처리 관련 부분도 같이 메서드로 처리
    for (int index = 1 ; index <= totalNumberOfEvents ; index++) {
        int eventId = index;
        service.execute(new Runnable() {
            @Override
            public void run() {
                try {
                    GHIssue issue = ghRepository.getIssue(eventId);
                    List<GHIssueComment> comments = issue.getComments();
                    Date firstCreatedAt = null;
                    Participant first = null;

                    // 여러 작업이 하나의 루프에서 반복되고 있음.
                    for (GHIssueComment comment : comments) {
                        Participant participant = findParticipant(comment.getUserName(), participants);
                        participant.setHomeworkDone(eventId);

                        if (firstCreatedAt == null || comment.getCreatedAt().before(firstCreatedAt)) {
                            firstCreatedAt = comment.getCreatedAt();
                            first = participant;
                        }
                    }

                    firstParticipantsForEachEvent[eventId - 1] = first;
                    latch.countDown();
                } catch (IOException e) {
                    throw new IllegalArgumentException(e);
                }
            }
        });
    }

    latch.await();
    service.shutdown();

    new StudyPrinter(this.totalNumberOfEvents, this.participants).execute();
    printFirstParticipants();
}

또한 Participants 객체를 한번 살펴보자. 각 루프를 메서드로 추출하면, 각 루프가 Participants 객체에 의존적이라는 것을 알 수 있다. 따라서 Participants는 특정 메서드에서만 쓰이는 것이 아니라, 클래스 전체에서 공용으로 쓰인다고 판단할 수 있다. 따라서 Participants를 로컬이 아닌 클래스 변수로 올려주고 참조하도록 수정할 수 있다. 

또한 현재는 병렬처리하는 부분의 코드도 구현 세부 사항을 나타내고 있기 때문에 굉장히 읽기 어렵다는 단점이 있다. 이걸 함수로 추출하면 가독성이 더 좋아질 것이다. 그런데 병렬처리하는 부분의 For문만 추출하게 되면 전달되어야 하는 매개변수가 많아진다. 예를 들어 CountLatch와 ExecutorService를 전달해야한다. 그런데 이 녀석들은 이 메서드 내부에서만 사용되는 것이기 때문에 굳이 매개변수로 전달하기 보다는 함수에서 직접 실행하는 형식으로 하는 것이 메서드의 매개변수를 줄이는데 더욱 도움이 될 것 같다. 

정리하면 다음 리팩토링을 진행해야 한다.

1. For문 하나 당 하나의 작업만 하도록 수정한다.
2. 각 For문을 메서드로 빼서 좀 더 의미를 알기 쉽게 바꾸어준다.
3. 병렬처리하는 For문도 메서드로 빼서 좀 더 의미를 알기 쉽게 바꾸어준다. 이 때 매개변수를 줄이기 위해 ExecutorService 등은 메서드 내부에서 생성하도록 한다. 

 

---

After(리팩토링 결과)

아래와 같이 리팩토링을 마무리 할 수 있게 된다.

public class StudyDashboard {

    // 필드로 추출
    private final List<Participant> participants;
    ...
    
    private void print() throws IOException, InterruptedException {
        // 리팩토링 완료
        GHRepository ghRepository = getGhRepository();
        checkGithubIssue(ghRepository);
        new StudyPrinter(this.totalNumberOfEvents, this.participants).execute();
        printFirstParticipants();
    }

    private void checkGithubIssue(GHRepository ghRepository) throws InterruptedException {
        ExecutorService service = Executors.newFixedThreadPool(8);
        CountDownLatch latch = new CountDownLatch(totalNumberOfEvents);

        for (int index = 1 ; index <= totalNumberOfEvents ; index++) {
            int eventId = index;
            service.execute(() -> {
                try {
                    GHIssue issue = ghRepository.getIssue(eventId);
                    List<GHIssueComment> comments = issue.getComments();
                    checkHomework(comments, eventId);
                    firstParticipantsForEachEvent[eventId - 1] = findFirst(comments);
                    latch.countDown();
                } catch (IOException e) {
                    throw new IllegalArgumentException(e);
                }
            });
        }

        latch.await();
        service.shutdown();
    }

    private Participant findFirst(List<GHIssueComment> comments) throws IOException {
        Date firstCreatedAt = null;
        Participant first = null;
        for (GHIssueComment comment : comments) {
            Participant participant = findParticipant(comment.getUserName(), participants);
            if (firstCreatedAt == null || comment.getCreatedAt().before(firstCreatedAt)) {
                firstCreatedAt = comment.getCreatedAt();
                first = participant;
            }
        }
        return first;
    }

    private void checkHomework(List<GHIssueComment> comments, int eventId) {
        for (GHIssueComment comment : comments) {
            Participant participant = findParticipant(comment.getUserName(), participants);
            participant.setHomeworkDone(eventId);
        }
    }

    private void printFirstParticipants() {
        Arrays.stream(this.firstParticipantsForEachEvent).forEach(p -> System.out.println(p.username()));
    }

    private GHRepository getGhRepository() throws IOException {
        GitHub gitHub = GitHub.connect();
        GHRepository repository = gitHub.getRepository("whiteship/live-study");
        return repository;
    }

    private Participant findParticipant(String username, List<Participant> participants) {
        return isNewParticipant(username, participants) ?
                createNewParticipant(username, participants) :
                findExistingParticipant(username, participants);
    }

    private Participant findExistingParticipant(String username, List<Participant> participants) {
        Participant participant;
        participant = participants.stream().filter(p -> p.username().equals(username)).findFirst().orElseThrow();
        return participant;
    }

    private Participant createNewParticipant(String username, List<Participant> participants) {
        Participant participant;
        participant = new Participant(username);
        participants.add(participant);
        return participant;
    }

    private boolean isNewParticipant(String username, List<Participant> participants) {
        return participants.stream().noneMatch(p -> p.username().equals(username));
    }

}