Skip to content

Conversation

@SangBeom-Hahn
Copy link
Collaborator

No description provided.

초기 엔터티를 구축, 테스트한다.
*InputView: 기능 구현
*feat: BridgeService 이동 검사 기능 및 테스트 구현

*feat: MoveResult 검사 결과 저장 기능 구현

*feat: UserSquare 사용자 입력 기능 구현

*feat: BridgeGame, InputView, OutputView 컨트롤러 관련 기능 구현
*feat: BridgeService 중간 결과 출력 기능 구현

*feat: OutputView 중간 결과 출력 기능 구현

*feat: BridgeGame 컨트롤러 관련 기능 구현

*refactor: CompareResult 클래스 이름을 결과와 관련되도록 변경
*feat: BridgeService 재시작 신호 입력 기능 구현

*feat: RestartStatus 재시작 기능 및 테스트 구현

*feat: InputView 재시작 신호 입력 기능 구현

*feat: BridgeGame 컨트롤러 관련 기능 구현
*feat: BridgeService 종료 기능 구현

*feat: FinalResult 종료 기능 구현

*feat: RestartStatus 종료 기능 및 테스트 구현

*feat: OutputView 최종 결과 출력 기능 구현
사용자가 잘못된 값을 입력할 경우 다시 그 부분을 입력 받도록 예외 처리 구현
*fix: BridgeMaker 입력 범위 검증 메서드 추가
@SangBeom-Hahn SangBeom-Hahn requested a review from sjiwon July 30, 2023 04:17
@sjiwon sjiwon changed the title 다리 미션 [SangBeom-Hahn] 다리 건너기 미션 [SangBeom-Hahn] Jul 30, 2023
Comment on lines 7 to +9
public static void main(String[] args) {
// TODO: 프로그램 구현
new BridgeGame().run();
Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO 제거

Comment on lines +100 to +104
public void retry(final boolean eachResult) {
if (eachResult == false) {
restartStatus = bridgeService.retry();
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public void retry(final boolean eachResult) {
if (eachResult == false) {
restartStatus = bridgeService.retry();
}
}
public void retry(final boolean eachResult) {
if (!eachResult) {
restartStatus = bridgeService.retry();
}
}

Comment on lines +1 to +3
package bridge.exception;

public class HasWhiteSpaceException extends IllegalArgumentException{
Copy link
Collaborator

Choose a reason for hiding this comment

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

exception 패키지에 있는 Exception들은 안쓰는거 같은데 만드신 이유가 있나요

Comment on lines +11 to +12
private static final String QUIT_SIGN = "Q";
private static final String RESTART_SIGN = "R";
Copy link
Collaborator

Choose a reason for hiding this comment

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

그냥 RESTART.signal, QUIT.signal로 쓰면 되지 않나요?
별도의 private static final 상수를 둔 이유는 뭘까요

Comment on lines +19 to +26
public static RestartStatus getRestartStatusBy(final String signal) {
validateCharacter(signal);

return Arrays.stream(values())
.filter(RestartStatus -> RestartStatus.signal.equals(signal))
.findFirst()
.orElseGet(() -> null);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

검증하는게 R, Q 중 하나로 제대로 들어온거 같은데 애초에 이 두 문자가 아닌 다른 문자가 들어오면 Exception을 발생시켜야 하지 않을까요?

밑에서 orElseGet으로 null을 반환하는 이유는 뭐죠? 잘못된 값을 Application 로직에서 null로 다루는건가요?

  • orElseGet -> orElseThrow

Comment on lines +17 to +27
private static void printInputLengthMessage() {
System.out.println(INPUT_BRIDGE_LENGTH_MESSAGE);
}

private static void printInputDirectMessage() {
System.out.println(INPUT_USER_SQUARE_MESSAGE);
}

private static void printInputRestartStatusMessage() {
System.out.println(INPUT_RESTART_MESSAGE);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

재사용할 일이 없어서 그냥 read~~ 메소드에 sout으로 넣으면 될거같은데 분리한 이유가 있을까요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

큰 의미 없이 뺀거긴 합니다. 그러면 이 또한 메서드 분리라고 볼 수 있을까요? 아니면 재사용할 일이 없다면 따로 뺄 필요가 없을까요?

Copy link
Collaborator

@sjiwon sjiwon Aug 8, 2023

Choose a reason for hiding this comment

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

재사용을 안하더라도 분리를 하는 목적은 그 메소드만의 분리 로직이 있다는 건데 위에 메소드는 그냥 단순하게 sout만 찍는거라서 분리의 의미가 없어보이네요

Comment on lines +75 to +89
private static boolean isDownDirectNotSame(final List<String> bridge, final List<Boolean> moveResult, final int i) {
return bridge.get(i).equals(DOWN_SQUARE) && moveResult.get(i) == false;
}

private static boolean isUpDirectNotSame(final List<String> bridge, final List<Boolean> moveResult, final int i) {
return bridge.get(i).equals(UP_SQUARE) && moveResult.get(i) == false;
}

private static boolean isDownDirectSame(final List<String> bridge, final List<Boolean> moveResult, final int i) {
return bridge.get(i).equals(DOWN_SQUARE) && moveResult.get(i) == true;
}

private static boolean isUpDirectSame(final List<String> bridge, final List<Boolean> moveResult, final int i) {
return bridge.get(i).equals(UP_SQUARE) && moveResult.get(i) == true;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. !moveResult.get(i)가 아니라 true/false와 비교한 이유는 무엇이죠
  2. final int i는 어떤 의미를 가진 인자죠? 아무런 의미도 없어보이는 네이밍같네요

Comment on lines +20 to +22
// given
String signal = "G";

Copy link
Collaborator

Choose a reason for hiding this comment

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

취향이긴한데 굳이 signal 변수 만들고 메소드에 넣어주지 말고 () -> RestartStatus.getRestartStatusBy("G")로 넣어주는게 어떤가요

  • 밑에도 마찬가지

Comment on lines +29 to +53
@Test
@DisplayName("게임을 재시작한다.")
void restart() {
// given
String signal = "R";

// when
RestartStatus restartStatus = RestartStatus.getRestartStatusBy(signal);

// then
assertThat(RestartStatus.RESTART).isEqualTo(restartStatus);
}

@Test
@DisplayName("게임을 종료한다.")
void quit() {
// given
String signal = "Q";

// when
RestartStatus restartStatus = RestartStatus.getRestartStatusBy(signal);

// then
assertThat(RestartStatus.QUIT).isEqualTo(restartStatus);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

assertThat과 isEqualTo 메소드의 파라미터 네이밍을 보면

  • assertThat -> int actual
  • isEqualTo -> int expected

근데 위에서 보이는 값은 assertThat에 expected를 isEqualTo에 actual을 넣으셨네요

Comment on lines +42 to +43
// then
assertThat(bridge.size()).isEqualTo(size);
Copy link
Collaborator

Choose a reason for hiding this comment

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

bridge의 모든 String이 U or D인지 검증하는 로직도 추가하면 좋을거같네요

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.

2 participants