Step4 자동차 경주(우승자)

[jikimee64]

Step4 자동차 경주(우승자) 완료했습니다.

피드백 부탁 드립니다. 감사합니다!! 😀

[shared-moon]

우철님 안녕하세요!
4단계 미션 잘 진행 해 주셨네요 :)
코멘트를 좀 남겨 두었는데, 확인 후 다음 미션 진행 해주세요!
고생 많으셨습니다 :)

[shared-moon]

이거 메인 기능은 결국 String.split밖에 없어보이는데 클래스로 뺐을 때 어떤 이득이 있나요 ?

[jikimee64]

@shared-moon

• 자동차 이름은 쉼표(,)를 기준으로 구분한다

해당 요구사항을 테스트 하고 싶어 별도 클래스의 public 메소드로 분리했습니다!
조금 과한 클래스 분리 일까요? 조언 부탁드립니다..!

[shared-moon]

말씀하신 테스트는 Cars에서 문자열을 입력받고 여러개의 Car가 원한대로 만들어지는지 정도로 봐도 되지 않을까요 ?
엄밀히 따지면 Split 이라는 클래스는 지금 파라미터로 들어오는 carName이 실제 carName으로 쓰일 녀석이 들어온건지, 리턴되는 값이 carNames의 목적에 맞게 쓰일건지는 관심사가 아니라고 생각해요

단순히 들어온 문자열을 , 를 기준으로 분리하고 리턴할 뿐이라서 되게 범용적인 책임을 가지고 있는걸로 보이거든요

물론 이런식으로 나누셔도 상관은 없지만, 딱히 메리트가 없어보여서 말씀 드렸습니다 ~

약간 주관적이긴 할텐데, 저는 개인적으로 과한 분리라고 생각합니다 ~

[shared-moon]

Factory가 주 역할이라 CarFactory가 더 맞지 않을까요 ?

[shared-moon]

Car에 toString을 오버라이드 하면 꽤 간결해지지 않을까요 ?

[shared-moon]

변수명이 winnerNames가 더 맞는 것 같아요..!
추가로, OutputView에서는 넘어온 Name들이 Winner인지 아닌지 판별이 불가능한데, 좀 더 범용적으로 표현하는게 어떨까요 ? printNames 정도 ?

[shared-moon]

어차피 Name은 Car에서만 쓰이는 것 같은데, 문자열을 받고 Car 생성자에서 Name으로 만들어주는 건 어떨까요 ?

[shared-moon]

for문으로 만드는게 낫지 않을까요 ..?

[shared-moon]

개인적으로는 약간 가독성이 떨어지는 느낌인데, list의 이름과 position을 검증하고 싶으신거라면 각각 따로 검증하는건 어떨까요 ?

[jikimee64]

@shared-moon
말씀하신 사항이 아래와 같은 테스트 로직일까요?

    @Test
    @DisplayName("성공 - 레이싱이 끝난 후 우승자의 이름을 구한다.")
    void success_racing_winner_names() {
        // given
        Cars cars = new Cars(
                List.of(
                        new Car("a", new TestMove(0)),
                        new Car("b", new TestMove(5)),
                        new Car("c", new TestMove(9))
                )
        );

        fiveStepRacing(cars);

        // when
        List<Car> winners = cars.findWinners();

        // then
        assertThat(winners).hasSize(2)
                .extracting("carName")
                .containsOnly(new Name("b"), new Name("c"));
    }

    @Test
    @DisplayName("성공 - 레이싱이 끝난 후 우승자의 위치를 구한다.")
    void success_racing_winners() {
        // given
        Cars cars = new Cars(
                List.of(
                        new Car("a", new TestMove(0)),
                        new Car("b", new TestMove(5)),
                        new Car("c", new TestMove(9))
                )
        );

        fiveStepRacing(cars);

        // when
        List<Car> winners = cars.findWinners();

        // then
        assertThat(winners).hasSize(2)
                .extracting("position")
                .containsOnly(5, 5);
    }

혹은 extracting(), containsExactly() 의 메소드 체이닝이 가독성이 떨어진다는 말씀이실까요?

혹..은.. 아래와 같이 assertAll을 사용해서 리스트에서 get으로 하나씩 꺼내서 검증하라는 말씀이실까요?
개인적으로는 언급해주신 부분에서 가독성이 떨어진다는 걸 크게 못느껴서요 ㅠㅠ
조언 부탁드립니다..!

      // when
        List<Car> winners = cars.findWinners();

        // then
        assertAll(
                () -> assertThat(winners.get(0)).isEqualTo("b"),
                () -> assertThat(winners.get(1)).isEqualTo("c")
        );

[shared-moon]

오 아뇨 제가 말씀드린 의도대로 한다면 사실

List<Name> winnerNames = winners.stream.map(Car::getName).collect(toList())
List<Position> winnerPositions = winners.stream.map(Car::getPosition).collect(toList())

assertThat(winnerNames).equals("b","c");
assertThat(winnerPositions).equals(5,5);

이런식으로 됐을거에요! 개인차가 있을 수 있어서 굳이 안바꾸셔도 되긴 합니다~
저는 tuple() 로 한번 감싸야되는거랑 뎁스가 한번 더 들어가게 되는게 보기 불편해서 말씀드렸어요 ~
Kotest나 Spock이었다면 좀 더 적극적으로 권해봤을텐데 Junit으로는 자바로 뽑아야 되니까 호불호가 갈릴거같아서 강하게 어필을 못하겠네요 ㅋㅋ