java-racingcar(songpang) #3

songpang · 2021-03-10T14:54:31Z

java-racingcar 미션 제출합니다 !
좋은 하루 되세요 :)

kouz95

고생하셨습니다!
리뷰 확인해주시고, 혹시라도 이해가 안되는 부분이 있거나 질문이 있으시다면 코멘트 남겨주세요! 🙂

kouz95 · 2021-03-27T15:48:06Z

src/main/java/domain/GamePlayer.java

+        if (randomNumber >= WINNER_CONDITION)
+            car.moveForward();


컨벤션을 확인해주세요 🙂

kouz95 · 2021-03-27T15:51:11Z

src/main/java/domain/GamePlayer.java

+    ArrayList<Car> inputNames() {
+        String messageCode = Message.GeneralMessages.INPUT_NAMEOFCAR.getMessage();
+        printer.printMessages(messageCode);
+        ArrayList<String> names = new ArrayList<>(Arrays.asList(receiver.receiveName()));
+
+        return makeCarList(names);
+    }
+
+    ArrayList<Car> makeCarList(List<String> names) {
+        ArrayList<Car> cars = new ArrayList<>();
+
+        for (int i = 0; i < names.size(); i++)
+            cars.add(new Car(names.get(i)));
+
+        return cars;
+    }


Suggested change

ArrayList<Car> inputNames() {

String messageCode = Message.GeneralMessages.INPUT_NAMEOFCAR.getMessage();

printer.printMessages(messageCode);

ArrayList<String> names = new ArrayList<>(Arrays.asList(receiver.receiveName()));

return makeCarList(names);

}

ArrayList<Car> makeCarList(List<String> names) {

ArrayList<Car> cars = new ArrayList<>();

for (int i = 0; i < names.size(); i++)

cars.add(new Car(names.get(i)));

return cars;

}

List<Car> inputNames() {

String messageCode = Message.GeneralMessages.INPUT_NAMEOFCAR.getMessage();

printer.printMessages(messageCode);

List<String> names = new ArrayList<>(Arrays.asList(receiver.receiveName()));

return makeCarList(names);

}

List<Car> makeCarList(List<String> names) {

List<Car> cars = new ArrayList<>();

for (int i = 0; i < names.size(); i++)

cars.add(new Car(names.get(i)));

return cars;

}

자바의 유명한 규칙중엔 program to interfaces not implementations 라는 부분이 있습니다.

객체 지향 기초 이야기 3, 유연함(Flexibility)

위 글 참고해보시면 좋을것 같아요 🙂

kouz95 · 2021-03-27T15:53:09Z

src/main/java/domain/GamePlayer.java

+        this.generator = new Generator();
+    }
+
+    void judgeAndMove(Car car, int randomNumber) {


judgeAndMove 라는것 자체로 메소드가 두가지 일을 하고 있다고 생각할 수 있지 않을까요?
메소드는 한가지 일만 할 수 있도록 메소드를 분리해봅시다 🙂

kouz95 · 2021-03-27T15:55:52Z

src/main/java/domain/GamePlayer.java

+import java.util.Arrays;
+import java.util.List;
+
+public class GamePlayer {


대부분의 메소드의 접근 제한자가 default인데 의도하신 건가요?

[Java] 접근 제한자

처음에 public으로 설정했다가 동민님이 외부에서 사용하지 않는데 굳이 public을 사용한 이유가 있냐고 피드백을 주셔서 이에 대해 고민해 본 이후 default로 변경했습니다!

src/main/java/domain/Validator.java

kouz95 · 2021-03-27T15:58:19Z

src/main/java/domain/Validator.java

+    private static final Character COMMA = ',';
+    private static final String NOTHING = "";
+
+    Printer printer;


접근 제한자가 필요할것 같아요!

src/main/java/domain/Winner.java

kouz95 · 2021-03-27T16:01:24Z

src/test/java/domain/ValidatorTest.java

+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class ValidatorTest {


실패하는 테스트가 많습니다!
테스트는 전부 통과해야합니다. 예외를 테스트하는 경우엔 예외가 발생한다는것을 테스트 해주세요!

refactor: 컨벤션 수정 Co-authored-by: Gyeongjun Kim <[email protected]>

…into songpang

refactor: 컨벤션 수정 Co-authored-by: Gyeongjun Kim <[email protected]>

…into songpang

kouz95

추가적인 리뷰 드렸으니 확인해주세요 🙏

리뷰 이외에도 컨벤션이 지켜지지 않은 부분이 있는데 전체적으로 확인해보시면 좋을것 같아요.
for문이나 if, while문 사이의 공백 같은 경우는 IDE을 이용하여 전체 reformat을 할 수 있으니 시도해보세요! 정렬하고 싶은 파일이나 패키지에 우클릭하여 Reformat Code를 누르면 됩니다 🙂

kouz95 · 2021-03-29T10:51:38Z

src/main/java/domain/GamePlayer.java

+import java.util.Arrays;
+import java.util.List;
+
+public class GamePlayer {


아마 클래스 내부에서만 사용되는 메소드를 public으로 설정해서 그런 피드백을 주지 않았나 생각이 되네요 🙂

클래스 내부에서만 사용되는 메소드는 private, 외부에서 메세지를 전달받는 메소드는 public으로 설정하는 것이 좋을 것 같아요.

kouz95 · 2021-03-29T10:53:31Z

src/main/java/domain/Validator.java

+        printer = new Printer();
+    }
+
+    public boolean validateName(String s) {


약어를 사용하기보단 적절한 이름을 사용해주세요!

kouz95 · 2021-03-29T10:54:55Z

src/main/java/domain/Validator.java

+
+    public boolean inputSameName(String s) {
+        String messageCode = Message.ExceptionMessages.INPUT_SAME_NAME.getMessage();
+        List<String> carNames = Arrays.asList(s.split(","));


","를 상수화 하여 의미를 표현해줄수 있지 않을까요?

kouz95 · 2021-03-29T10:58:00Z

src/test/java/domain/ValidatorTest.java

+
+    @ParameterizedTest
+    @ValueSource(strings = {"123,!@a,BDs5", ",,asd,asd,"})
+        //이름 유효성 체크


테스트 메소드의 의도를 드러낼땐 @DisplayName 을 사용하여 표현해줄수 있습니다.
@ParameterizedTest의 경우엔 name 속성에 DisplayName을 부여할 수 있어요 🙂

kouz95 · 2021-03-29T10:59:30Z

src/main/java/domain/GamePlayer.java

+    List<Car> inputNames() {
+        String messageCode = Message.GeneralMessages.INPUT_NAMEOFCAR.getMessage();
+        printer.printMessages(messageCode);
+        List<String> names = new ArrayList<>(Arrays.asList(receiver.receiveName()));


Suggested change

List<String> names = new ArrayList<>(Arrays.asList(receiver.receiveName()));

List<String> names = Arrays.asList(receiver.receiveName());

new ArrayList<>()를 안해줘도 괜찮습니다.

kouz95 · 2021-03-29T11:04:01Z

src/main/java/domain/GamePlayer.java

+        return receiver.receiveNumber();
+    }
+
+    List<Car> makeCarList(List<String> names) {


List<Car> 라는 return type으로 이미 List임을 알 수 있지 않을까요?

의미 있게 구분하라.
구분 짓지 못하는 변수명은 의미가 없다. 예를 들어 ProductInfo와 ProductData의 개념은 분리 되는가?
또 변수 뒤에 타입을 작성하는 것도 의미가 없다면 좋지 않다. NameString과 Name이 다른 점이 무엇인가?

- Clean Code

makeCars 정도로 표현 할 수 있을 것 같아요 🙂

kouz95 · 2021-03-29T11:06:20Z

src/main/java/domain/Winner.java

+    public static String makeWinnerToString(List<Car> cars) {
+        StringBuilder winner = new StringBuilder(cars.get(0).getName());
+
+        if (cars.size() > 1) {
+            for (int i = 1; i < cars.size(); i++) {
+                winner.append(", ").append(cars.get(i).getName());
+            }
+        }
+
+        return winner.toString();
+    }


Winner가 우승자가 컴마로 이어진다는("a,b") 사실을 알 필요가 있을까요?
Winner의 책임은 우승자가 누군지만 알려줘도 괜찮지 않을까요?
비지니스 로직과 뷰로직을 분리해봅시다!

kouz95 · 2021-03-29T11:09:20Z

src/main/java/domain/Car.java

+    public String getProgressWithSymbol() {
+        String progress = "";
+        for(int i = 0;i < this.position;i++) {
+            progress += "-";
+        }
+
+        return progress;
+    }


현재 위치를 "-" 로 표현된다는건 뷰의 관심사인것 같아요.
현재의 콘솔 환경에선 "-", 웹 환경이라면 자동차의 위치에 맞는 이미지가 될수도 있지 않을까요?

kouz95 · 2021-03-29T11:10:04Z

src/main/java/io/Printer.java

+public class Printer {
+    Message message = new Message();
+
+    private static String DEFAULT_SYMBOL = "-";


사용되지 않는것 같네요 🙂
또한, 상수로 사용할땐 private static final을 사용해주세요!

kouz95 · 2021-03-29T11:13:27Z

src/main/java/io/Receiver.java

+    boolean reEnter = false;
+    private String inputLine = "";


reEnter 와 inputLine이 인스턴스 변수로 선언되어 있는것이 괜찮을까요?

chae-heechan added 30 commits March 4, 2021 17:38

docs: 기능 구현 목록 및 클래스 정리

0a5d963

test: 입력 및 유효성 검사 클래스 테스트 케이스 작성

216cb6d

feat: Receiver 클래스 구현

ae04e19

feat: Validator 인터페이스 생성

a6e8c0f

docs: 클래스 분류 재정리

804fe2d

feat: Message인터페이스 생성

a02977d

test: validator클래스 구조 변경에 따른 테스트 변경

3ac806d

feat: 출력 메세지 인터페이스 작성

a973407

refactor: 메세지 호출 메서드 삭제, 입력 예외 추가

78bb06d

feat: 프린터 클래스 구현

bb9c87b

feat: 게임 시작 기능 추가

07f6628

docs: 클래스 및 메서드 분류 재정리

cdfc826

feat: Car클래스 구현

0ff13cc

feat: Message클래스 구현

6519379

feat: Validator클래스 구현

7aaac17

refactor: receiveLine 메서드 삭제

b115436

feat: 게임 실행 부분 구현

8d840f7

feat: 난수 생성 기능 구현

74d8054

style: overSizeCharacters 메서드 이름 변경

02b5186

test: Paramiterizedtest로 변경

6954307

feat: 게임 내 출력 메서드 추가

305ea0d

test: Receiver 테스트 케이스 작성

fc3b773

docs: Paramiterized 테스트 사용하기 위해 testCompile 부분 추가

e0fe4fa

style: overSizeCharacters 메서드 이름 변경

2d34588

style: 불필요한 라인 제거

e1c98f3

test: Car 테스트 케이스 작성

38feeaa

test: GamePlayer 테스트 케이스 작성

c47f82f

test: Generator 테스트 케이스 작성

2c62d0c

test: Message 테스트 케이스 작성

838a38b

test: Printer 테스트 케이스 작성

1a10b46

songpang added 16 commits March 21, 2021 03:23

refactor: 메소드 및 변수 네이밍 수정

66dd3b6

refactor: 위험한 접근 제어자 수정 및 불필요한 코드 제거

a595c43

refactor: 위와 같음

0852319

refactor: assertThat actual과 expected의 오용 수정

422c3da

refactor: 절차지향적인 코드 수정

8d80c12

refactor: 이번 미션에서 불필요한 생성자 제거

645be80

refactor: 불필요한 코드제거 및 획일성을 위한 상수 설정

302139e

refactor: Printer클래스의 역할 제한 및 관련 메소드 이동

93dde0d

refactor: Car 클래스에 maxNumber를 확인하는 역할 부여

bfb6277

refactor: MessageInterface 삭제 및 Message클래스 내부 Hashmap에서 enum으로 변경

b87577f

refactor: 이전 커밋의 변경사항 Validator Class에 적용

9a31667

refactor: GamePlayer Class 위의 커밋 내용 적용

51d46b3

refactor: Printer Class의 Progress bar 만드는 메소드 Car Class로 위임

5d7064e

refactor: 사용하지 사용하지 않거나 불필요한 interface 삭제

b3444f0

refactor: Winner Class 생성, Gameplayer Class 역할 분리

978faee

refactor: Car Class에 자신의 position을 Progress bar로 리턴하는 역할 부여

eb18b53

songpang requested a review from kouz95 March 26, 2021 13:58

refactor: README 수정

021f98d

kouz95 requested changes Mar 27, 2021

View reviewed changes

songpang and others added 9 commits March 28, 2021 13:47

refactor: 컨벤션 수정 및 구현체를 인터페이스로 변경

c296369

refactor: 메소드 역할 분리

e577df7

refactor: 제한자 및 컨벤션 수정

560f251

Update src/main/java/domain/Validator.java

8f84168

refactor: 컨벤션 수정 Co-authored-by: Gyeongjun Kim <[email protected]>

Merge branch 'songpang' of https://github.com/songpang/java-racingcar …

0428c22

…into songpang

Update src/main/java/domain/Winner.java

ed92c1f

refactor: 컨벤션 수정 Co-authored-by: Gyeongjun Kim <[email protected]>

refactor: 접근 제한자 설정 및 변수명 변경

83eaf8c

refactor: 실패 케이스 삭제

54109bd

Merge branch 'songpang' of https://github.com/songpang/java-racingcar …

506b630

…into songpang

songpang requested a review from kouz95 March 28, 2021 05:24

kouz95 requested changes Mar 29, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

java-racingcar(songpang) #3

java-racingcar(songpang) #3

songpang commented Mar 10, 2021

kouz95 left a comment

kouz95 Mar 27, 2021

kouz95 Mar 27, 2021

kouz95 Mar 27, 2021

kouz95 Mar 27, 2021

songpang Mar 28, 2021

kouz95 Mar 27, 2021

kouz95 Mar 27, 2021

kouz95 left a comment

kouz95 Mar 29, 2021

kouz95 Mar 29, 2021

kouz95 Mar 29, 2021

kouz95 Mar 29, 2021

kouz95 Mar 29, 2021

kouz95 Mar 29, 2021

kouz95 Mar 29, 2021

kouz95 Mar 29, 2021

kouz95 Mar 29, 2021

kouz95 Mar 29, 2021


		import static org.assertj.core.api.Assertions.assertThat;

		class ValidatorTest {

	List<String> names = new ArrayList<>(Arrays.asList(receiver.receiveName()));
	List<String> names = Arrays.asList(receiver.receiveName());

java-racingcar(songpang) #3

Are you sure you want to change the base?

java-racingcar(songpang) #3

Conversation

songpang commented Mar 10, 2021

kouz95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kouz95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment