[럿고] 자동차 경주 미션 제출합니다. #104

ksy90101 · 2020-02-13T08:18:58Z

안녕하세요. 이번 리뷰 잘 부탁드리겠습니다!!
이번에 하면서 궁금한게 많아서 리뷰 해주시면 DM으로 많이 물어보겠습니다. 양해 부탁드려요 ㅠㅠ

경우 해당 숫자 반환 테스트

RacingCar

phs1116

안녕하세요, 럿고! 리뷰어 화투입니다.
요구사항 반영 및 테스트 작성 매우 잘해주셨어요.
피드백 추가했는데 참고 부탁드릴게요. :)
궁금하신 점 있으시면 코멘트나 혹은 DM 주세요! :)

phs1116 · 2020-02-15T05:28:47Z

src/main/java/calculator/StringCalculator.java

+        if (text == null || text.isEmpty()) {
+            return 0;
+        }
+        if (text.contains("-")) {


하드코딩 보다는 상수로 빼는게 더 좋을 것 같네요. :)

아직 이런 부분이 부족하네요..! 신경쓰겠습니다 감사합니다!

phs1116 · 2020-02-15T05:29:42Z

src/main/java/calculator/StringCalculator.java

+            return 0;
+        }
+        if (text.contains("-")) {
+            throw new RuntimeException("message");


이펙티브 자바 아이템 75. 예외의 상세 메시지에 실패 관련 정보를 담으라 참고하세요. :)

놓친 부분이네요.... 고쳐놓겠습니다!

phs1116 · 2020-02-15T05:30:55Z

src/main/java/calculator/StringCalculator.java

+
+    private static int splitAndSum(String text) {
+
+        Matcher m = Pattern.compile("//(.)\n(.*)").matcher(text);


이펙티브 자바 아이템6 불필요한 객체 생성을 피하라 참고,
Pattern 객체는 생성 비용이 비싸고 재사용이 가능한 객체입니다. 필드로 빼서 사용하시는게 좋습니다. :)

말씀하신 부분을 한번 읽어 보고, 생성 비용이 비싸고 재사용이 가능한 객체는 무엇이 있는지에 대해서 공부를 해봐야 겠습니다. 감사합니다!

phs1116 · 2020-02-15T05:31:13Z

src/main/java/calculator/StringCalculator.java

+            String[] tokens = m.group(2).split(customDelimiter);
+            return sum(tokens);
+        }
+        String[] numbers = text.split(",|:");


이런 부분에 대해서도 상수로 빼주는게 좋을 것 같네요. :)

넵. 고치겠습니다!

phs1116 · 2020-02-15T05:32:29Z

src/main/java/racingcar/domain/Car.java

+    public String getCurrentPosition() {
+        StringBuilder currentPosition = new StringBuilder(name.getName());
+        currentPosition.append(" : ");
+
+        for (int i = 0; i < position; i++) {
+            currentPosition.append(POSITION_MARK);
+        }
+        return currentPosition.toString();
+    }


getter를 사용하시려면 객체의 상태 그대로를 전달하는게 좋을 것 같네요. :)
뷰에 종속적인 내용이다보니 출력 용 데이터는 도메인 객체가 아닌 View단에서 처리하는게 좋을 것 같습니다.

MVC 패턴에 대해서 공부가 필요하겠네요 ㅠㅠ 감사합니다!

phs1116 · 2020-02-15T05:43:07Z

src/main/java/racingcar/domain/Cars.java

+    public void moveCars() {
+        for (Car car : cars) {
+            car.movePosition(createRandomValue());
+        }
+    }


해당 공개된 메서드는 테스트하기 매우 어려워 보이는데요.
이는 Random이라는 컨트롤 하기 어려운 부분 때문으로 보입니다.
테스트를 위해서 테스트 하기 어려운 부분은 인터페이스로 추상화하는 방법을 많이 사용하곤 합니다.
랜덤이 아닌 특정 값을 돌려주는 인터페이스를 만들고, 랜덤을 사용해서 돌려주는 구현체,
원하는 값을 순서대로 돌려주는 구현체가 있을 수 있겠죠. :)
후자를 Cars에 주입해주면 테스트가 용이하겠죠?

혹은 구현 자체를 다르게 보실수 있을 것 같아요.
랜덤값을 돌려주는 것이 아닌, 이동을 할지 말지 결정을 해주는 객체지만 랜덤 기반으로 판단으로 결졍할지,
혹은 원하는 순서대로 움직이라고 지시를 내리는 객체가 될수도 있구요.

고민해보시고 테스트 하기 쉬운 구조로 변경해보세요. :)

제가 리팩토링한 부분이 말씀하신것인지가 의문입니다..ㅠㅠ 확인후에 아니라고 생각하시면 말씀해주시면 다시 리팩토링해보도록 하겠습니다.

phs1116 · 2020-02-15T05:43:32Z

src/main/java/racingcar/domain/Cars.java

+    public String getCurrentResult() {
+        StringBuilder currentResult = new StringBuilder();
+        for (Car car : cars) {
+            currentResult.append(car.getCurrentPosition());
+            currentResult.append("\n");
+        }
+        return currentResult.toString();
+    }


뷰에서 처리하는게 어울릴 메서드네요. :)

위에 피드백을 확인하고 같이 이동하였습니다!

phs1116 · 2020-02-15T05:44:03Z

src/main/java/racingcar/domain/Cars.java

+    public List<Car> getCars() {
+        return cars;


getCars를 써야할 이유가 있을까요?
만약에 써야한다면 Collections.unmodifiableList 등으로 감싸서 돌려주어서, 불변을 보장해주는게 좋을 것 같네요. :)

넵 그생각을 놓쳤네요 ㅎㅎ 감사합니다!

phs1116 · 2020-02-15T05:44:58Z

src/main/java/racingcar/domain/Names.java

+    }
+
+    public List<Name> getNames() {
+        return names;


Colelctions.unmodifiableList 참고하세요. :)

위에 피드백을 참고하여 같이 조치하였습니다!

phs1116 · 2020-02-15T05:45:22Z

src/main/java/racingcar/domain/Winners.java

+        for (Car car : cars) {
+            if (car.isMaxPosition(maxDistanceCar)) {
+                winners.add(car);
+            }


stream을 공부해서 사용해보셔도 좋을 것 같네요. :)

stream으로 변경하였습니다!

ksy90101 · 2020-02-16T07:42:00Z

이번에는 세세한 사항들을 못지킨것 같습니다. 상수처리와 View에 대한 처리에 대해서도 놓친 부분들이 많은 것 같은데, 그런 부분들을 잡아주셔서 감사합니다. 또한 슬랙으로 질문했던 내용들에 대한 답변도 너무 감사합니다. 막혀있던 부분들이 많이 해소된것 같았습니다~
테스트 하기 어려운 부분은 인터페이스로 추상화 이부분에 대해서는 자료를 찾으면서 조금 더 공부해보도록 하겠습니다~ 1차 피드백 고생하셨습니다~

phs1116

안녕하세요! 리뷰어 화투입니다.
피드백 반영 잘 해주셨는데 놓치신 듯한 부분,
아쉬운 부분이 조금 있어서 피드백 추가했어요!
참고 부탁드릴게요. :)

phs1116 · 2020-02-17T03:46:13Z

src/main/java/racingcar/domain/Car.java

+    }
+
+    @Override
+    public String toString() {


빠트리신 것 같아요. 이름에 대한 내용보다는 객체가 담고있는 데이터 전체를 표현해주는게 좋아요!

phs1116 · 2020-02-17T03:52:29Z

src/main/java/racingcar/view/OutputView.java

+        System.out.println();
+    }
+
+//    public static void printCurrentResult(String result) {


사용하지 않은 코드는 삭제해주세요. :)

phs1116 · 2020-02-17T03:55:32Z

src/main/java/racingcar/domain/Winners.java

+import java.util.List;
+import java.util.stream.Collectors;
+
+public class Winners {


Winners가 클래스 변수로 자동차 리스트를 관리하고 있네요.
동기화 문제 때문에 변경 가능한 값을 static 변수로 사용되는건 권장되지 않습니다. :)
특히 웹 환경에서는 더더욱 잘 사용되지 않구요.

Winners라는 객체를 인스턴스로 관리해보면 어떨까요?
또한 Winners객체는 Cars에서 생성해도 될 것 같아요.

말씀하신대로 변경하였습니다.

phs1116 · 2020-02-17T03:57:35Z

src/main/java/racingcar/domain/Name.java

+
+    private String name;
+
+    public Name(String name) throws IllegalArgumentException {


이것도 다시 확인 부탁드릴게요. :)

phs1116 · 2020-02-17T03:58:28Z

src/main/java/racingcar/domain/Cars.java

+    public void moveCars(int randomValue) {
+        for (Car car : cars) {
+            car.movePosition(randomValue);
+        }
+    }


현재 로직 상 움직일 땐 다같이 움직이는 것 같은데 아닌가요?
요구사항에 맞지 않은 것 같아요. :)
혹시 구현에 어려운 사항 있으시면 DM 주세요!

interface를 해서 구현했는데, 맞는지 모르겠습니다!
제가 너무 안일하게 생각한 것같습니다. 1차 피드백때 말씀해주신 방법대로 한번 코딩해봤습니다!

페어의 피드백을 보고 완전히 터득했습니다! 말씀하신 방법이 맞을꺼같아요!
https://www.slideshare.net/ssuser59a869/ksug-2019?fbclid=IwAR2UcYpT58l7AUEPj8vAXIUpllk4meCTlLk4aGDZJSnaECgMwpRR1sVkYAc
(51페이지부터 참조)

private CarMoveValueGenerator carMoveValueGenerator = () -> (int)(Math.random() * RANDOM_LIMIT_VALUE);

이런식으로 람다를 이용해서 인터페이스에 구현했습니다! 어려웠지만 깨닫고 나니깐 너무 뿌듯하네요! 좋은 패턴 알려주셔서 너무 감사합니다!

Signed-off-by: Seyun <ksy90101@gmail.com>

phs1116

안녕하세요! 럿고.
구현 잘해주셨어요. 이번 단계는 머지할게요!
간단한 피드백 하나 남겼는데 참고 부탁드릴게요.
개발하시느라 수고 많으셨어요. :)

phs1116 · 2020-02-18T03:30:39Z

src/main/java/racingcar/domain/Cars.java

+import java.util.stream.Collectors;
+
+public class Cars {
+    public static List<Car> winners = new ArrayList<>();


public static 으로 가져가면 동기화 문제가 있을 수 있을것 같네요. :)
Cars가 가진 Car가 Cars 인스턴스마다 다를텐데 클래스 변수로 들고 있어야할까요?

또한 public이 아닌 캡슐화를 고민해보시면 좋을 것 같습니다. :0

클래스 변수가 아니라 지역변수로 했는데, 맞는지 잘 모르겠네요! 일주일 동안 고생하셨습니다~ 감사합니다!

YebinK and others added 29 commits February 11, 2020 15:49

[DOCS] 문자열 덧셈 계산기 요구 사항 작성

0022dd7

[FEATURE] 입력값이 null 또는 빈문자열인 경우 0을 반환 기능 추가

ba078de

[FEATURE] 숫자 하나를 문자열로 입력할

8a0924e

경우 해당 숫자 반환 테스트

[FEATURE] 숫자 두개를 쉼표(,) 구분자로 입력할 경우 두 숫자의 합 반환 기능 구현

d305c05

[FEATURE] 구분자를 쉼표(,) 이외에 콜론(:)을 사용하는 경우 기능 구현

b4956f0

[FEATURE] 커스텀 구분자를 지정 기능 구현

1398f05

[FEATURE] 음수를 전달할 경우 RuntimeException 예외 발생 기능 구현

643b30f

[DOCS] 자동차 경주 게임 요구사항 작성

98e65a3

[FEATURE] 자동차 이름 쉼표 기준으로 구분 기능 구현

4f0db60

[FEATURE] 공백이 입력되었을 경우 예외처리 기능 구현

710a7e6

[FEATURE] 이름이 6자 이상 입력되었을 경우 예외처리 기능 구현

7622dfa

[FEATURE] 이름 입력받기 기능 구현

5133e61

[FEATURE] 시도 횟수가 숫자가 아닌 경우 예외처리 구현

efb2554

[FEATURE] 시도 횟수가 양수가 아닌 경우 예외처리 기능 구현

2454c99

[FEATURE] 자동차에 이름, 포지션 부여 기능 구현

032e9f9

[FEATURE] 자동차 전진 기능 구현

b2be83a

[FEATURE] 랜덤값을 부여해 자동차 움직임 기능 구현

18cfd3d

[FEATURE] 전진 횟수를 -로 출력

f39db65

[FEATURE] 전체 자동차 진행상황 출력

e86547e

[FEATURE] 시도 횟수만큼 진행상황 출력

eea92f6

[FEATURE] 최종 우승자 출력 기능 구현

dedc0c3

[REFACTOR] 우승자 출력 로직 리팩토링

199cebf

[REFACTOR] 이름 입력받기 리팩토링

a6a1ee3

[REFACTOR] 시도횟수 입력 받기 리팩토링

22c2e95

[REFACTOR] 전체 테스트 코드 리팩토링

e33873e

[REFACTOR] 우승자 출력 리팩토링

a90a426

[FEATURE] 쉼표 입력시 예외처리 구현

e69ee92

[REFACTOR] README.md 수정

3523bbf

Merge pull request #1 from YebinK/racingcar

9f06b4f

RacingCar

phs1116 suggested changes Feb 15, 2020

View reviewed changes

ksy90101 added 6 commits February 16, 2020 15:05

[REFACOTR] 음수 기호 상수로 감싸기 및 예외 메시지 변경

220de6b

[REFACOTR] Metcher를 필드로 변경

9edf9cd

[REFACTOR] 구분문자 하드코딩 상수 처리

4c99e33

[REFACTOR] 중간결과를 출력해주는 부분 View단으로 이동

a017534

[REFACTOR] 우승자를 선택하는 과정 for문을 stream으로 변경

55b62e6

[REFACTOR] Random 객체 생성 후, 랜덤 값 생성

ddeac55

[REFACTOR] 전진값 상수 처리 및 최대 움직임 확인 메소드 조건 변경

9a12ace

phs1116 suggested changes Feb 17, 2020

View reviewed changes

ksy90101 added 8 commits February 17, 2020 13:07

[REFACTOR] 주석 제거

0006205

[REFACOTR] 랜덤값 인터페이스화 리팩토링

ac0a8db

[REFACOTR] 랜덤값 인터페이스화 리팩토링 및 Unchecked Exception 중복 제거

3558196

[REFACTOR] 랜덤 테스트 삭제

69bb282

[REFACTOR] winners 객체 삭제

a26c9ba

Signed-off-by: Seyun <ksy90101@gmail.com>

[REFACOTR] Random 객체 삭제 및 테스트 하기 쉬운 코드로 작성

a9f29ea

[REFACOT] Game 로직 메소드 분리

c5302a4

[REFACOTR] Fommetter 및 전반적인 리팩토링

d0c10c2

phs1116 approved these changes Feb 18, 2020

View reviewed changes

phs1116 merged commit 5f754f5 into woowacourse:ksy90101 Feb 18, 2020


		private static int splitAndSum(String text) {

		Matcher m = Pattern.compile("//(.)\n(.*)").matcher(text);


		private String name;

		public Name(String name) throws IllegalArgumentException {

[럿고] 자동차 경주 미션 제출합니다. #104

[럿고] 자동차 경주 미션 제출합니다. #104

Conversation

ksy90101 commented Feb 13, 2020

phs1116 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

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

ksy90101 commented Feb 16, 2020

phs1116 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

phs1116 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment