-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[자동차 경주 게임] 변해빈 미션 제출합니다. #356
base: main
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.
2주차 고생하셨습니다 ㅎㅎ
proceedRounds(cars, roundCount); | ||
publishFinalResult(cars); | ||
|
||
Console.close(); |
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.
세심한 부분이네요.. 저도 잘배우고갑니다
public Integer getScore() { | ||
return score; | ||
} | ||
} |
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.
dto를 이렇게 잘 사용하셨네요... 잘배우고 갑니다..
|
||
import java.util.List; | ||
|
||
public record FinalResponse( |
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.
이야.. record를 사용해서 dto를 이렇게 관리하시는군요... 저는 아직 record를 어떻게 써야할지 감도 안오는데 정말 인상깊었고 한수 배워갑니다
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.
리드미나 프로젝트 구조부터 정리를 정말 잘하시는것 같아요 정말 부럽습니다 🥲
클레스 수가 적지 않은편인데도 어떤 클레스가 어떤 역활을 하게될지 프로젝트 구조만 봐도 알기 쉬웠어요. 많이 배워갑니다 :)
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.
경주를 여러번 진행하고 나서 우승자를 구하는 메서드가 의도하는데로 작동하는지 확인하는 테스트 케이스가 필요하다고 생각하는데 해빈님 생각은 어떠세요 ??
public 으로 선언된 메서드들은 단위 테스트가 필요하다고 생각하는 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.
이번 과제는 테스트 케이스를 단위(계층)별로 정교하게 짜지 못헀어요ㅠ
개인적으로 가장 아쉬운 부분입니다.
다음 과제에서는 MVC 패턴의 각 단위에 해당하는 테스트를 다르게 진행해 보려고 합니다!
이번엔 설계의 이슈로...모킹테스트 외에 부분을 조금 아쉽게 진행했던 것 같아요!
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.
잘 보고 많은거 배우고 갑니다.
저도 더 노력 해보겠습니다.
final Cars cars, | ||
final int roundCount |
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.
final 의 경우 값을 초기화를 단 한번만 할 수 있다라는 조건에 불변이라는 의식이 있지만,
primitive type 과 reference tpye 의 경우가 다를 수 있다라는 것도 알아두면 좋을거 같아요.
primitive type 은 주소가 아닌 값을 메모리 상에 가지고 있기 때문에 불변의 기능이 제대로 활용이 됩니다.
다만, referenct type 은 주소값을 메모리 상에 가지는 것이므로 주소는 불변이지만 주소 값을 이용해 객체의 상태는 바꿀수 있는 상황이 될 수 있으므로 생각한 불변의 기능이 아니게 되지요.
private static final Integer START_SCORE = 0; | ||
|
||
private final Name name; | ||
private Integer score; |
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.
primitive type 과 Wrapper type 의 경우 상황에 따라 바뀔 수 있는 설계 인거 같습니다.
꼭 누가 좋다/나쁘다 를 나누기보다는 사용처에 따라 혹은 값이 주소인지 실제 값인지 이러한 것들을 알고 계시는게 더 정확하게 판단하여 적절한 사용을 할 수 있을거 같습니다.
Int 타입과 Integer 타입을 예시로 기본 타입의 경우 초기화 값이 필수로 처리가 되므로 API 연계 시 해당 값이 null 이 아닌 초기화 된 "0" 값으로 전달될 수 있지요.
다만 "0"이라는 값도 무언가에 대한 정보를 전달하고 실제 데이터를 식별하는데 사용되는 것이라면 무작정 "0" 을 보낼 수 없겟지요.
이러할때 래퍼타입이나 객체타입의 null 을 가지는 점을 이용해 데이터가 없음을 나타낼 수 있을 겁니다.
이거는 각각의 상황에 따라 달라질 수 있는 내용이라 정확한 답이 없지 않을가 싶습니다.
} | ||
} | ||
|
||
private static void validateContainWhiteSpace(final String name) { |
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.
Project Flow Chart 에서는 Name 제약조건에 보이는 빈값 체크가 2개가 있던데 기능 상에서는 한번만 체크 하는 것 같아요
제가 놓친 부분이 있을가요?
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.
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.
전반적인 코드네이밍과 테스트 코드 작성 부분에서 많이 배워갑니다.
이미 많은 리뷰와 저의 짧은 지식으로 구경만 하다 가네요 성장해서 더 좋은 리뷰로 돌아오겠습니다.
public static RacingCarException of(ErrorMessage errorMessage) { | ||
Console.close(); |
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.
저 또한 , 예외 발생 시 자원을 정리하는 부분을 추가를 고민했었는데 예외 클래스를 생성해서 처리 해주는 부분 너무 좋은 것 같습니다. !
@DisplayName("[Success] Cars 일급 컬렉션 객체 생성에 성공합니다.") | ||
void Given_ValidName_When_Create_Then_Success() { | ||
//given | ||
String validCarNames = VALID_CAR_NAMES_INPUT.getName(); | ||
|
||
//when && then | ||
assertDoesNotThrow(() -> Cars.create(validCarNames)); | ||
} |
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.
1주차에 일급 컬렉션 관련해서 제대로 사용하였는지 피드백을 요청하셨던 것으로 기억하는데
테스트 과정에 넣으신 부분이 인상적입니다 :)
피드백 과정을 잘 정리하셔서 다음 주차에 적용하려고 하는 노력 본받아야겠네요!
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.
감사합니다 수란님 :) 덕분에 더욱 성장할 수 있었습니다.
public static Car create(final String name) { | ||
return new Car(name); | ||
} | ||
|
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.
댓글로부터 팩토리 패턴이라는 것도 처음 알았고 생소하지만 많이 배워갑니다! 저도 이런 팩토리 패턴을 적용하신 이유와 final로 매개변수를 지정하신 이유가 궁금합니다
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.
정적 팩토리메소드를 통해 메소드 자체의 동작 행위의 이름을 규정해, 더욱 직관적인 코드 구조 설계가 가능합니다!
final의 경우 아래 답변드렸습니다 :)
List<String> carNames = Parser.parseCarNames(input); | ||
this.cars = createCars(carNames); | ||
} | ||
|
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.
파서 클래스도 따로 클래스로 나눈 것이 인상깊네요 인자에 대부분 final을 쓰셨군요
INVALID_ROUND_COUNT("The round count cannot be smaller than configured by the game setting."), | ||
DUPLICATED("The input cannot contain a duplication."), | ||
SYSTEM_ERROR("The game system crashed."); | ||
|
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.
이번에 enum으로 예외를 작성하면서 어떻게 써야하나 헷갈렸는데 이렇게 작성해야하는군요 형용사나 명사위주로 작성하고 메세지는 영어로 작성해야한다는 점을 알 수 있었습니다
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.
메세지는 한글로 작성하셔도 무방하긴 합니다!
다만, 프로덕트 코드 레벨에서는 영어 사용이 좋다고 생각해서 그렇게 표현하게 되었습니다.
} | ||
|
||
public static List<String> parseCarNames(final String input) { | ||
InputValidator.validateEndsWithComma(input); |
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.
콤마 하나까지 선언해서 사용하실 줄은 몰랐는데 참고하고 갑니다! final을 작성하신 이유도 궁금해요!
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.
정확히는 콤마로 끝나는 인풋을 처리합니다!
김해빈, 박해빈, 변해빈, 과 같이 콤마로 끝나는 예외를 검증합니다!
final로 작성해서 input 자체의 파라미터의 값을 변경하는 일을 막을 수 있습니다.
throw RacingCarException.of(ENDS_WITH_DELIMITER); | ||
} | ||
} | ||
|
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.
추가적인 검증부분을 구현하신 점 인상깊습니다! of로 메서드명을 지정하신 이유가 있으신가요??
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.
익셉션의 경우 of로 팩토리 메소드를 만드는 관례가 있는 걸로 알고 있어요.
정적 팩토리 메소드 컨벤션에 의하면 from 메소드 네이밍이 더 적절할 것 같아요!
void Given_SameScore_When_isSameScore_Then_ReturnTrue() { | ||
//given | ||
Car car = Car.create(VALID_NAME_0.getName()); | ||
|
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.
dto까지 만들어서 테스트코드로 확인할 생각은 미처 못했는데 DTO를 활용하신 부분 배워갑니다!
@Test | ||
@DisplayName("[Success] Cars 일급 컬렉션 객체 생성에 성공합니다.") | ||
void Given_ValidName_When_Create_Then_Success() { | ||
//given | ||
String validCarNames = VALID_CAR_NAMES_INPUT.getName(); | ||
|
||
//when && then | ||
assertDoesNotThrow(() -> Cars.create(validCarNames)); | ||
} | ||
} |
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.
1주차에 일급 컬렉션 관련해서 제대로 사용하였는지 피드백을 요청하셨던 것으로 기억하는데
테스트 과정에 넣으신 부분이 인상적입니다 :)
피드백 과정을 잘 정리하셔서 다음 주차에 적용하려고 하는 노력 본받아야겠네요!
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.
해빈님 안녕하세요.
플로우차트 작성하신걸 보았는데, 아주 깔끔하게 잘 작성하셨더라구요. 저도 간단하게 흐름은 플로우차트로 구현해보려는 편인데 해빈님이 하신 것 처럼 객체들의 연관성까지 같이 표현하면 더 좋겠다는 생각이 들었습니다.
제 코드도 한번 가볍게 둘러봐 주심 감사할 것 같아요! 이번주도 고생 많으셨습니다~
private static final Integer START_SCORE = 0; | ||
|
||
private final Name name; | ||
private Integer score; |
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.
int 대신 Integer 클래스를 사용할 수도 있다는 사실을 배우고 갑니다.
Integer에서 제공하는 메서드들을 가져다가 사용할 수도 있다는 것도 장점이 되지 않을까? 싶은 생각이 들어요
proceedRounds(cars, roundCount); | ||
publishFinalResult(cars); | ||
|
||
Console.close(); |
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.
혹시 close 하는 해당 부분을 사용자입력을 리턴한 다음에 바로 닫아주도록 배치하는 것은 어떻게 생각하시는지 궁금합니다.
닫는 부분도 view에서 처리해주면 어떨까 하는 고민이 들어서요! 🤔
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.
@seondays 프리코스 API를 참고해보면 입력을 받는 Scanner가 싱글톤 패턴으로 구현되어 있어요. View에서 서로 다른 입력을 받기 위해서 Scanner를 형성하고 다시 닫고, 열고 닫고, 와 같은 과정으로 진행하는 경우 확장성을 고려한다면 static으로 선언한 메리트가 떨어진다고 생각합니다.
개인적으로는 전체적인 로직이 끝난 이후에 .close() 하는게 더 좋은 방법이라고 생각해요!
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.
저도 매 번 닫기를 했었는데, 저만 그런진 잘 모르겠는데 테스트 코드가 동작이 잘 안되더라고요..!
그래서 저도 모든 로직이 끝났을 때 했어요!
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.
저도 Name 자체를 객체로 바라볼 생각은 전혀 못했었는데, 생각해보니 차를 생성하기 위한 검증에 관한 모든 것이 결국 Name에 관한 것이었군요... 깨달음 얻고 갑니다.
} | ||
} | ||
|
||
private static void validateExceedLength(final String name) { |
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.
이름 검증 메서드들은 따로 Validator에 분리해서 사용하신 것이 아니고, 객체 내부에 작성 하셨는데 혹시 어떤 의도이셨는지 궁금해요
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.
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.
Validator 클래스의 InputValidator의 경우, 파싱이나 형변환을 위해 기본적으로 검증해야 할 최소한의 검증 조건만을 담고 있습니다.
이 외 검증 조건(비즈니스 요구 사항)은 도메인 계층의 생성자에서 생성과 동시에 검증을 진행합니다 :)
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.
다음 과제에서는 DTO를 저도 활용해서 구현해볼 수 있도록 해야겠습니다.
코드 보고 많이 배워가요.!
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.
한 주 고생 많으셨습니다!
Readme부분이야 뭐 말할 것도 없이 너무 잘하셨고 제가 가장 인상깊게 본 부분은 dto를 만드신 부분인데
- FinalResponse - 최종 우승자를 출력하는 부분
- RoundResponse - 라운드가 진행될 때 마다 선수 이름과 "-" 형태로 변환 하는 부분
- RoundResponses - RoundResponse를 List형태로 변환
이렇게 dto로 잘게 쪼개서 컨트롤러에서 사용하니 코드가 간결해진다는 느낌을 받았습니다.
혹시 dto로 데이터를 보낼 때 기준을 어떻게 잡으시는 편 인가요? 아직 dto에 대해서 와닿지 않아서 더 공부하고 싶은 부분입니다 ㅎㅎ
테스트 코드 작성 부분도 여러가지 edge case들을 커버한 것을 보고 정성이 느껴졌습니다!
3주차도 파이팅입니다~!
|
||
private static void validateEmpty(final String name) { | ||
if (name.isEmpty()) { | ||
throw RacingCarException.of(EMPTY); |
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.
throw IllegalArgumentException이 아니라 RacingCarException클래스를 활용한 방식 배워갑니다!
private static final String DELIMITER = ","; | ||
|
||
private Parser() { | ||
} |
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.
인스턴스화 방지 👍👍
public static String requestRoundCount() { | ||
View.printConstantMessage(ASK_GAME_COUNT); | ||
String request = Console.readLine(); | ||
printNewLine(); |
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.
저는 direct하게 System.out.println()을 사용했는데 메서드화 해서 작성하니까 한결 더 가독성이 좋네요!
final int roundCount | ||
) { | ||
View.printConstantMessage(ROUND_RESULT_MESSAGE); | ||
range(0, roundCount) |
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.
저도 똑같은 의문점을 갖고 있었는데 많은 분들이 여쭤보셨네요! 저도 IntStream이랑 뭐가 다른건지 궁금했는데 static import하셨군요!
Cars cars = Cars.create(carsNameRequest); | ||
|
||
String roundCountRequest = View.requestRoundCount(); | ||
int roundCount = Parser.parseRoundCount(roundCountRequest); |
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.
Integer.parseInt로 바로 변환하기보다 Parser를 사용해서 int 타입으로 변환과 validation 처리 👍
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.
한 주 고생 많으셨습니다!
Readme부분이야 뭐 말할 것도 없이 너무 잘하셨고 제가 가장 인상깊게 본 부분은 dto를 만드신 부분인데
- FinalResponse - 최종 우승자를 출력하는 부분
- RoundResponse - 라운드가 진행될 때 마다 선수 이름과 "-" 형태로 변환 하는 부분
- RoundResponses - RoundResponse를 List형태로 변환
이렇게 dto로 잘게 쪼개서 컨트롤러에서 사용하니 코드가 간결해진다는 느낌을 받았습니다.
혹시 dto로 데이터를 보낼 때 기준을 어떻게 잡으시는 편 인가요? 아직 dto에 대해서 와닿지 않아서 더 공부하고 싶은 부분입니다 ㅎㅎ
테스트 코드 작성 부분도 여러가지 edge case들을 커버한 것을 보고 정성이 느껴졌습니다!
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.
코드를 읽으면서 객체의 활용부터 분리, 컬렉션의 사용까지.. 정말 많은 부분에서 배우고 갑니다!
2주 차 너무 고생하셨고, 3주 차에도 서로 같이 파이팅해요!
import static racingcar.view.constants.ConstantMessage.ASK_MULTIPLE_CAR_NAMES; | ||
|
||
public class View { | ||
public static String requestCarsName() { |
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.
저 같은 경우는 자동차 이름을 입력받는 메서드를 패키지 내의 별개의 클래스로 분리하였는데, 혹시 해당 부분을 따로 분리하지 않으신 이유가 있을까요?
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.
조금 더 단일책임을 준수하는 방법으로 리팩토링 하셨던 것 같아요!
해당 부분은 컨트롤러 계층에서 개행으로 충분히 분리가 가능한 부분이라고 판단했습니다 :)
class movableTest { | ||
|
||
@Test | ||
@DisplayName("[Success] 전진 조건에 부합하여 true를 리턴합니다.") |
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.
테스트 메서드를 작성할 때, 어노테이션을 사용해서 한글을 사용하는 것과이 메서드명에 한글을 사용하는 방식에 비해 어떤 장점을 가지는지 궁금합니다!
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.
@DisplayName 어노테이션을 사용해서 사용하게 된다면, 일단 굉장히 자유롭고 편하게 작성이 가능합니다!
언더바에 구애받지 않고, 편리하게, 그리고 자유롭게 작성이 가능합니다.
무엇보다, Non-Ascii라는 이유로 IDE가 노란줄을 삑삑 긋는 이슈를 미리 예방할 수 있어요!
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.
혜빈님, 먼저 리드미는 여전히 Description에 명시한 내용 그대로였습니다👍 추가로, 지난 주차 미션 관련해서 언급해주신점도 감사드려요:)
이번 미션에서 DTO를 어떻게하면 좋을지 정말 많이 고민했었는데, Request, Response의 네이밍 그리고 record 부분에 대해서도 더 알아가보겠습니다
예외에서도 Fixture, Enum 활용한 모습까지 Enum을 적극적으로 사용해야겠다는 생각이 들었어요
잘 보고 갑니다!
2주 차 미션 고생 많으셨어요!~
proceedRounds(cars, roundCount); | ||
publishFinalResult(cars); | ||
|
||
Console.close(); |
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.
@seondays 프리코스 API를 참고해보면 입력을 받는 Scanner가 싱글톤 패턴으로 구현되어 있어요. View에서 서로 다른 입력을 받기 위해서 Scanner를 형성하고 다시 닫고, 열고 닫고, 와 같은 과정으로 진행하는 경우 확장성을 고려한다면 static으로 선언한 메리트가 떨어진다고 생각합니다.
개인적으로는 전체적인 로직이 끝난 이후에 .close() 하는게 더 좋은 방법이라고 생각해요!
final Cars cars, | ||
final int roundCount |
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.
@IMWoo94 final 관련해서 이야기하고 싶었던 부분을 언급해주셨습니다!
저도 Reference 타입과 Primitive 타입에서의 불변에 대해서도 한번 더 생각하게 되었어요:)
private static void proceedOneRound(final Cars cars) { | ||
cars.playOneRound(); | ||
|
||
RoundResponses roundResponses = cars.buildRoundResponses(); | ||
View.printListWithNewLine(roundResponses.getResponses()); |
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.
@h-beeen
레이어간 데이터의 교환에 대해서 DTO 라는 개념이 선행되어 코드를 접근하려고 했던 저 자신을 되돌아보게 되는 네이밍입니다! 참고할게요:)
private static final Integer START_SCORE = 0; | ||
|
||
private final Name name; | ||
private Integer score; |
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.
@h-beeen 오토 박싱, 오토 언박싱으로 자동으로 컴파일러가 지원해주는 기능입니다~! 참고하시면 도움이 될 거에요
public void move() { | ||
MovementCondition condition = MovementCondition.create(); | ||
if (condition.canMovable()) { | ||
score++; |
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.
다른 분 코드 리뷰를 하다 더 좋은 방식이지 않을까? 하는 생각에 공유하고 싶어 리뷰 남겨요. 저 역시 score++ 처럼 +1 되는 방향으로 로직을 구성했는데, += 을 활용해서 전진 횟수가 변경될 상황을 고려한 코드가 존재했습니다
if (condition.canMovable()) { score += FORWARD_COUNT }
매직 넘버를 상수 처리함으로써 가독성을 높이고 전진 회수가 1칸이 아닌 다른 n칸인 경우도 생각이 가능하더라구요!
} | ||
|
||
private Integer generateMovementCondition() { | ||
return Randoms.pickNumberInRange(NUMBER_LOWER_BOUND_CONSTRAINT, NUMBER_UPPER_BOUND_CONSTRAINT); |
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.
@h-beeen 다른 분 코드 리뷰에서 언급하셨든 프로덕션 코드와 테스트 코드 간 관계 역시 저와 같은 고민을 하고 계셨더라구요. 테스트를 위한 프로덕션 코드의 수정이 과연 올바른 방향일지, 혹은 단순한 리팩토링으로 봐야 할 지..
저도 더 생각해봐야 할 듯 합니다...
public class MovementCondition { | ||
public static final Integer FORWARD_MOVEMENT_LOWER_BOUND_CONDITION = 4; | ||
public static final Integer NUMBER_LOWER_BOUND_CONSTRAINT = 0; | ||
public static final Integer NUMBER_UPPER_BOUND_CONSTRAINT = 9; | ||
|
||
private final Integer condition; | ||
|
||
private MovementCondition() { | ||
this.condition = generateMovementCondition(); | ||
} | ||
|
||
public static MovementCondition create() { | ||
return new MovementCondition(); | ||
} | ||
|
||
public boolean canMovable() { | ||
return condition >= FORWARD_MOVEMENT_LOWER_BOUND_CONDITION; | ||
} | ||
|
||
private Integer generateMovementCondition() { | ||
return Randoms.pickNumberInRange(NUMBER_LOWER_BOUND_CONSTRAINT, NUMBER_UPPER_BOUND_CONSTRAINT); | ||
} | ||
|
||
} |
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.
@Arachneee 객체의 역할이 많다고 하셨는데, 다른 방법은 어떤게 있는지 알려주실 수 있을까요?? 사실 역할이 많다고 생각하지 않아서..
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.
@h-beeen 저는 Name 그리고 움직인 횟수를 Position 클래스로 각각 독립된 클래스로 구분해서 작성했습니다. 가장 큰 이유는 랜덤 값에 따른 전진 유무에서 Position을 추가로 Wrapping 함수로 정의했어요!
} | ||
} | ||
|
||
private static void validateExceedLength(final String name) { |
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.
@DisplayName("[MovementCondition Test] - Domain Layer") | ||
class MovementConditionTest { | ||
|
||
private static MockedStatic<Randoms> randoms; |
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.
TestConfig 그리고 Enum 2가지를 저도 생각해봐야겠어요!
proceedRounds(cars, roundCount); | ||
publishFinalResult(cars); | ||
|
||
Console.close(); |
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.
제공되는 API에 close()
메서드가 있는 줄 몰랐습니다. 확실히 프로그램 로직이 모두 돌고 난 이후에 명시적으로 close()
를 호출하는 것이 리소스를 안전하게 관리할 수 있다고 생각합니다. 과제를 진행하면서 API를 직접 파헤쳐 볼 생각을 전혀 하지 못했는데, 잘 배우고 갑니다!
혹시 맨 위에 있는 gif파일은 어떻게 올리셨는지 알 수 있을까요 ? 누르면 readme로 이동하는 기능도 있는 게 너무 신기하네요 .. ! |
@@ -0,0 +1,23 @@ | |||
package racingcar.exception; | |||
|
|||
public enum ErrorMessage { |
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.
코드에 정성이 있는것 같아 멋집니다.
하나 추가해야될점은 첫 시작에
"경주할 자동차 이름을 입력하세요." 라는 값에 자동차 입력값이 하나인경우 이셉션 발생코드가 없습니다.
레이싱경주에 자동차 혼자 경주하는경우는 없기 떄문에 최소 2개이상 입력하라는 문구 이셉션이 있었으면좋겠습니다.
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.
물론 경주
라는 명사적 어원이 사람, 동물, 차량 따위가 일정한 거리를 달려 빠르기를 겨루는 일. 또는 그런 경기.
겨루다의 의미가 복수의 의미를 담고 있긴 하지만,
이 부분에 대한 익셉션은 관점에 따라 다를 것 같아요!
저는 한개의 자동차도, 경주 결과를 보여주는 것이 이상적인 프로그램 설계라고 판단했습니다!
요구조건의 이해의 차이인 것 같습니다 :)
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 static boolean hasWhiteSpace(final String name) { | ||
return name.chars().anyMatch(Character::isWhitespace); |
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.
저는 이름 앞뒤에 입력하는 공백 정도는 봐줘도 된다고 생각했는데, 이렇게 엄격하게 처리하는 것도 좋아보이네요 배워갑니다 :)!!
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.
parser를 inputView에서 처리하면서 더 좋은 방법이 없나 싶었는데 이렇게 Parser 클래스로 분리하는 거 좋네요 :) 배워갑니다 !
리드미 최상단에 있는 버셀 링크를 활용한 API를 활용했습니다! 마크다운 문법으로 해당 이미지 클릭시, 제 리드미로 이동하도록 설정했어요! |
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.
궁금한 점 대부분을 많은 분들이 질문 남겨주셔서 간단하게 궁금한 점만 남겼습니다ㅎㅎ정말 고민의 흔적이 많이 보이는 것 같아서 반성하고 가요ㅎㅎ
* [(Each) Car] `MoveCondition.create` 함수를 호출해 1~9 사이의 랜덤 정수를 담은 조건 객체를 생성한다. | ||
* [(Each) Car] `MoveCondition.movable` 함수를 호출해 이동 가능 여부 메세지를 보내고, 이동 여부에 따라 행동한다. | ||
|
||
+ [X] [Game] 각 라운드 종료`(모든 Each Car가 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.
지금 어떤 상황인지, 어떤 클래스에 해당하는 내용인지 정확하게 다 짚어주셔서 README 하나로도 프로그램을 이해할 수 있었어요. 정말 감탄에 또 감탄하고 갑니다..!
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.
감사합니다 :) 리드미를 꼼꼼하게 적은 부분도 많은 분들께 좋은 리뷰를 받았던 것 같아요!
다음 과제에서도 더욱 자세한 명세를 바탕으로 한 리드미를 만들어보겠습니다!
## ✨ 구현 목록 | ||
|
||
### 1️⃣ Non-Functional Requirement | ||
|
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.
기능과 비기능을 나누어 처리하는건 정말 현업에서 꼭 사용하는 방법인데.ᐟ.ᐟ 이미 실무에 대해서도 앞서나가계시네요 ㅎㅎ
* [RoundResponses] stream 문법을 사용해 모든 List<Car>를 순회하며 List<RoundResponse>를 생성한다. | ||
* [RoundResponse] 각 RoundResponse는 name과 score를 가지며, getResponse 편의 메소드로 <br>`이름 : --` 형태로 출력한다. | ||
* [RoundResponses] 레코드의 일급 컬렉션 멤버변수인 List<RoundResponse>를 순회하며 모든 자동차의 라운드 결과를 <br>`이름 : --` 형태의 `String`으로 리턴한다. | ||
* [View] RoundResponses.getResponses()를 통해 각 라운드 진행 현황을 사용자에게 출력한다. |
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.
저는 단순히 출력하는 곳에서는 깊이있게 고민하지 않았는데 출력을 하기까지 비즈니스를 연결지어 생각할 수도 있었네요ㅎㅎ
|
||
-------------------------------------------------------- | ||
|
||
## 🆘 고민의 흔적 |
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.
고민을 함께 적어주셔서 모르는 내용을 찾아가며 공부하는 계기가 되었던 것 같아요.ᐟ.ᐟ
|
||
### 2️⃣ 객체 간 관계를 Static으로 풀어낸 결과, 어려워진 테스트 코드 작성 | ||
|
||
- 도메인 구조를 싱글톤으로 가져가지 않고, 설계하다 보니 테스트 코드 작성 간 어려움을 겪었습니다. |
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.
Game객체는 싱글톤이어도 된다고 생각했어요.ᐟ.ᐟ 혹시 싱글톤을 사용하지 않으신 이유가 있을까요?
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.
여전히 컨트롤러 객체를 싱글톤으로 관리하는 것에 대한 생각이 많습니다!
퓨어한 자바 환경에서 MVC 패턴을 처음 적용해봐서, Controller계층의 설계에 고민이 많은데요.
아직까지는 static method를 활용해 호출하는 방식으로만 사용해 왔어요!
자식클래스를 만들수 없다는 점과, 내부 상태를 변경하기 어렵다는 점 등 여러가지 문제들이 존재한다.
결과적으로 이러한 문제들을 안고있는 싱글톤 패턴은 유연성이 많이 떨어지는 패턴이라고 판단합니다.
아직까지의 볼륨에서는 싱글톤을 무조건 적용해야 할 이유를 아직 찾지 못했어요.
차기 과제에서, 적용할 적합한 과제가 있다면, 한번 적용해보고 싶긴 해요!
<tr><td colspan="3"></td></tr> | ||
<tr> | ||
<td rowspan="1"><b><img align="center" src="https://github.com/woowacourse-precourse/java-racingcar-6/assets/112257466/9a2cdecd-2df4-4541-86ec-4e8fa1017643" height="32px"> README</b></td> | ||
<td>0x00. 어떤 객체가 기능들을 할당하는지도 같이 명시되어 있으면 좋을 것 같다.</td> |
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.
넘버링을 16진수로 하신 것도 눈길이 가네요.ᐟ.ᐟ
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.
코딩변태들이나 한다는 16진수 넘버링..제가 한 번 해봤습니다!
(깨알이지만 0x00부터 시작합니다)
개발자는 숫자를 0부터 센다죠
<td><b>@junseoparkk</b></td> | ||
</tr> | ||
<tr> | ||
<td>0x02. 컨벤션이 없는 상황에서 <b>정적 팩토리 메소드 사용은 혼선</b>을 가져올 수 있다.</td> |
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.
그럴 것 같아요ㅠㅠ저는 이번 미션에서 실수한 부분인데 잘 알고계셨던 부분이라 실수하지 않으신 것 같습니다
final int roundCount | ||
) { | ||
View.printConstantMessage(ROUND_RESULT_MESSAGE); | ||
range(0, roundCount) |
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.
경우에 따라선 static import 여서 더 코드가 깔끔해지기도 하지만, 때로는 이게 어떤 메소드인지 직관적으로 이해하기가 어려울 때가 있는것 같아요ㅎㅎ이 부분에 대해 고민해보는 건 좋은 것 같습니다~!
.forEach(Game::proceedOneRound); | ||
} | ||
|
||
private static void proceedOneRound(final Cars cars) { |
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를 어떤식으로 테스트해야할지, 고민이 많았는데 저 또한 이 부분은 안티패턴이 아닐까 하는 생각이 많았습니다! 이번 과제를 통해 제 고민을 함께 생각해보고 더 나은 방향을 찾게될 수 있을 것 같아요
private static void publishFinalResult(final Cars cars) { | ||
FinalResponse finalResponse = cars.buildFinalResponse(); | ||
System.out.println(finalResponse.getResponse()); | ||
} |
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.
만약, 책임을 분리하며 위임을 한다면 어떨때 좋은 방향인지 고민해보는것도 좋을 것 같아요.ᐟ.ᐟ
컨트롤러에서 유저에게 모델의 데이터를 출력해야 할 때, 이렇게, 뷰-도메인 의존성이 발생했을 때 사용했습니다! |
📦 패키지 구조
✨ 구현 목록
1️⃣ Non-Functional Requirement
2️⃣ Functional Requirement (Controller Code Flow)
[Game] 사용자에게 자동차 이름을
,(Comma)
를 기준으로 입력 받는다[Game] 파싱된 List 문자열로 List를 멤버변수로 갖는 Cars 객체를 생성한다.
[Game] 사용자에게 라운드 횟수를 입력받는다.
[Game] 라운드 횟수에 따라, 각 라운드를 반복 진행한다
MoveCondition.create
함수를 호출해 1~9 사이의 랜덤 정수를 담은 조건 객체를 생성한다.MoveCondition.movable
함수를 호출해 이동 가능 여부 메세지를 보내고, 이동 여부에 따라 행동한다.[Game] 각 라운드 종료
(모든 Each Car가 1회전 경주를 마친 상황)
이름 : --
형태로 출력한다.이름 : --
형태의String
으로 리턴한다.[Game] 모든 라운드 종료
(라운드 횟수 만큼 모든 자동차가 경주를 진행한 상황)
되었을 때최종 우승자 : 해빈, 햅, 빈빈
형태의
String
으로리턴한다.
Scanner.close()
기능을 수행한다.🆘 고민의 흔적
1️⃣ MVC 패턴에서 View Layer와 Domain Layer의 검증 책임
Input Layer에서 유효성 검사를 진행하는게 맞을까에 대한 고민을 꾸준히 해왔습니다.
위 순서로 View Layer에서 검증하지 않고, 별도의 Validator 도메인에서 유효성/자료형 검증을 진행합니다.
이렇게 기본적인 유효성 검증을 마치고, 개발 요구사항에 대한 검증은 일급컬렉션 및 각 도메인의 생성자에서 검증합니다.
위와 같은 방식의 설계가 MVC 패턴에 입각해 좋은 설계로 구성되었는지 봐주시면 좋을 것 같아요. ⸝⸝ʚ ̯ʚ⸝⸝
2️⃣ 객체 간 관계를 Static으로 풀어낸 결과, 어려워진 테스트 코드 작성
도메인 구조를 싱글톤으로 가져가지 않고, 설계하다 보니 테스트 코드 작성 간 어려움을 겪었습니다.
MovementCondition
의 경우, 개별Car
가 매 라운드마다 새로운MovementCondition
객체를 생성하고, 전진 여부를 요청합니다.MovementCondition은 Static 객체라, 해당 조건으로 분기하는 Car 객체는 일반적인 방법으로 테스트를 진행하기 어려웠습니다.
그래서 우회 방법으로
Randoms
객체를 모킹해, Random Generated Value를 모킹해서, 테스트를 진행했습니다.해당 방법은 좋은 테스트 방법이라고 생각되지 않습니다! 이 부분에 대해서 리뷰를 꼭 받아보고 싶어요.
(어쩌면 초기 설계가 부적절 했을 수도 있다고 생각합니다.)
👬 31명과 함께한 223개의 소중한 코드리뷰 적용기
(유니콘 414 에러가 뜨면서 PR창이 뜨지 않는다면 새로고침 해주세요!)
🌱 0x00
어떤 객체가 기능들을 할당하는지도 같이 명시되어 있으면 좋을 것 같다.
🌱 0x01
검증 메소드에서 긍정 조건을 사용하는게 가독성에 좋다.
🌱 0x02
컨벤션이 없는 상황에서 정적 팩토리 메소드 사용은 혼선을 가져올 수 있다.
출처 : Static Factory Method 네이밍 컨벤션 - Tistory, Effective java
🌱 0x03
MVC 패턴에서 모델에서 뷰의 정보를 알아서는 안 된다.
View에서는 DTO의 Response를 단순 출력하는 방식으로 설계했습니다.
🌱 0x04
일부 접근지정자가 세밀하게 조정되지 않았다.
🌱 0x05
Open-Closed Principle vs YAGNI
출처 : YAGNI - Wikipedia
🌱 0x06
Flag 네이밍은 boolean의 의미가 강하다. 정수는 다른 네이밍이 필요하다.
xxFlag
,xxOption
,xxOrNot
과 같이 해석에 혼동을 줄 수 있는 변수와 클래스명 사용을 지양했습니다.🌱 0x07
클래스 내부 함수의 선언 순서가 세밀하게 조정될 필요가 있다.
클래스 내부 선언 순서 컨벤션
출처 : Are there Any Java method ordering conventions? - Stack Overflow
🌱 0x08
예외 복구, 예외 회피, 예외 전환의 역할을 잘 알고 활용하자.
자바에서 예외처리는 예외 복구, 예외 회피, 예외전환을 방식이 각각 있습니다.
출처 : 1주차 숫자야구 코드리뷰 - @IMWoo94
RacingCarException
이라는 전역 예외를 공통으로 던지도록 설계했습니다.hasMessageContaining
메소드를 활용해 해당예외 발생 여부와 Containing ErrorMessage 여부를 이중으로 검증해, 간결하면서 신뢰도 높은 테스트를 만들고자 했습니다.
IllegalArgumentException
을 원시 형태로 발생시키는 것 보다,전역에서 에러를 공통 규격으로 관리하는 것이 더욱 확장성이 높다고 판단합니다.
🌱 0x09.
Protected 생성자 vs Private 생성자
출처 : [JPA] JPA에서 Entity에 protected 생성자를 만드는 이유 - Velog
🌱 0x0a.
조건에 따라 분기하는 것은 검증(Validator)계층이 아니다.
isExitFlag
,isRestartFlag
)와 같은 함수는 값에 대한검증
의 성격을 띄지 않는다는 리뷰를 받았습니다.