Conversation
ImTotem
left a comment
There was a problem hiding this comment.
과제 진행 요구사항에 "기능을 구현하기 전 docs/README.md에 구현할 기능 목록을 정리해 추가한다"가 있어요. 구현한 기능들을 정리해서 채워보면 좋겠습니다.
| public class Application { | ||
| public static void main(String[] args) { | ||
| // TODO: 프로그램 구현 | ||
| Scanner scanner = new Scanner(System.in); |
There was a problem hiding this comment.
프로그래밍 요구사항에 "Scanner API 대신 camp.nextstep.edu.missionutils.Console의 readLine()을 사용하여 구현해야 한다"고 되어 있어요. 이 부분은 변경이 필요합니다.
| try { | ||
| n= scanner.nextInt(); | ||
| if (n<=0){ | ||
| throw new IllegalArgumentException("0번이하"); | ||
| } | ||
| } catch (Exception e){ | ||
| throw new IllegalArgumentException("숫자비었음"); | ||
| } |
There was a problem hiding this comment.
catch (Exception e)가 모든 예외를 잡기 때문에, n<=0일 때 던진 IllegalArgumentException("0번이하")도 여기서 잡혀서 "숫자비었음"으로 바뀌어요. 의도한 동작인지 한 번 확인해보면 좋겠습니다.
| import java.util.*; | ||
|
|
||
|
|
||
| class CarNames { |
There was a problem hiding this comment.
이 클래스는 하나의 자동차를 표현하고 있는데, 이름이 복수형(CarNames)이에요. Car처럼 단수형으로 쓰면 의미가 더 명확해질 것 같아요.
| System.out.println("경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)"); | ||
| String input = scanner.nextLine(); | ||
|
|
||
| String[] a = input.split(","); |
There was a problem hiding this comment.
a라는 변수명은 어떤 데이터인지 파악하기 어려워요. carNames처럼 의미가 드러나는 이름을 쓰면 코드를 읽기 더 편해집니다. n, r, c 등도 같이 살펴보면 좋겠습니다.
|
|
||
| import camp.nextstep.edu.missionutils.Randoms; | ||
|
|
||
| import java.util.*; |
There was a problem hiding this comment.
와일드카드(*) import보다는 실제 사용하는 클래스(ArrayList, HashSet 등)를 명시적으로 import하면 어떤 클래스를 쓰고 있는지 한눈에 보여서 더 좋아요.
| for(int i=n; i>0; i--){ | ||
| for(CarNames c:cars){ | ||
| int r = Randoms.pickNumberInRange(0,9); | ||
| if(r>=4){ |
There was a problem hiding this comment.
4라는 숫자가 "전진 기준값"이라는 의미인데, 매직 넘버 대신 상수로 분리하면 코드 의도가 더 명확해질 것 같아요.
ImTotem
left a comment
There was a problem hiding this comment.
1주차에서 main 하나에 모든 로직이 있던 것을 Car, Game, Input, Winner 등 여러 클래스로 분리한 점은 좋아요 👍 여기서 한 발 더 나아가 각 클래스의 책임을 더 명확하게 나눠보면 좋겠습니다.
| import java.util.*; | ||
|
|
||
| public class Input { | ||
| Scanner scanner = new Scanner(System.in); |
There was a problem hiding this comment.
1주차에서도 언급했는데, 프로그래밍 요구사항에 "Scanner 대신 camp.nextstep.edu.missionutils.Console의 readLine()을 사용해야 한다"고 되어 있어요. 이 부분은 꼭 변경이 필요합니다.
| String name; | ||
| int moveCount; |
There was a problem hiding this comment.
2주차에서 다룬 캡슐화 관점에서, 이 필드들의 접근 제어자를 한 번 생각해보면 좋겠습니다. 외부에서 직접 접근할 수 있어야 하는지, 아니면 메서드를 통해서만 접근하게 할지 고민해보면 좋겠습니다.
| if (r >= 4) { | ||
| c.move(); | ||
| } | ||
| System.out.printf("%s : %s%n", c.name, "-".repeat(c.getMoveCount())); |
There was a problem hiding this comment.
Game 클래스가 게임 진행과 출력을 모두 담당하고 있는데, '객체의 책임' 관점에서 출력은 어느 객체가 맡는 게 적절할지 생각해보면 좋을 것 같아요.
| if (r >= 4) { | ||
| c.move(); | ||
| } | ||
| System.out.printf("%s : %s%n", c.name, "-".repeat(c.getMoveCount())); |
There was a problem hiding this comment.
c.name으로 필드에 직접 접근하고 있는데, 캡슐화를 적용하면 어떤 방식으로 접근하는 게 좋을지 생각해보면 좋겠습니다.
| import java.util.Set; | ||
|
|
||
|
|
||
| public class Input2 { |
There was a problem hiding this comment.
Input2라는 클래스명은 이 클래스가 어떤 역할을 하는지 파악하기 어려워요. 실제로 하는 일(검증)이 드러나는 이름으로 바꾸면 어떨까요?
| for (int i = n; i > 0; i--) { | ||
| for (Car c : cars) { | ||
| int r = Randoms.pickNumberInRange(0, 9); | ||
| if (r >= 4) { |
There was a problem hiding this comment.
4라는 숫자가 '전진 기준값'이라는 의미를 담고 있는데, 매직 넘버 대신 상수로 분리하면 어떨까요?
| public class Input2 { | ||
| List<Car> cars = new ArrayList<>(); | ||
|
|
||
| public List<Car> vCarNames(String a){ |
There was a problem hiding this comment.
매개변수 a와 메서드명 vCarNames가 어떤 의미인지 파악하기 어려워요. validateCarNames(String input) 같이 의미가 드러나는 이름을 쓰면 코드를 읽기 더 편해집니다.
| @@ -0,0 +1,15 @@ | |||
| package racingcar; | |||
|
|
|||
| class Car {// 클래스 이름 단수형으로 | |||
There was a problem hiding this comment.
피드백 반영 좋아요 👍
이전 리뷰 피드백 메모가 코드 주석으로 남아 있어요. 제출 전에 정리해주면 좋겠습니다.
| public int getMoveCount(){ | ||
| return moveCount; | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
newline에 대해 알아보아요
https://bcsdlab.slack.com/archives/CGQK77GCB/p1774940985721839
No description provided.