-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[자동차 경주] 김민창 4단계 미션 제출합니다. #62
base: idle2534
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고 하셨습니다.
테스트에 대해서 고민을 많이 한게 느껴지고, 전체적으로 깔끔하게 짜주셨네요.
몇몇 코멘트 남겼는데, 제가 남긴 코멘트가 절대적인 답은 아닙니다. 코드에 정답은 없으니까요.
코멘트에 대한 토론, 반박은 언제나 환영입니다!
질문에 대한 답은 제가 일요일 전에 달거나,
아니면 일요일에 다 답변해드리도록 하겠습니다.
수고하셨습니다!!
src/test/java/RacingCarTest.java
Outdated
int expected = racingCar.getScore(); | ||
int actual = racingCar.move(); | ||
|
||
assertThat(actual).isIn(expected, expected + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 isIn 메서드로도 테스트를 어느정도 할 수 있긴 하지만, 명확하게 move에 대한 테스트는 하기 어려울 것 같아요.
어떻게 잘 테스트해볼 수 있을까요??
src/test/java/RacingCarTest.java
Outdated
int expected = 0; | ||
int actual = racingCar.move(3); | ||
|
||
assertThat(actual).isEqualTo(expected); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertThat을 하나의 테스트 메서드 안에 쓰는 경우 어떤 문제가 있을까요?
이런 경우 parameterizedTest를 활용해볼 수도 있을 것 같네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameterizedTest를 활용해야하는 것은 이해했는데 assertThat을 하나의 테스트 메서드 안에 쓰는 경우가 정확히 무엇을 의미하는지 모르겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분에 대해서 설명이 좀 부족할 수 있네요.
만약
assertThat(case1)
assertThat(case2)
assertThat(case3)
이렇게 되어 있을 때 case1이 실패되면, case 2, case3에 대한 검증을 못합니다.
assertThat(case1)에서 테스트가 멈추거든요.
assertAll 같은 함수도 학습해보면 좋을 것 같습니다.
src/main/java/RacingCar.java
Outdated
} | ||
|
||
public String getResultString(int phase) { | ||
StringBuilder resultString = new StringBuilder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 method 파라미터에 phase를 넣은 이유가 있을까요?
src/test/java/RacingCarTest.java
Outdated
RacingCarGame racingCarGame = new RacingCarGame(); | ||
System.setIn(new ByteArrayInputStream("test1\n1".getBytes())); | ||
|
||
assertThatThrownBy(racingCarGame::setGame).isInstanceOf(InputMismatchException.class).hasMessage("최소한 두 명 이상의 플레이어가 필요합니다."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
얘는 어째서 ::와 같은 방식으로 사용할 수 있을까요?
src/test/java/RacingCarTest.java
Outdated
|
||
|
||
String actual = racingCar.getResultString(5); | ||
String expected = new String(new char[racingCar.getScore()]).replace("\0", "-"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 경우 여러줄로 분리하는 것도 좋아보여요 너무, 한줄에 많은 의미가 담겨있고 괄호가 많아 가독성이 떨어지게 보일 수도 있을 것 같아요
src/main/java/RacingCarGame.java
Outdated
printResult(); | ||
} | ||
|
||
public void resetGame() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resetGame을 만드신 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기능 요구사항에 없었고 실제로 Application에 별 의미없었으며 private으로 할 수도 있었고 애초에 함수로 따로 뺄 필요도 없었지만 그냥 게임을 다시 시작할 수 있도록 초기화해주는 외부 함수가 있으면 좋을 것 같아서 만들어 둔 것입니다.
지금 Main에서 처음부터 초기화 한다고 가정했을 때 setGame() -> startGame() -> resetGame() 이렇게 진행되는데 이를 private으로 바꾸고 public void game()으로 묶어서 외부에서 하나의 함수만 호출되게하는게 좋을까요? 아니면 3개를 쪼개서 필요에 따라 호출할 수 있기 하는 편이 좋을까요? 어떤 프로그램이냐에 따라 다를 것 같긴 한데 이번 프로그램에서는 어떻게 하실 것 같으신지 의견 듣고 싶습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 필요하지 않는 메서드는 굳이 만들지 않는 주의라서 resetGame이 필요없다고 생각해요.
지금 시작하고 터미널에서 입력이 끝나면 어플리케이션 자체가 종료되기 때문에 어떤 이유로 만들었는지 궁금했었습니다.
src/main/java/RacingCarGame.java
Outdated
List<RacingCar> racingCars = new ArrayList<>(); | ||
boolean isGameInit = false; | ||
|
||
public void setGame() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 메서드는 15줄을 넘기면 안된다는 요구사항을 만족하지 못한 것 같아요 어떻게 분리할 수 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
솔직히 제가 카톡에 장문 쓰기 위해서 요구사항을 다시 한번 정독하기 전까지는 15줄을 넘기면 안된다는 것을 못봤어서 그 전에 짠 코드인데 수정을 깜빡한 것 같네요. 저는 일단 자동차를 입력 받는 부분과 시도할 횟수를 입력받는 부분을 각각 함수로 따로 빼서 분리할 것 같습니다. 예외처리도 따로 분리 하고 싶은데 자동차의 경우 가능하지만 횟수(phase)는 try catch로 감싸서 처리했는데 try catch 안에 분리한 입력 함수를 넣는 방식은 조금 별로라고 생각해서 어떻게 분리해야할지 잘 모르겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요거는 피드백을 남겨드리고 싶었는데, 지금 바뀌어서 피드백을 드리기가 힘들겠군요.
다만 궁금한게 있는데
try catch 안에 분리한 입력 함수를 넣는 방식은 조금 별로라고 생각해서 어떻게 분리해야할지 잘 모르겠습니다.
라고 생각하신 이유가 있을까요?
import org.junit.jupiter.api.Nested; | ||
import org.junit.jupiter.api.Test; | ||
|
||
public class RacingCarTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
클래스 이름은 RacingCarTest인데, RacingCarGame에 대한 부분도 포함된 것 같아요.
사이즈가 커지는 경우 RacingCarGame에 대한 테스트를 확인하기 어려울 수도 있을 것 같아요.
어떤 기준으로 테스트 클래스를 나눠볼 수 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예외처리를 하고 싶은 부분이 어느 클래스에 있는지를 기준으로 테스트 클래스를 나뉠 것 같습니다. 이번의 경우 Nested로 처리한 SetRacingCarGameTest를 RacingCarGameTest 클래스를 새로 만들어 거기로 옮긴 후 Nested class 이름을 SetGameTest 수정할 듯 합니다.
src/main/java/RacingCar.java
Outdated
return move(randPower()); | ||
} | ||
|
||
public int move(int power) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 메서드는 test말고 외부 클래스에서 호출된 적이 없는데, public이네요.
이런 경우 접근제어자를 변경하는 거에 대해서 어덯게 생각하시나요?
### 기능 요구사항 | ||
- 주어진 횟수 동안 n대의 자동차는 전진 또는 멈출 수 있다. | ||
- 각 자동차에 이름을 부여할 수 있다. 전진하는 자동차를 출력할 때 자동차 이름을 같이 출력한다. | ||
- 자동차 이름은 쉼표(,)를 기준으로 구분하며 이름은 5자 이하만 가능하다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이름은 5자 이하만 가능하다하였는데, 이 부분은 구현이 안 된 것 같아요!
int round = scanner.nextInt(); | ||
|
||
if (round <= 0) | ||
throw new NumberFormatException("횟수는 1회 이상이여야 합니다."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
round가 0보다 작은 경우는 NumberFormatException에 속할까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
View는 입력,출력과 같은 부분을 처리하는 부분이에요. 이런 곳에 round가 0보다 작은 경우에 대한 예외처리를 하면 어떤 문제가 발생할 수 있을까요?
|
||
List<String> names = List.of(input.split(",")); | ||
|
||
if (names.size() <= 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 view에서 round처럼 플레이어 수와 관련된 예외처리를 하면 어떤 문제가 있을 수 있을까요?
System.out.println(); | ||
} | ||
|
||
public void printWinner(List<RacingCarResultDto> racingCarResultDtoList) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
변수의 이름에 List같은 자료형과 직접적으로 연관되는 건 피하는게 좋다고 해요. 왜 그럴까요?
(요 부분은 그렇게 중요한 건 아닌데 한번 생각해보면 좋습니다.)
|
||
@Builder | ||
@Getter | ||
@NoArgsConstructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NoArgsCounstructor랑 AllArgsConstructor를 둘 다 붙인 기준이 있으신가요?
@RequiredArgsConstructor | ||
public class RacingCarController { | ||
|
||
final RacingCarView racingCarView; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
객체지향적으로 밖으로 들어날 필요가 없는 변수는 숨기는게 좋습니다.(은닉성)
private으로 숨겨보면 어떨까요?
throw new IllegalStateException("게임이 초기화되지 않았습니다."); | ||
|
||
racingCarView.printResultTitle(); | ||
for(int currentRound = 0; currentRound < round; ++currentRound){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
controller에서 너무 많은 로직이 있는 것 같아요.
"round 만큼 움직이는 자동차는 움직임을 시도한다" 라는 책임을 안으로 넣어보면 어떨까요?
return RacingCarCreateDto.builder().name(name).build(); | ||
} | ||
|
||
public static RacingCarResultDto toRacingCarResultDto(RacingCar racingCar) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converter에서 너무 출력에 대한 책임을 지고 있는 것 같아요.
만약 자동차가 움직일 떄 출력되어야 하는 문자열이 "-" 가 아니라 "*" 로 바뀌었을 떈 Converter가 바뀌는게 맞을까요?
name(racingCar.getName()) | ||
.resultString(racingCar.getResults().stream().map(RacingCarConverter::resultToString).collect( | ||
Collectors.joining())) | ||
.distance((int) racingCar.getResults().stream().filter(v -> v).count()).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서 filter를 이렇게 넣으신 이유가 있을까요?
return RacingCarCreateDto.builder().name(name).build(); | ||
} | ||
|
||
public static RacingCarResultDto toRacingCarResultDto(RacingCar racingCar) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 너무 괄호안에 로직이 많은 것 같아요.
다른 사람들도 잘 읽을 수 있게 분리하는 건 어떨까요?
일단 코드를 전체적으로 봤을 때 스프링의 향기가 진하게 나네요. 그 뒷부분에 대한 질문은 제가 이해를 못해서 혹시 나중에 디테일하게 설명해주실 수 있을까요?? |
민창님에게 몇몇 질문을 던지고 싶어요.
이번 미션에서 Layer 아키텍쳐에 대해서 다루진 않을 거에요. 아마 mvc와 Layer 아키텍쳐가 혼재되어있어서 민창님이 어려워하는 것 같은데, 여기서는 mvc 관점에서만 보시는 걸 추천드립니다.
일단 간단하게 lombok을 안쓰는 방법이 있습니다. |
2024-10-3
출력이 어떤 식으로 되는지 확인하는 김에 3단계도 했습니다. 3단계는 메인 함수만 추가하는거라 별 문제는 없다고 생각합니다.
2024-10-11
mvc를 이론만 알지 직접 활용해보는 것은 처음이라 개념 공부와 설계 과정에서 시간이 좀 걸렸습니다. 일단 mvc로 racingCar 다시 만들었고 일어나서 Exception이랑 Test 마무리할 예정입니다. 또한, mvc로 다시 만들면서 몇 가지 궁금증이 생겼습니다.