Skip to content

Conversation

@sinb57
Copy link

@sinb57 sinb57 commented Feb 11, 2022

안녕하세요! 이프입니다~
프리코스 때부터 코드리뷰를 받고 싶어서 엄청 기대하고 있었는데, 막상 리뷰받으려니 많이 떨리네요ㅎ

나름대로 많은 생각을 가지고서 코드를 작성했지만, 어떻게 보이실지 모르겠습니다.ㅜ
가장 궁금한 질문을 먼저 드리고, 그 외 부분은 지적해주시는대로 답변드리겠습니다!

  1. 구조 설계
    입출력 로직과 비즈니스 로직을 명확히 분리하기 위해
    Controller와 Service, View로 분리하여 구조를 설계했는데
    이 방식이 괜찮은지, 아니라면 어떠한 문제점이 있는지 궁금합니다.

  2. 자동차 전진 기능
    자동차 전진 기능은 랜덤값에 따라 이뤄지는 로직입니다.
    어떻게 하면 해당 기능을 테스트할 수 있을지 프리코스 때부터 고민했었고
    당시 사용한 방법을 이번 과제에서도 사용을 했습니다.
    NumberPicker 인터페이스를 생성해서 미리 지정한 숫자를 반환할 수 있는 CustomNumberPicker 구현체를 만듦으로써
    전진 조건에 사용되는 랜덤값을 순차적으로 지정할 수 있도록 했습니다.
    나름 괜찮은 방식이라고 생각하지만 보다 확실한 답을 듣고 싶어서 질문드립니다.
    이 방식에 대한 럿고님의 생각이 궁금하며, 또 현업에서는 랜덤값에 대한 테스트를 어떻게 진행하는지도 궁금합니다.

sinb57 and others added 30 commits February 9, 2022 17:49
* refactor: 기존의 @test을 @ParameterizedTest로 변경

* refactor: 테스트 메소드명 수정
* refactor: 명확한 의미 전달을 위해 Calculator 메소드명 변경

* feat: Separator 클래스 생성
  - 쉼표(,)와 콜론(:)을 이용한 split 로직 구현
  - 커스텀 구분자를 이용한 split 로직 구현
* test: 자동차 이름 입력 기능 테스트 수정

* feat: Delimiter enum 클래스 생성
* feat: Delimiter 클래스에 콜론(:), 바(-)에 대한 concat, repeat 메소드 구현
* feat: Delimiter 클래스에 쉼표(,)에 대한 join 메소드 구현
* test: 이동횟수 입력 테스트 구현

* refactor: Round 클래스에서 이뤄지던 숫자 검증 로직을 InputView 클래스로 이동

* refactor: InputView 클래스에서 이동횟수 입력 메소드의 반환 값을 int형으로 변경
* style: picker 패키지를 service 패키지로 이동

* refactor: Controller 클래스 내 출력 로직, 메소드 분리

* feat: GameService 클래스 생성
* fix: RoundRange.MINIMUM 값 변경 (0->1)
  - 양수 검증 로직에 대한 테스트 실패 해결

* feat: RoundValidator 클래스 추가
@sinb57
Copy link
Author

sinb57 commented Feb 15, 2022

다시 한번 더 PR 리뷰 요청드립니다!! 코멘트 달아주신 여러 피드백이 정말 많이 도움되고 있습니다!! 정말 감사합니다!

Copy link

@ksy90101 ksy90101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 이프 :) 리뷰어 럿고 입니다.
리뷰를 남긴것들을 모두 잘 적용해주시고 깊은 고민후에 답변을 남겨주셔서 저도 많이 배웠습니다 💯
1단계 미션은 여기까지 하도록 하겠습니다. 너무 고생 많으셨어요.

테스트 코드 추가 피드백 및 간단한 리뷰를 남겼는데, 해당 부분은 충분히 2단계에서 또 수정하시면서 적용이 될거라고 생각이 들어서, 머지하도록 하겠습니다.

2단계때 다시 뵙겠습니다!

this.outputView = outputView;
}

public void initGame() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도 한번 테스트 코드를 작성해볼까요?
실제 전체적으로 잘 되는지 어떻게 확인할 수 있을까요?
이프는 충분히 할 수 있을거 같아서 어려운 테스트 이지만, 한번 시도 해보시면 좋을거 같아요!

Comment on lines +14 to +20
public boolean isNotFinished() {
return this.count != 0;
}

public void decreaseCount() {
this.count--;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 개인적으로 이러한 간단한 메서드도 테스트 코드를 작성해둬야 한다고 생각이 듭니다.
혹시라도 다른 개발자가 정책과 다르게 변경하게 되었을때 유연하지 못할수도 있어서요.

import racingcar.domain.round.Round;
import racingcar.service.picker.NumberPicker;

public class GameService {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이쪽도 충분히 테스트 코드를 작성할 수 있을거 같아요.
한번 작성해보실까요?

Comment on lines +14 to +17
@Override
public int pickNumber() {
return RandomRange.pick(random);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도 하나의 정책이기 때문에 테스트 코드를 작성해두면 좋을거 같아요.
Junit에서는 isBetween()이러한 메서드를 제공해주고 있습니다.

Comment on lines +5 to +8
public static final String MessageOfRequestCarNames = "경주할 자동차 이름을 입력하세요(이름은 쉼표(,)를 기준으로 구분).";
public static final String MessageOfRequestRound = "시도할 회수는 몇회인가요?";
public static final String MessageOfStatusTitle = "실행 결과";
public static final String MessageFormatOfPrintWinner = "%s가 최종 우승했습니다.";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

상수의 명명 컨벤션을 무엇일까요!?

@@ -0,0 +1,10 @@
package racingcar.view.output.message;

public class OutputMessage {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도 생성이 되면 안될거 같은데요.
생성자를 private으로 설정해두면 어떨까요?
아니면 ExceptionMessage때 처럼 enum으로 해주셔도 되구요 ㅎㅎ

private static final Pattern CUSTOM_SEPARATOR_PATTERN = Pattern.compile("//(.*)\n(.*)");
private static final String STANDARD_UNITS = ",:";
private static final String SEPARATOR_FORMAT = "[%s]";
private static final String emptyString = "";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위의 상수와 컨벤션이 다르네요 ㅎㅎ

@ksy90101 ksy90101 merged commit 78d0e34 into woowacourse:sinb57 Feb 15, 2022
@verus-j
Copy link

verus-j commented Feb 27, 2025

@sinb57
왜 레이어드 아키텍처를 사용하셨나요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants