DEV ℧ Developer Diary

[Refactoring] 냄새 4. 긴 매개변수 목록

29 Sep 2022

해당 포스트는 inflearn의 백기선님의 강의인 리팩토링 을 듣고 정리한 글입니다.

냄새 4. 긴 매개변수 목록

  • 어떤 함수에 매개변수가 많을수록 함수의 역할을 이해하기 어려워진다.
    • 과연 그 함수는 한가지 일을 하고 있는게 맞는가?
    • 불필요한 매개변수는 없는가?
    • 하나의 레코드로 뭉칠 수 있는 매개변수 목록은 없는가?
  • 어떤 매개변수를 다른 매개변수를 통해 알아낼 수 있다면, “매개변수를 질의 함수로 바꾸기 (Replace Parameter with Query)”를 사용할 수 있다.
  • 기존 자료구조에서 세부적인 데이터를 가져와서 여러 매개변수로 넘기는 대신, “객체 통째로 넘기기 (Preserve Whole Object)”를 사용할 수 있다.
  • 일부 매개변수들이 대부분 같이 넘겨진다면, “매개변수 객체 만들기 (Introduce Parameter Object)”를 적용할 수 있다.
  • 매개변수가 플래그로 사용된다면, “플래그 인수 제거하기 (Remove Flag Argument)”를 사용할 수 있다.
  • 여러 함수가 일부 매개변수를 공통적으로 사용한다면 “여러 함수를 클래스로 묶기 (Combine Functions into Class)”를 통해 매개변수를 해당 클래스의 필드로 만들고 메서드에 전달해야 할 매개변수 목록을 줄일 수 있다.

리팩토링 14. 매개변수를 질의 함수로 바꾸기

  • 함수의 매개변수 목록은 함수의 다양성을 대변하며, 짧을수록 이해하기 좋다. 길어질수록 많은 책임을 가질수 있기 때문에 안좋을 수 있다.
  • 어떤 한 매개변수를 다른 매개변수를 통해 알아낼 수 있다면 “중복 매개변수”라 생각할 수 있다.
  • 매개변수에 값을 전달하는 것은 “함수를 호출하는 쪽”의 책임이다. 가능하면 함수를 호출하는 쪽의 책임을 줄이고 함수 내부에서 책임지도록 노력한다.
  • “임시 변수를 질의 함수로 바꾸기”와 “함수 선언 변경하기”를 통해 이 리팩토링을 적용한다.
  • 새로운 의존성이 생긴다면? 매개변수를 변경하는 것이 옳은지 다시 한번 고민해봐야한다.

예제코드

  • Order
public class Order {

    private int quantity;

    private double itemPrice;

    public Order(int quantity, double itemPrice) {
        this.quantity = quantity;
        this.itemPrice = itemPrice;
    }

    public double finalPrice() {
        double basePrice = this.quantity * this.itemPrice;
        int discountLevel = this.quantity > 100 ? 2 : 1;
        return this.discountedPrice(basePrice, discountLevel);
    }

    private double discountedPrice(double basePrice, int discountLevel) {
        return discountLevel == 2 ? basePrice * 0.9 : basePrice * 0.95;
    }
}
  • OrderTest
class OrderTest {

    @Test
    void discountedPriceWithDiscountLevel2() {
        int quantity = 200;
        double price = 100;
        assertEquals(quantity * price * 0.90, new Order(quantity, price).finalPrice());
    }

    @Test
    void discountedPriceWithDiscountLevel1() {
        int quantity = 100;
        double price = 100;
        assertEquals(quantity * price * 0.95, new Order(quantity, price).finalPrice());
    }

}

리팩토링 전에는 반드시 테스트코드가 있어야 한다. 그래야 리팩토링이 잘 진행되었는지 확인할 수 가 있다.

먼저 코드의 리팩토링을 진행하기 전에 테스트코드를 실행 시켜주어 정상 작동을 확인한다.

리팩토링001

정상적으로 작동한것을 확인했으면 이제 코드 리팩토링을 진행해보도록 하자.

public double finalPrice() {
    double basePrice = this.quantity * this.itemPrice;
    int discountLevel = this.quantity > 100 ? 2 : 1;
    return this.discountedPrice(basePrice, discountLevel);
}

private double discountedPrice(double basePrice, int discountLevel) {
    return discountLevel == 2 ? basePrice * 0.9 : basePrice * 0.95;
}

discountedPrice 함수는 기본값과, 상품의 할인율을 가지고 최적의 가격을 계산해주는 로직이다. 여기서 질의함수로 넣어주는 변수를 함수로 빼줄 수 있다.

public double finalPrice() {
    double basePrice = this.quantity * this.itemPrice;
    int discountLevel = discountLevel();
    return this.discountedPrice(basePrice, discountLevel);
}

private int discountLevel() {
    return this.quantity > 100 ? 2 : 1;
}

또한 discountedPrice 에서 discountLevel을 매개변수로 받는것이 아니라, 빼준 함수로 직접적으로 호출 해준다면 discountedPrice에 넘겨줄 매개변수가 하나 줄어들게 된다.

public double finalPrice() {
    double basePrice = this.quantity * this.itemPrice;
    return this.discountedPrice(basePrice);
}

private int discountLevel() {
    return this.quantity > 100 ? 2 : 1;
}

private double discountedPrice(double basePrice) {
    return discountLevel() == 2 ? basePrice * 0.90 : basePrice * 0.95;
}

정말 또 중요한 순서가 남았는데, 코드를 변경한 후 다시한번 테스트 코드를 돌려서 제대로 동작하는지 확인해준다. 만약 기능이 달라지거나, 결과가 달라진다면, 개선이 아닌 변경이 되어버리므로 주의해주도록 하자!!

리팩토링 15. 플래그 인수 제거하기

  • 플래그는 보통 함수에 매개변수로 전달해서, 함수 내부의 로직을 분기하는데 사용한다.
  • 플래그를 사용한 함수는 차이를 파악하기 어렵다.
    • 플래그가 여러개인 경우 함수에 대한 파악이 어려워 진다.
    • bookConcert(customer, false), bookConcert(customer, true)
    • bookConcert(customer), premiumBookConcert(customer)
  • 조건문 분해하기 (Decompose Condition)를 활용할 수 있다.

에제코드

  • Order
public class Order {

    private LocalDate placedOn;
    private String deliveryState;

    public Order(LocalDate placedOn, String deliveryState) {
        this.placedOn = placedOn;
        this.deliveryState = deliveryState;
    }

    public LocalDate getPlacedOn() {
        return placedOn;
    }

    public String getDeliveryState() {
        return deliveryState;
    }
}
  • Shipment
public class Shipment {

    public LocalDate deliveryDate(Order order, boolean isRush) {
        if (isRush) {
            int deliveryTime = switch (order.getDeliveryState()) {
                case "WA", "CA", "OR" -> 1;
                case "TX", "NY", "FL" -> 2;
                default -> 3;
            };
            return order.getPlacedOn().plusDays(deliveryTime);
        } else {
            int deliveryTime = switch (order.getDeliveryState()) {
                case "WA", "CA" -> 2;
                case "OR", "TX", "NY" -> 3;
                default -> 4;
            };
            return order.getPlacedOn().plusDays(deliveryTime);
        }
    }
}
  • ShipmentTest
class ShipmentTest {

    @Test
    void deliveryDate() {
        LocalDate placedOn = LocalDate.of(2021, 12, 15);
        Order orderFromWA = new Order(placedOn, "WA");

        Shipment shipment = new Shipment();
        assertEquals(placedOn.plusDays(1), shipment.deliveryDate(orderFromWA, true));
        assertEquals(placedOn.plusDays(2), shipment.deliveryDate(orderFromWA, false));
    }

}

테스트 코드를 먼저 실행 해본뒤 정상 작동을 확인한다.

리팩토링001

여기서 중요한것이 테스트 코드를 보면 shipment.deliveryDate(orderFromWA, true), shipment.deliveryDate(orderFromWA, false) 메소드의 매개변수중 true, false의 역할을 쉽게 파악할 수 가 업다. 해당 리팩토링을 진행하고자 하는 이유라고 간단하게 생각하면 된다. 리팩토링은 코드의 가시성을 개선해주기 위함이다.

리팩토링을 진행해보도록 하자. 아까 deliveryDate를 통해 전달해 주던 매개변수의 정체는 위의 예제코드를 보면 알겠지만 deliveryDate(Order order, boolean isRush) 해당 메소드로 빠른 배송 유무를 뜻한다. 각각 true와 false로 빠른 배송인지 아닌지를 if (isRush) {} else {} 를 통해 분기처리를 한다.

먼저 빠른 배송을 선택했을 때와 아닐때에 대한 로직을 각각의 메소드로 분리해준다.

public LocalDate deliveryDate(Order order, boolean isRush) {
    if (isRush) {
        return rushDeliveryDate(order);
    } else {
        return regularDeliverDate(order);
    }
}

public LocalDate regularDeliverDate(Order order) {
    int deliveryTime = switch (order.getDeliveryState()) {
        case "WA", "CA" -> 2;
        case "OR", "TX", "NY" -> 3;
        default -> 4;
    };
    return order.getPlacedOn().plusDays(deliveryTime);
}

public LocalDate rushDeliveryDate(Order order) {
    int deliveryTime = switch (order.getDeliveryState()) {
        case "WA", "CA", "OR" -> 1;
        case "TX", "NY", "FL" -> 2;
        default -> 3;
    };
    return order.getPlacedOn().plusDays(deliveryTime);
}

그렇다면? 이제는 간단하다. 기존의 test코드에서 호출해주던 메소드를 상황에 맞는 메소드로 변경해주면 된다. 일전의 true 매개변수를 넘겨주던 메소드는 rushDeliveryDate 메소드를, false 변수를 넘겨주던 메소드는 regularDeliverDate를 호출했다.

이후 deliveryDate는 이제 사용하지 않으니 지워주도록 하자.

class ShipmentTest {

    @Test
    void deliveryDate() {
        LocalDate placedOn = LocalDate.of(2021, 12, 15);
        Order orderFromWA = new Order(placedOn, "WA");

        Shipment shipment = new Shipment();
        assertEquals(placedOn.plusDays(1), shipment.rushDeliveryDate(orderFromWA));
        assertEquals(placedOn.plusDays(2), shipment.regularDeliverDate(orderFromWA));
    }

}

다시한번 테스트코드를 실행시켜 정상적으로 작동하는지 확인해주면 리팩토링은 종료된다!

리팩토링 16. 여러 함수를 클래스로 묶기

  • 비슷한 매개변수 목록을 여러 함수에서 사용하고 있다면 해당 메소드를 모아서 클래스를 만들 수 있다.
  • 클래스 내부로 메소드를 옮기고, 데이터를 필드로 만들면 메소드에 전달해야 하는 매개변수 목록도 줄일 수 있다.

예제코드

  • Participant
public record Participant(String username, Map<Integer, Boolean> homework) {
    public Participant(String username) {
        this(username, new HashMap<>());
    }

    public void setHomeworkDone(int index) {
        this.homework.put(index, true);
    }
}
  • StudyDashboard
public class StudyDashboard {

    private final int totalNumberOfEvents;

    public StudyDashboard(int totalNumberOfEvents) {
        this.totalNumberOfEvents = totalNumberOfEvents;
    }

    public static void main(String[] args) throws IOException, InterruptedException {
        StudyDashboard studyDashboard = new StudyDashboard(15);
        studyDashboard.print();
    }

    private void print() throws IOException, InterruptedException {
        GitHub gitHub = GitHub.connect();
        GHRepository repository = gitHub.getRepository("whiteship/live-study");
        List<Participant> participants = new CopyOnWriteArrayList<>();

        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 = repository.getIssue(eventId);
                        List<GHIssueComment> comments = issue.getComments();

                        for (GHIssueComment comment : comments) {
                            String username = comment.getUserName();
                            boolean isNewUser = participants.stream().noneMatch(p -> p.username().equals(username));
                            Participant participant = null;
                            if (isNewUser) {
                                participant = new Participant(username);
                                participants.add(participant);
                            } else {
                                participant = participants.stream().filter(p -> p.username().equals(username)).findFirst().orElseThrow();
                            }

                            participant.setHomeworkDone(eventId);
                        }

                        latch.countDown();
                    } catch (IOException e) {
                        throw new IllegalArgumentException(e);
                    }
                }
            });
        }

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

        try (FileWriter fileWriter = new FileWriter("participants.md");
             PrintWriter writer = new PrintWriter(fileWriter)) {
            participants.sort(Comparator.comparing(Participant::username));

            writer.print(header(participants.size()));

            participants.forEach(p -> {
                String markdownForHomework = getMarkdownForParticipant(p.username(), p.homework());
                writer.print(markdownForHomework);
            });
        }
    }

    private String getMarkdownForParticipant(String username, Map<Integer, Boolean> homework) {
        return String.format("| %s %s | %.2f%% |\n", username, checkMark(homework), getRate(homework));
    }

    /**
     * |:white_check_mark:|:white_check_mark:|:white_check_mark:|:x:|
     */
    private String checkMark(Map<Integer, Boolean> homework) {
        StringBuilder line = new StringBuilder();
        for (int i = 1 ; i <= this.totalNumberOfEvents ; i++) {
            if(homework.containsKey(i) && homework.get(i)) {
                line.append("|:white_check_mark:");
            } else {
                line.append("|:x:");
            }
        }
        return line.toString();
    }

    private double getRate(Map<Integer, Boolean> homework) {
        long count = homework.values().stream()
                .filter(v -> v == true)
                .count();
        return (double) (count * 100 / this.totalNumberOfEvents);
    }

    /**
     * | 참여자 (420) | 1주차 | 2주차 | 3주차 | 참석율 |
     * | --- | --- | --- | --- | --- |
     */
    private String header(int totalNumberOfParticipants) {
        StringBuilder header = new StringBuilder(String.format("| 참여자 (%d) |", totalNumberOfParticipants));

        for (int index = 1; index <= this.totalNumberOfEvents; index++) {
            header.append(String.format(" %d주차 |", index));
        }
        header.append(" 참석율 |\n");

        header.append("| --- ".repeat(Math.max(0, this.totalNumberOfEvents + 2)));
        header.append("|\n");

        return header.toString();
    }
}

먼저 StudyDashboard에서 주로 사용되고 있는 매개변수는 totalNumberOfEvents와 Participant 이다.

try (FileWriter fileWriter = new FileWriter("participants.md");
    PrintWriter writer = new PrintWriter(fileWriter)) {
    participants.sort(Comparator.comparing(Participant::username));

    writer.print(header(participants.size()));

    participants.forEach(p -> {
        String markdownForHomework = getMarkdownForParticipant(p.username(), p.homework());
        writer.print(markdownForHomework);
    });
}

위의 소스 뿐만 아니라, getMarkdownForParticipant, getRate, header의 메소드들은 각각 totalNumberOfEvents와 Participant만으로 사용이 가능한 메소드이다. 그러면 해당 메소드들에 매개변수를 줄이기 위해 totalNumberOfEvents와 Participant두개의 전역변수를 가지는 클래스를 만들어 보도록 하자.

public class StudyPrinter {
    private int totalNumberOfEvents;
    private List<Participant> participants;

    public StudyPrinter(int totalNumberOfEvents, List<Participant> participants) {
        this.totalNumberOfEvents = totalNumberOfEvents;
        this.participants = participants;
    }
}

Participant를 사용하는 markdown으로 추출하는 로직을 함수로 추출해서, StudyPrinter 로 옮겨주도록 하자. 해당 메소드를 빼서 옮겨주는 이유는, 분리한 전역변수들을 사용하는 기능끼리 묶어주기 위함이다.

print()를 호출 하던 부분은 아래와 같이 변경해줄 수 있다.

  • StudyDashboard
...
new StudyPrinter(this.totalNumberOfEvents, participants).print();
...
  • StudyPrinter
public class StudyPrinter {
    private int totalNumberOfEvents;
    private List<Participant> participants;

    public StudyPrinter(int totalNumberOfEvents, List<Participant> participants) {
        this.totalNumberOfEvents = totalNumberOfEvents;
        this.participants = participants;
    }

    private void print(List<Participant> participants) throws IOException {
        try (FileWriter fileWriter = new FileWriter("participants.md");
             PrintWriter writer = new PrintWriter(fileWriter)) {
            participants.sort(Comparator.comparing(Participant::username));

            writer.print(header(participants.size()));

            participants.forEach(p -> {
                String markdownForHomework = getMarkdownForParticipant(p.username(), p.homework());
                writer.print(markdownForHomework);
            });
        }
    }
}

이제 header이나, getMarkdownForParticipant에 빨간불이 들어온다. 해당 클래스에 메소드가 없기때문이다. 관련된 메소드들을 하나둘씩 옮겨 오도록 하자.

public class StudyPrinter {
  private int totalNumberOfEvents;
  private List<Participant> participants;

  public StudyPrinter(int totalNumberOfEvents, List<Participant> participants) {
    this.totalNumberOfEvents = totalNumberOfEvents;
    this.participants = participants;
  }

  public void print(List<Participant> participants) throws IOException {
    try (FileWriter fileWriter = new FileWriter("participants.md");
         PrintWriter writer = new PrintWriter(fileWriter)) {
      participants.sort(Comparator.comparing(Participant::username));

      writer.print(header(participants.size()));

      participants.forEach(p -> {
        String markdownForHomework = getMarkdownForParticipant(p.username(), p.homework());
        writer.print(markdownForHomework);
      });
    }
  }

  private String getMarkdownForParticipant(String username, Map<Integer, Boolean> homework) {
    return String.format("| %s %s | %.2f%% |\n", username, checkMark(homework), getRate(homework));
  }

  /**
   * |:white_check_mark:|:white_check_mark:|:white_check_mark:|:x:|
   */
  private String checkMark(Map<Integer, Boolean> homework) {
    StringBuilder line = new StringBuilder();
    for (int i = 1 ; i <= this.totalNumberOfEvents ; i++) {
      if(homework.containsKey(i) && homework.get(i)) {
        line.append("|:white_check_mark:");
      } else {
        line.append("|:x:");
      }
    }
    return line.toString();
  }

  private double getRate(Map<Integer, Boolean> homework) {
    long count = homework.values().stream()
            .filter(v -> v == true)
            .count();
    return (double) (count * 100 / this.totalNumberOfEvents);
  }

  /**
   * | 참여자 (420) | 1주차 | 2주차 | 3주차 | 참석율 |
   * | --- | --- | --- | --- | --- |
   */
  private String header(int totalNumberOfParticipants) {
    StringBuilder header = new StringBuilder(String.format("| 참여자 (%d) |", totalNumberOfParticipants));

    for (int index = 1; index <= this.totalNumberOfEvents; index++) {
      header.append(String.format(" %d주차 |", index));
    }
    header.append(" 참석율 |\n");

    header.append("| --- ".repeat(Math.max(0, this.totalNumberOfEvents + 2)));
    header.append("|\n");

    return header.toString();
  }
}

이제 선언된 전역변수와 중복되는 매개변수들을 지워 줄수 있다. 가급적 전역변수를 사용할 경우 this를 사용하면 쉽게 구분해줄 수가 있다.

먼저 print(List<Participant> participants) 에는 이제 전역변수 List<Participant> participants가 선언되어 있기 때문에 매개변수를 전달 해줄 필요가 없으므로 지워줄 수 있다.

public void print() throws IOException {
    try (FileWriter fileWriter = new FileWriter("participants.md");
        PrintWriter writer = new PrintWriter(fileWriter)) {
        this.participants.sort(Comparator.comparing(Participant::username));

        writer.print(header(this.participants.size()));

        this.participants.forEach(p -> {
            String markdownForHomework = getMarkdownForParticipant(p.username(), p.homework());
            writer.print(markdownForHomework);
        });
    }
}

그리고 header의 경우 totalNumberOfParticipants는 결국 전역변수 participants의 size이기 때문에 해당 매개변수를 지워주고, participants의 사이즈를 대신 넣어줄 수 있다.

private String header() {
    StringBuilder header = new StringBuilder(String.format("| 참여자 (%d) |", participants.size()));

    for (int index = 1; index <= this.totalNumberOfEvents; index++) {
        header.append(String.format(" %d주차 |", index));
    }
    header.append(" 참석율 |\n");

    header.append("| --- ".repeat(Math.max(0, this.totalNumberOfEvents + 2)));
    header.append("|\n");

    return header.toString();
}

최종적인 코드는 아래와 같다.

public class StudyPrinter {
    private int totalNumberOfEvents;
    private List<Participant> participants;

    public StudyPrinter(int totalNumberOfEvents, List<Participant> participants) {
        this.totalNumberOfEvents = totalNumberOfEvents;
        this.participants = participants;
    }

    public void print() throws IOException {
        try (FileWriter fileWriter = new FileWriter("participants.md");
            PrintWriter writer = new PrintWriter(fileWriter)) {
            this.participants.sort(Comparator.comparing(Participant::username));

            writer.print(header());

            this.participants.forEach(p -> {
                String markdownForHomework = getMarkdownForParticipant(p.username(), p.homework());
                writer.print(markdownForHomework);
            });
        }
    }

    private String getMarkdownForParticipant(String username, Map<Integer, Boolean> homework) {
        return String.format("| %s %s | %.2f%% |\n", username, checkMark(homework), getRate(homework));
    }

    /**
     * |:white_check_mark:|:white_check_mark:|:white_check_mark:|:x:|
     */
    private String checkMark(Map<Integer, Boolean> homework) {
        StringBuilder line = new StringBuilder();
        for (int i = 1 ; i <= this.totalNumberOfEvents ; i++) {
            if(homework.containsKey(i) && homework.get(i)) {
                line.append("|:white_check_mark:");
            } else {
                line.append("|:x:");
            }
        }
        return line.toString();
    }

    private double getRate(Map<Integer, Boolean> homework) {
        long count = homework.values().stream()
                .filter(v -> v == true)
                .count();
        return (double) (count * 100 / this.totalNumberOfEvents);
    }

    /**
     * | 참여자 (420) | 1주차 | 2주차 | 3주차 | 참석율 |
     * | --- | --- | --- | --- | --- |
     */
    private String header() {
        StringBuilder header = new StringBuilder(String.format("| 참여자 (%d) |", participants.size()));

        for (int index = 1; index <= this.totalNumberOfEvents; index++) {
            header.append(String.format(" %d주차 |", index));
        }
        header.append(" 참석율 |\n");

        header.append("| --- ".repeat(Math.max(0, this.totalNumberOfEvents + 2)));
        header.append("|\n");

        return header.toString();
    }
}

이 리팩토링을 진행하는 방식을 간단하게 정리하자면 가급적이면 관련있는 데이터들을 한군데로 모으고, 메소드를 옮기다면 메소드들에 전달되는 매개변수들을 많이 줄일 수가 있다.

Related posts