[미션1] 구현 완료했습니다. 리뷰 요청드립니다. by sunghyuki · Pull Request #7 · sproutt/heroes2-java-mission

sunghyuki · 2020-08-19T02:57:21Z

description :
•어려웠던 점
객체 지향 프로그래밍이 익숙하지 않아서 객체가 하는 일을 규정하고, 한 객체가 어떤 책임을 갖고 다른 객체들과 협력해야 하는지 로직을 디자인하는데 있어서 시간이 많이 걸렸던 것 같다.
캡슐 내의 요소들에 대한 세부 구현 사항을 외부로부터 숨기는 객체 지향의 관점에서 프로그래밍을 하는 능력이 부족한 것 같아서 자료를 많이 찾아보고 공부하고 있는 중이다.
한 메서드에 오직 한 단계의 들여쓰기만 해야 하는 규칙을 지키기가 어려웠던 것 같다.

hyukjin-lee

고생하셨습니다 ~
열심히 고민하신 흔적이 보이네요 !
피드백 조금 남겼으니 확인 후 반영해주시면 감사하겠습니다

hyukjin-lee · 2020-08-23T02:00:40Z

src/Car.java

+        return position;
+    }
+
+    public void setPosition(int position) {


규칙 : setter 를 사용하지 않는다 (getter 사용은 지양한다)

객체지향 프로그래밍에서 setter 와 getter 사용을 되도록 지양하라고 하는 이유가 무엇일까요?

불필요한 setter/getter는 객체의 속성을 언제든 변경할 수 있는 상태가 되기 때문에 지양하도록 하는 것 같습니다! 굳이 사용하지 않아도 된다면 반영해보도록 하겠습니다!

hyukjin-lee · 2020-08-23T02:02:52Z

src/Car.java

+        if(random.nextInt(10) >= 4){
+            this.setPosition(this.getPosition()+1);
+        }
+    }


랜덤을 통해 특정 조건에서 전진하는 게임의 룰을 자동차라는 모델 클래스가 알고 있을 필요가 있을까요?
예를 들어 게임의 모드가 추가되어서 랜덤 룰이 아닌 다른 게임 룰이 생긴다고 했을 때,
모델이 게임의 룰에 관계없이 pure 하게 짜여져 있으면 모델을 수정하지 않아도 되지 않을까요?
moveForward 가 단순히 car 의 position 을 ++ 하는 로직만 가지고 있으면 어떤지 의견드려봅니다 ~

게임의 룰을 자동차 모델 클래스가 가지고 있을 필요는 없다고 생각했는데 보다 더 좋은 방법을 아직 생각해보지 못한 것 같습니다. 원래는 Rule 클래스를 따로 만들어 구현하려고 했으나 아직 방법을 고민 중에 있습니다! 고민해서 반영할 수 있도록 노력해보겠습니다

hyukjin-lee · 2020-08-23T02:03:27Z

src/Car.java

+    public void goForward(){
+        Random random = new Random();
+        if(random.nextInt(10) >= 4){
+            this.setPosition(this.getPosition()+1);


다음에는 code formatting 을 한 뒤에 제출하는 습관을 들이시면 좋을 것 같아요 ~ :)

넵 code formatting 하는 습관을 들이겠습니다!

hyukjin-lee · 2020-08-23T02:05:47Z

src/Car.java

+
+    public void goForward(){
+        Random random = new Random();
+        if(random.nextInt(10) >= 4){


10, 4 가 어떤 의미를 지니고 있는지 2년 뒤에 이 코드를 보거나 타인이 보면 이해가 잘 안될 수 있을 것 같아요.
이런 magic number 를 상수로 추출해보는게 어떨까요?

magic number 역시 추후에 봤을 때 이해할 수 있도록 상수로 선언하는 습관을 들이겠습니다!

hyukjin-lee · 2020-08-23T02:07:06Z

src/InputView.java

+import java.util.Scanner;
+
+public class InputView {
+    private static final Integer MAX_LENGTH = 6;


primitive type 이 아니라 wrapper type 으로 선언하신 이유가 있을까요?

hyukjin-lee · 2020-08-23T02:23:00Z

src/Race.java

+import java.util.stream.Collectors;
+
+public class Race {
+    final static String drive = "-";


접근 제어자가 생략되었네요.
앞의 클래스에서는 static final 로 쓰셨던데 통일해보면 어떨까요?
상수의 네이밍 컨벤션은 ALL_대문자 입니다 ~

메모하겠습니다!

hyukjin-lee · 2020-08-23T02:23:17Z

src/Race.java

+
+public class Race {
+    final static String drive = "-";
+    private List<Car> carLineUp = new ArrayList<>();


cars 라는 네이밍이 좋을 것 같아요

초기화를 생성자를 통해 하지 않고 필드에서 직접 해주는 이유가 있나요?

hyukjin-lee · 2020-08-23T02:26:02Z

src/Race.java

+    private List<Car> carLineUp = new ArrayList<>();
+    private int trial;
+    InputView inputView = new InputView();
+    OutputView outputView = new OutputView();


Race 라는 도메인 클래스에서 View 에 대한 의존성을 없애면 어떨까요?
나중에 요구사항이 늘어나서 View 가 변경되면 그에 따라 Race 클래스가 영향을 받게 되는데
여기서 View 에 대한 책임을 Main(RacingGame) 으로 위임하면 추후에 view 에 변경이 생겨도
도메인 클래스들에는 영향이 최소화 되지 않을까요~?

src/Race.java

hyukjin-lee · 2020-08-23T02:29:57Z

src/RacingGame.java

@@ -0,0 +1,8 @@
+public class RacingGame {


전체적으로 테스트 코드가 없어서 아쉽네요 ㅠㅠ

hyukjin-lee

고생하셨습니다~
추가 피드백 남겼으니 확인 부탁드려요~

hyukjin-lee · 2020-09-02T03:57:57Z

src/InputView.java

+
+    private static void processStreamAboutCheckName(String[] trimmedInputCarName) {
+        Stream<String> stream = Arrays.stream(trimmedInputCarName);
+        stream.forEach(e -> checkNameLength(e));


메서드 체이닝을 적용해서 Arrays.stream(trimmedInputCarName).forEach(~~)
로 이어 붙이시면 어떨까요

hyukjin-lee · 2020-09-02T04:10:36Z

src/Race.java

+
+    public void race(List<Car> cars) {
+        for(Car car: cars) {
+            goOrStay(car);


레이스에 참여하는 cars 의 정보를 직접 Race 가 들고 있으면 어떨까요?
getWinner 부터 race 까지
메서드를 사용함에 있어서 객체가 상태를 들고 있지 않기 때문에
응집도가 떨어지고 사용하는 쪽의 클래스와 결합도가 커지는 것 같습니다.

hyukjin-lee · 2020-09-02T04:12:16Z

src/Race.java

+import java.util.stream.Collectors;
+
+public class Race {
+    Rule rule = new Rule();


접근제어자 표기를 안하신 이유가 있을까요?
Rule 객체는 상태가 없는 객체인데 util 클래스 목적으로 추출하신걸까요?
util 목적으로 사용하실 클래스라면, Race 객체가 생성될 때마다 함께 생성될 필요가 있을까요?

hyukjin-lee · 2020-09-02T04:12:57Z

src/Race.java

+    public List<String> getWinner(List<Car> cars) {
+        List<Car> winner = new ArrayList<>();
+        int positionOfWinner;
+        positionOfWinner = findPositionOfWinner(cars);


선언과 할당을 같은 곳에서 해주면 가독성에 좀 더 유리할 것 같습니다

hyukjin-lee · 2020-09-02T04:14:19Z

src/Race.java

+    }
+
+    public List<String> getWinner(List<Car> cars) {
+        List<Car> winner = new ArrayList<>();


여기서 객체 할당을 해주는 것은 의미없는 메모리 낭비일 것 같습니다.
(stream 연산을 통해 변형된 cars 를 할당받고 있기 때문에)

hyukjin-lee · 2020-09-02T04:34:44Z

test/RacingGameTest.java

+import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertThat;
+
+public class RacingGameTest {


view 를 제외한 모든 클래스에 대해 테스트를 진행해보시면 어떨까요?

hyukjin-lee · 2020-09-02T05:52:24Z

test/RacingGameTest.java

+    }
+
+    @Test
+    public void inputCarName() throws NoSuchMethodException, InvocationTargetException, IllegalAccessException {


reflection 을 사용하지 않고 테스트를 구현해보시면 어떨까요?

관련해서 도움이 될 만한 자료 첨부드립니다
blog.benelog.net/2685835.html

[미션1] 구현 완료했습니다. 리뷰 요청드립니다.

d59f99a

hyukjin-lee requested changes Aug 23, 2020

View reviewed changes

sunghyuki added 2 commits August 26, 2020 09:57

refactoring: 피드백 일부 반영

6ab9666

refactoring: 피드백 반영

55a5ff1

hyukjin-lee requested changes Sep 2, 2020

View reviewed changes

sunghyuki added 2 commits September 6, 2020 15:44

refactoring: 2차 피드백 일부 반영

211f0a3

add: 테스트 코드 추가

fc33cc4

Conversation

sunghyuki commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hyukjin-lee 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!

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!

hyukjin-lee 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sunghyuki commented Aug 19, 2020 •

edited

Loading