[자동차 경주 미션] 정다영 미션 제출합니다 #24
Conversation
TaeyeonRoyce
left a comment
TaeyeonRoyce
left a comment
There was a problem hiding this comment.
안녕하세요, 다영님!
자바가 익숙치 않다고 하셨던것 같은데, 잘 작성해주셨네요.
코드에 대한 전반적인 설명과 의도를 코멘트에서 설명해주셔서 리뷰하는데 수월했습니다👍
로직들 각각에 맞게 분리하려는 고민이 돋보였네요.
몇가지 코멘트 남겼으니 확인해주세요!
완료하지 못한 테스트와 예외처리도 추가하시면 좋을 것 같아요ㅎㅎ
코멘트 답변
java와 객체에 대한 개념이 부족하여 패키지, 클래스를 어느 기준으로 구별해야 깔끔하고 명확해지는지
클래스를 먼저 말씀드리면 역할이나 논리를 기준으로 나누는편이 자연스러워요.
사실 정답은 없는 영역이라 딱 말씀드리긴 어렵네요..
반대로, 왜 클래스를 나누는지에 대해 생각해보면 좋겠습니다.
코멘트로 남긴 사례를 가져와볼게요.
다영님이 Car라는 클래스를 만들고 이와 관련한 로직을 모았습니다.
다른 개발자가 "자동차가 움직임"이라는 로직을 찾으려면 현재 구조에서는 Car라는 클래스에서 존재하길 기대하겠죠..?
하지만, Car라는 클래스에 getRandomNumber()라는 멤버가 존재한다면 혼동을 줄수도 있죠.
비슷한 로직(관심사)끼리 모으는 것 역시 클래스를 나누는 이유중 하나가 아닐까 싶습니다.
패키지의 경우는 나누는 기준이 클래스에 비해 다양합니다. 계층으로 나눌 수도 있고, 역할이나 도메인 등등의 기준들이 다양하게 있지만 중요한건 하나의 기준으로만 나누어져 있어야 한다는 점입니다.
명확하게 잘라 기준을 설명드리긴 어렵네요.. 다른사람의 코드도 많이 보고, 많이 고민하면서 작성하시다 보면 다영님만의 기준이 세워질것 같습니다...!
코드를 더 간결하게 만들 수 있는 부분이 있는지
간결한 코드보단 사람이 읽기 좋은 코드로 생각하면 좋습니다!
간결할수록 읽기 편하지만 가독성과는 조금 다른 영역일수 있어요.
return power > 4 ? this.position + 1 : this.position
// or
return moveOrStopByPower(power);
private int moveOrStopByPower(int power) {
if (power > 4) {
return this.position + 1;
}
return this.position
}위 코드에서 아래 예시가 길이가 길어도, 어떤 역할을 하는지 더 명확하게 보이지 않나요?
특별한 경우가 아니라면, 간결함보단 개발자가 읽기 편한 코드를 고민해보는걸 권장합니다!
-
변수명이 보기에 불편하지 않은지
불편하지 않아요! -
테스트를 진행하려고 public으로 대부분 작성했는데, 어떤 기준으로 해야할지 (단순 작동이 가능하게 하려고 진행한 것이라 명확한 기준이 없었습니다.)
테스트를 위한 프로덕션 코드의 수정은 불필요한 작업일 수 있어요. 의도적으로private으로 선언한 메서드가 테스트 때문에 공개되면 의도가 변경되니까요.
아래 예시 테스트에서는private으로 담긴 메서드의 로직을 테스트 했는데, 참고해보세요!
src/test/java/domain/CarNameTest.java -
MVC 패턴에 맞게 작성된게 맞는지, domain/veiw/Application 으로 제대로 나누어 진 것인지
코멘트에서 확인해주세요!
| public static void main(final String... args) { | ||
| final var carNames = InputView.getCarNames();//이름입력 | ||
| final var tryCount = InputView.getTryCount();//횟수입력 |
There was a problem hiding this comment.
final 이 해당 메서드에만 추가되어있네요.
일관성있게 수정해주세요!
| public static final int INITIAL_POSITION = 1; //위치 기본값 상수 | ||
| public static final int MOVE_VALUE = 4;//움직임 기준 상수 | ||
| private final String name; | ||
| private int position; |
There was a problem hiding this comment.
공백을 통해 상수와 필드를 분리해서 보여주면 좋습니다!
| public static final int INITIAL_POSITION = 1; //위치 기본값 상수 | |
| public static final int MOVE_VALUE = 4;//움직임 기준 상수 | |
| private final String name; | |
| private int position; | |
| public static final int INITIAL_POSITION = 1; //위치 기본값 상수 | |
| public static final int MOVE_VALUE = 4;//움직임 기준 상수 | |
| private final String name; | |
| private int position; |
| } | ||
|
|
||
| public void playGame() { | ||
| System.out.println("실행결과"); |
There was a problem hiding this comment.
간단하더라도 출력과 관련된 로직은 domain으로부터 분리되어야 합니다.
| public int getRndNum() { //랜덤 변수생성(0~9) | ||
| Random random = new Random(); | ||
| return random.nextInt(10); | ||
| } |
There was a problem hiding this comment.
랜덤 숫자 생성가 Car객체의 멤버(메서드)로 있는게 어색하지 않나요?
숫자생성 로직을 다른 클래스로 분리해봐도 좋을 것 같아요.
| private int position; | ||
|
|
||
| public Car(String name) { //Car 객체 생성 | ||
| this.name = name; |
There was a problem hiding this comment.
자동차 이름의 길이에 대한 요구사항이 누락되었습니다.
- 자동차 이름은 쉼표(,)를 기준으로 구분하며 이름은 5자 이하만 가능하다.
InputView에서 관련한 검증이 이루어지고 있네요.
자동차 이름이 5자 이하라는 규칙은 도메인에서 드러나지 않아 헷갈렸습니다.
다영님이 정의하신 InputView를 통해서 입력 받고 자동차를 생성하는 경우에는 해당 규칙이 반영되지만,
자동차 도메인은 그런 규칙이 존재하지 않네요.
테스트에서 new Car("12345678")로 생성해도 정상적으로 동작할 것 같아요.
해당 규칙을 도메인에서도 검증이 필요할 것 같습니다!
| if (isNameValid(trimmedName)) { | ||
| cars.add(trimmedName); | ||
| } |
There was a problem hiding this comment.
5글자 이상의 자동차는 무시가 되는군요.
코멘트 남겨주신 것처럼 추가 학습후에 예외상황으로 처리하면 좋겠습니다!
| if (winners.isEmpty()) { | ||
| System.out.println("우승자가 없습니다."); | ||
| } else { |
| for (int i = 0; i < winners.size(); i++) { | ||
| System.out.print(winners.get(i)); | ||
| if (i != winners.size() - 1) { | ||
| System.out.print(", "); | ||
| } | ||
| } |
There was a problem hiding this comment.
String.join() 혹은 StringJoiner를 활용해보세요!
| @@ -0,0 +1,2 @@ | |||
| public class test { | |||
| void testRndNum(){ | ||
| Random random = new Random(); // Random 객체 생성 | ||
| int randomNumber = random.nextInt(10); // 랜덤 값 생성 | ||
| assertTrue(randomNumber >= 0 && randomNumber <= 9, "랜덤 숫자가 0부터 9 사이여야 합니다."); |
There was a problem hiding this comment.
AssertJ를 사용하면 assertThat().isBetween()과 같은 다양한 검증 메서드를 활용할 수 있습니다!
3 participants
✅해결하지 못한점
✅미션을 진행하며 생긴 궁금한 점들/수정해야할 사항
자바에 익숙치 않아 수정해야할 부분이 많이 보이는 것 같습니다! 여러 방면에서 조언해주시면 반영하도록 하겠습니다
+) 원격과 로컬연결에서 오류가 생겨 레포토리지를 처음부터 다시 생성해서 진행해서 단계별 과정이 없어졌습니다ㅠㅠ.
보시는데 불편하실 것 같아서 죄송합니다. 이점 다음 미션부터는 주의 하겠습니다.