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

glassk · 2020-08-20T09:03:22Z

저번 과제에서 구현하지 못했던 예외 처리에 대해서 공부하고, 1자 미만이나 6자 이상의 이름을 입력한 경우에 대한 사용자 정의 예외 처리를 구현했습니다.
Array와 ArrayList의 차이점에 대해 공부하고 적용했습니다. 사용자가 입력한 이름 문자열을 split한 것을 담는 Array와 우승자를 담는 ArrayList로 구분하여 사용했습니다.
racingGame 클래스에 대한 단위 테스트에서 어떤 결과에 대해 assertThat을 적용해야 하는 것인지 감을 잡지 못했습니다. 그래서 동일 인스턴스인지에 대한 타입을 비교하는 것으로 다수 검증을 하였는데 관련 예제를 많이 접하고 이해하도록 하겠습니다.
한 메서드에 오직 한 단계의 들여쓰기만 해야 하는 규칙을 지키기 어려웠지만 그만큼 하나의 역할만 하는 메서드의 필요성을 체감할 수 있었습니다.
저번 과제를 통해 알게 된 객체지향 생활체조에서 마지막 원칙인 getter/setter 사용 금지에 대해 다시 정리해보면서 getter/setter를 사용하지 않는 방향으로 코드를 수정했습니다.

hyukjin-lee

고생하셨습니다 ~
Winner 에 대한 로직을 분리시키신거 👍
피드백 조금 남겼으니 확인 후 반영해주시면 감사하겠습니다
몇개 안되니 반영 완료해주시면 다른 부분에 대한 피드백 추가로 드리겠습니다 ~

hyukjin-lee · 2020-08-23T04:07:45Z

src/mission1/Car.java

+    public int position;
+
+    public int addPosition() {
+        return this.position++;


addPosition 도 데이터 변경에 대한 설명이 되는 것 같지만,
run 이라는 행위를 표현하는 단어로 메서드명을 지어보면 어떨까요?

추후에 자동차 기본 이동 조건이 +2 로 바뀐다고 가정 했을 때
addPosition 이면 해당 메서드를 사용하는 입장에서 position+1 일 것 같다는 생각이 드는데
run 으로 추상화 되어있으면 +2 를 하든 +1 을 하든 자동차가 이동한다는 행위를 나타내서 혼동이 없을 것 같습니다 ~

메서드명을 지을 때 조건 변경의 가능성을 꼭 고려하도록 하겠습니다! 단번에 이해되는 좋은 예시 제시해주셔서 감사합니다.

hyukjin-lee · 2020-08-23T04:08:16Z

src/mission1/Car.java

+        System.out.print(this.name + " : ");
+        for (int i = 0; i < this.position; i++) {
+            System.out.print("-");
+        }


결과를 보여주는 View 의 역할은 ResultView 에 위임하면 어떨까요?

추후에 콘솔이 아닌 GUI 프로그램으로 확장된다고 했을 때 Car 이라는 pure 해야하는 모델 클래스가 더럽혀지지 않을까요~?

GUI 프로그램으로 확장될 경우도 고려해야 한다는 점 명심하겠습니다!

hyukjin-lee · 2020-08-23T04:10:02Z

src/mission1/GameWinner.java

+import java.util.ArrayList;
+
+public class GameWinner {
+    public ArrayList<String> findWinner(Car[] car) {


조슈아 블로크의 effective java 에는 '객체는 인터페이스를 사용해 참조하라.'
라는 부분이 있습니다.
한번 읽어보시고 개선해보시면 좋을 것 같아요 ~
jaehun2841.github.io/2019/03/01/effective-java-item64/#%EC%9C%A0%EC%97%B0%ED%95%9C-%ED%94%84%EB%A1%9C%EA%B7%B8%EB%9E%A8%EC%9D%84-%EC%83%9D%EC%84%B1%ED%95%98%EB%8A%94-%EC%9D%B8%ED%84%B0%ED%8E%98%EC%9D%B4%EC%8A%A4-%ED%83%80%EC%9E%85-%EB%B3%80%EC%88%98

배열 사용을 지양하고 List 로 최대한 넘겨받으면 어떨까요?

자료 감사합니다👍 List인터페이스 구현해보도록 하겠습니다.

hyukjin-lee · 2020-08-23T04:12:16Z

src/mission1/Main.java

+        for (int i = 0; i < time; i++) {
+            racingGame.raceByTime(car);
+            resultView.showResultByTime(car);
+            System.out.println();


Main 에 콘솔 출력하는 코드를 없애보면 어떨까요~?

hyukjin-lee · 2020-08-23T04:12:56Z

src/mission1/RacingGame.java

+import java.util.Random;
+
+public class RacingGame {
+    static final int RANDOM_RANGE = 10;


접근제어자를 생략하신 이유가 있을까요? 생략하면 어떤 접근제어자가 붙을까요?

메서드의 접근제어자에 몰두한 나머지 변수의 접근제어자는 미처 고려하지 못했던 것 같습니다. 생략할 경우 default 접근제어자가 되어 해당 패키지 내에서 접근가능하게 합니다. 여기에서는 해당 클래스 내에서만의 접근이 필요하므로 private를 붙여 수정하겠습니다.

glassk

피드백 감사합니다👍 반영해서 수정하도록 하겠습니다!

glassk · 2020-08-23T04:36:54Z

src/mission1/Car.java

+    public int position;
+
+    public int addPosition() {
+        return this.position++;


메서드명을 지을 때 조건 변경의 가능성을 꼭 고려하도록 하겠습니다! 단번에 이해되는 좋은 예시 제시해주셔서 감사합니다.

glassk · 2020-08-23T04:37:08Z

src/mission1/Car.java

+        System.out.print(this.name + " : ");
+        for (int i = 0; i < this.position; i++) {
+            System.out.print("-");
+        }


GUI 프로그램으로 확장될 경우도 고려해야 한다는 점 명심하겠습니다!

glassk · 2020-08-23T04:54:06Z

src/mission1/RacingGame.java

+import java.util.Random;
+
+public class RacingGame {
+    static final int RANDOM_RANGE = 10;


메서드의 접근제어자에 몰두한 나머지 변수의 접근제어자는 미처 고려하지 못했던 것 같습니다. 생략할 경우 default 접근제어자가 되어 해당 패키지 내에서 접근가능하게 합니다. 여기에서는 해당 클래스 내에서만의 접근이 필요하므로 private를 붙여 수정하겠습니다.

glassk · 2020-08-23T07:58:11Z

src/mission1/GameWinner.java

+import java.util.ArrayList;
+
+public class GameWinner {
+    public ArrayList<String> findWinner(Car[] car) {


자료 감사합니다👍 List인터페이스 구현해보도록 하겠습니다.

hyukjin-lee

잘 수정해주셨네요 ! 2차 피드백 간단하게 드립니다 ~

hyukjin-lee · 2020-09-05T04:58:32Z

src/mission1/Car.java

@@ -0,0 +1,8 @@
+public class Car {
+    public String name;
+    public int position;


car 의 상태가 외부에서 접근가능한 public 인데요~
private 으로 변경해주셔야할 것 같습니다 !

hyukjin-lee · 2020-09-05T04:59:57Z

src/mission1/GameWinner.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class GameWinner {


Winner 객체에서 들고 있으면 좋을만한 상태(필드 변수)들을 정의하고 책임을 부여해보면 어떨까요?

hyukjin-lee · 2020-09-05T05:00:32Z

src/mission1/GameWinner.java

+
+public class GameWinner {
+    public List<String> findWinner(List<Car> cars) {
+        int max = cars.get(0).position;


객체지향에서는 객체의 상태에 직접 접근해서는 안됩니다.
private 으로 변경하시면 getter 로 변경하셔야할 것 같아요~

hyukjin-lee · 2020-09-05T05:07:09Z

src/mission1/GameWinner.java

+        List<String> winnerList = new ArrayList<>();
+        for (Car car : cars) {
+            addWinnerList(car, max, winnerList);
+        }
+        return winnerList;


java8 의 stream api 를 적용해보면 어떨까요~?

hyukjin-lee · 2020-09-05T05:08:45Z

src/mission1/InputView.java

+import java.util.List;
+import java.util.ArrayList;
+
+public class InputView {


InputView 에는 상태값이 없는데요,
util 클래스로 활용될 수 있을 것 같아서 모든 메서드들에 static 을 활용해보시면 어떨까요?

hyukjin-lee · 2020-09-05T05:10:18Z

src/mission1/RacingGame.java

+    private static final int RANDOM_RANGE = 10;
+    private static final int MIN_MOVE_STANDARD = 4;
+
+    public void raceByTime(List<Car> cars) {


random 으로 인해 게임의 핵심 pulbic 메서드가 테스트 불가능해졌는데
어떻게 개선해 볼 수 있을까요?

glassk added 8 commits August 19, 2020 23:49

Car 객체 구현

3f99a4c

InputView Class 및 예외 처리

00145ad

핵심 비즈니스 로직 구현

3e43773

ResultView Class

a6aa9b4

Main Class

8dc2694

inputCarName Method와 registerCar Method 분리

c60d5ab

Unit Test 구현

b1abcd6

setter/getter 삭제

8240ecf

hyukjin-lee requested changes Aug 23, 2020

View reviewed changes

glassk commented Aug 23, 2020

View reviewed changes

1차 피드백 반영 수정

34998ef

hyukjin-lee approved these changes Sep 5, 2020

View reviewed changes

Conversation

glassk commented Aug 20, 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!

glassk 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

glassk commented Aug 20, 2020 •

edited

Loading