[우아한테크코스] 2주차 과제 피드백 후기

2주차 피드백 후기

 

이번 과제는 사다리 타기를 구현하는 프로그램이다.
몇 가지 중요했던 피드백들을 정리해 보았다.

일급 컬렉션

기존 코드

public class Players {
    private List<Player> players;

    public Players() {
        players = new ArrayList<>();
    }

    public void add(Player player) {
        if (checkDuplicateName(player)) {
            throw new IllegalArgumentException("이름은 중복될 수 없습니다");
        }
        players.add(player);
    }

    public void addAll(List<Player> players) {
        this.players = players;
    }

    private boolean checkDuplicateName(Player player) {
        return players.contains(player);
    }

    // 중략
}

일급 컬렉션을 제대로 만들지 못했다. 일급 컬렉션은 컬렉션의 불변을 보장해야 한다. 하지만 저렇게 add(Player player) 함수를 노출시키게 되면 컬렉션은 계속 변경될 것이다. 저렇게 만든 것은 일급 컬렉션이 아니다.

피드백 이후

public class Players {
    private final List<Player> players;

    public Players(List<Player> players) {
        checkDuplicateName(players);
        this.players = players;
    }

    private void checkDuplicateName(List<Player> players) {
        Set<Player> nameSet = new HashSet<>(players);
        if (nameSet.size() != players.size()) {
            throw new IllegalArgumentException("이름은 중복될 수 없습니다");
        }
    }

    // 중략
}

필드는 final로 선언 한 후 생성자에서 값을 검증하고 초기화 하는 것으로 변경했다. 이렇게 함으로써 한 번 객체가 생성되면 players는 변경을 할 수 없게되어 안전하게 된다.

객체의 역할

기존 코드

Ladder ladder = new Ladder(new NaturalNumber(height), new NaturalNumber(countOfPerson));
ladder.initLadder(booleanGenerator);

사다리를 만들고 초기화를 시키는 것을 따로 두었다. 그리고 Ladder의 필드는 아래와 같았다.

private List<Line> lines;
private NaturalNumber height;
private NaturalNumber countOfPerson;

피드백 이후

public class Ladder {
    private final List<Line> lines;

    public Ladder(NaturalNumber height, NaturalNumber countOfPerson, BooleanGenerator booleanGenerator) {
        lines = new ArrayList<>();
        for (int h = 0; h < height.getNumber(); h++) {
            lines.add(new Line(countOfPerson));
        }
        initLadder(booleanGenerator, height, countOfPerson);
    }

    private void initLadder(BooleanGenerator booleanGenerator, NaturalNumber height, NaturalNumber countOfPerson) {
        for (int h = 1; h <= height.getNumber(); h++) {
            loopInPerson(booleanGenerator, h, countOfPerson);
        }
    }
    // 중략
}

래더의 필드는 한 개로 줄었고 Ladder를 new 할시 바로 래더가 반환되게 수정했다. 불필요한 코드를 없애고 클래스 변수의 갯수를 줄일 수 있었다.

Dto의 역할

예외처리를 도메인에서 진행했다. 핵심 적인 부분이기때문에 도메인에서 하는 것은 필수적이다. 그럼에도 불구하고 나는 Dto 에서 까지 검증을 했다. 이것이 과연 올바른 방법인가?

사실 올바른 방법이란게 없다. 어느 것이 더 효율적인지 고민해 보았다. 리뷰어이신 써머님 께서는

중복 로직이 발생할 수 있다는 문제를 짚어주셨다. 사실 나도 Dto를 만들면서 굳이 도메인에서 하는데 여기서 까지 할 필요가 있을까? 라는 생각이 들긴했다. 실제로도 도메인만으로도 통신해도 문제가 없긴 하니까 말이다. 그래도 한 번 시도해 보자라는 마음으로 Dto에서도 예외처리를 넣어놓았다. 그렇지만 요구사항 변경으로 인한 코드 변경이 생길 때 수정해야 할 양이 2배가 되는 건 사실이다. 요구사항 변경시 코드의 변경을 최소화하는게 클린코드 핵심인데 이것은 그 논리를 벗어나는 것 같다. 많이 고민해 봐야할 문제인듯 하다.

테스트 당 assert 하나

한 테스트에서 여러 개념을 테스트하는 것을 지양하는게 좋다.

기존 코드

@Test
void 사다리_타기() {
    // given
    ladder.putBridge(new NaturalNumber(1), new NaturalNumber(1));
    ladder.putBridge(new NaturalNumber(1), new NaturalNumber(3));
    ladder.putBridge(new NaturalNumber(2), new NaturalNumber(2));
    ladder.putBridge(new NaturalNumber(3), new NaturalNumber(3));

    // when
    // 1번 사람은 4번 인덱스로 반환 되어야 함
    int firstIdx = ladder.takeLadder(new NaturalNumber(1));

    // 2번 사람은 1번 인덱스로 반환 되어야 함
    int secondIdx = ladder.takeLadder(new NaturalNumber(2));

    // 3번 사람은 3번 인덱스로 반환 되어야 함
    int thirdIdx = ladder.takeLadder(new NaturalNumber(3));

    // 4번 사람은 2번 인덱스로 반환 되어야 함
    int fourthIdx = ladder.takeLadder(new NaturalNumber(4));


    // then
    assertThat(firstIdx).isEqualTo(4);
    assertThat(secondIdx).isEqualTo(1);
    assertThat(thirdIdx).isEqualTo(3);
    assertThat(fourthIdx).isEqualTo(2);
}

피드백 이후

사다리 생성을 @Before로 빼놓고 각 사람마다 검증을 하는 식으로 했다.

 @Test
void 사다리타기_1번사람() {
    // 1번 사람은 4번 인덱스로 반환 되어야 함
    int firstIdx = ladder.takeLadder(new NaturalNumber(1));

    assertThat(firstIdx).isEqualTo(4);
}

@Test
void 사다리타기_2번사람() {
    // 2번 사람은 1번 인덱스로 반환 되어야 함
    int secondIdx = ladder.takeLadder(new NaturalNumber(2));

    assertThat(secondIdx).isEqualTo(1);
}

@Test
void 사다리타기_3번사람() {
    // 3번 사람은 3번 인덱스로 반환 되어야 함
    int thirdIdx = ladder.takeLadder(new NaturalNumber(3));

    assertThat(thirdIdx).isEqualTo(3);
}

@Test
void 사다리타기_4번사람() {
    // 4번 사람은 2번 인덱스로 반환 되어야 함
    int fourthIdx = ladder.takeLadder(new NaturalNumber(4));

    assertThat(fourthIdx).isEqualTo(2);
}

느낀 점

써머님께서 리뷰를 너무 꼼꼼하고 빠르게 해주셔서 감사했다. 덕분에 고민할 시간도 충분했고 나의 문제점들을 알 수 있었다. 주말 새벽에 리뷰를 해주실정도로 열심히 잘 해주셔서 더욱 자극 받았던 이번 주였다.