java-racingcar(songpang) by songpang · Pull Request #3 · biforest/java-racingcar

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 <lastpuzly@gmail.com>

…into songpang

refactor: 컨벤션 수정 Co-authored-by: Gyeongjun Kim <lastpuzly@gmail.com>

…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 <lastpuzly@gmail.com>

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 <lastpuzly@gmail.com>

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


		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());

Conversation

songpang commented Mar 10, 2021

Uh oh!

kouz95 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kouz95 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants