-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Step3 - 자동차 경주 #6042
base: minji-go
Are you sure you want to change the base?
Step3 - 자동차 경주 #6042
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생 하셨습니다!
절차지향적이 아닌 객체지향적으로 다시 한번 설계를 고민해보시면 어떨까요!?
혹시 궁금하신게 있다면 언제든 DM보내주세요~
홧팅입니다!
src/main/java/InputView.java
Outdated
tryCount = scanner.nextInt(); | ||
System.out.println(); | ||
|
||
if (carNumber < 1 || tryCount < 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1이라는 값을 상수화를 해서 어떤걸 의미를 가지는지 작성해두면 어떨까요?
src/main/java/InputView.java
Outdated
private int carNumber; | ||
private int tryCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
View가 상태를 가지고 있는게 맞을까요?
src/main/java/InputView.java
Outdated
import java.util.Scanner; | ||
|
||
public class InputView { | ||
private final Scanner scanner = new Scanner(System.in); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static을 붙여 메모리를 아껴보면 어떨까요 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static을 붙이면 테스트시에 Scanner를 공유하게 되어 에러가 나는데, 테스트할 때 close() 후 new Scanner(System.in)으로 재할당하면 될까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아... 테스트 코드를 그렇게 짜셨군요 그럼 일단 이건 현행 유지하시죠!
src/main/java/RacingCar.java
Outdated
@@ -0,0 +1,35 @@ | |||
import java.util.Random; | |||
|
|||
public class RacingCar { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
객체지향적으로 생각해보면
우리가 이 게임에서 필요한 모델은 Car이지 않을까요?
Car라는 도메인이 있고 그 Car가 Position을 가지고 있고, move하는 행위도 스스로 하는거 아닐까요?
이러한 흐름의 객체지향 설계를 고민해보시면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 리뷰 감사합니다 :) 객체지향 개념을 어렴풋이 알고있어서 적용할 생각을 못했네요!
src/main/java/RacingCar.java
Outdated
} | ||
|
||
private boolean canMove() { | ||
return random.nextInt(10) >= 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상수를 통해 해당 코드의 의미를 나타내보시면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다~
좀더 구조를 변경해보기 위한 피드백을 남겼으니 확인한번 부탁드립니다~
궁금한게 있다면 언제든 슬랙 DM으로 연락주세요!
src/main/java/CarRace.java
Outdated
|
||
public class CarRace { | ||
private final ResultView result = new ResultView(); | ||
private final List<Car> cars = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/main/java/ResultView.java
Outdated
System.out.println("실행 결과"); | ||
} | ||
|
||
public void printDash(int count) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
java11을 사용하고 있으니, String.repeat()을 통해 처리하면 더 좋을거 같긴합니다~
src/main/java/Car.java
Outdated
public void move() { | ||
if (canMove()) position++; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
canMove에서 랜덤값을 처리하기 떄문에 실제 테스트를 하기가 어려운 문제가 있습니다. 이유는 position이 1이 될지, 0이 될지 알수가 없기 떄문이죠.
이런 경우 테스트 하기 좋은 코드로 인터페이스를 통해 전략패턴을 구현해볼수 있는데요. 한번 시도해보시면 어떨까요?
https://tecoble.techcourse.co.kr/post/2020-05-17-appropriate_method_for_test_by_interface/
위의 글을 참고해보시면 좋을거 같습니다~
src/main/java/Car.java
Outdated
public class Car { | ||
private static final Random random = new Random(); | ||
private static final int moveThreshold = 4; | ||
private int position; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
position의 유효성은 따로 없을까요?
예를들어 0 이상의 값이여야만 한다. 같은 거요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private void incrementPosition(){
position++;
if(position <0) throw new RuntimeException();
}
이렇게 만들자니 불필요한 검증처럼 느껴지는데, 어떤 걸 의도하신 코멘트이신지 조금 더 설명해주실 수 있으신가요? 😁
클래스 내부 어디에서든 position에 접근해서 조작할 수 있는데 매번 position 값을 조작할 때마다 위에처럼 검증해야하는지도 궁금합니다👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- position을 생성할때 0이 아닌 다른 값을 넣을수도 있는거 아닐까요?
- 말씀하신 함수는 +1 만 하기 때문에 굳이 넣을 필요는 없어보여요~
즉, 저는 생성시 문제가 생길수도 있다! 라는 말을 하고 싶었습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 :) step3 리뷰 반영이 조금 늦었습니다... 열심히 해야겠네요 ㅎㅎ
리팩토링을 하다 몇가지 궁금한게 생겨서 질문도 같이 남겨두겠습니다!
리뷰 항상 감사합니다 👍
src/main/java/InputView.java
Outdated
import java.util.Scanner; | ||
|
||
public class InputView { | ||
private final Scanner scanner = new Scanner(System.in); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static을 붙이면 테스트시에 Scanner를 공유하게 되어 에러가 나는데, 테스트할 때 close() 후 new Scanner(System.in)으로 재할당하면 될까요?
src/main/java/Car.java
Outdated
public class Car { | ||
private static final Random random = new Random(); | ||
private static final int moveThreshold = 4; | ||
private int position; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private void incrementPosition(){
position++;
if(position <0) throw new RuntimeException();
}
이렇게 만들자니 불필요한 검증처럼 느껴지는데, 어떤 걸 의도하신 코멘트이신지 조금 더 설명해주실 수 있으신가요? 😁
클래스 내부 어디에서든 position에 접근해서 조작할 수 있는데 매번 position 값을 조작할 때마다 위에처럼 검증해야하는지도 궁금합니다👀
src/main/java/Car.java
Outdated
public int move() { | ||
return move(new RandomNumberGenerator()); | ||
} | ||
|
||
public int move(NumberGenerator numberGenerator) { | ||
if (isMovable(numberGenerator)) incrementPosition(); | ||
return position; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CarRace 클래스에서 테스트를 위해 move() 메소드에 NumberGenerator를 받을 수 있게 수정했습니다!
-
여기서 public 메소드에서 받아서 private 메소드로 한번 더 전달하고 있는데, 이걸 개선할 수 있을지 궁금합니다.
지금은 isMovable()메소드 까지만 전달하면 되지만, 만약 여러번 전달해야되면 복잡해질 거 같아요 😱 -
공유해주신 링크에서 move() 메서드의 시그니처로 int 보다 NumberGenerator 객체를 사용해서 전달하면 응집도가 높아진다고 표현했는데 그 부분이 정확히 이해가 가지 않습니다! 혹시 int와 NumberGenerator 사용의 차이를 조금 더 설명해주실 수 있으신가용?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이부분에 대한 질문의 의도를 잘 파악하지 못했습니다 ㅠ 혹시 가능하실때 슬랙 DM주시면 구두로 이야기 드릴게요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
남겨주신 질문은 따로 슬랙 DM을 주시면 제가 구두로 답변 한번 드릴게요~
간단한 피드백을 좀더 남겼으니 한번 확인해주시면 감사하겠습니다!
src/main/java/Car.java
Outdated
public class Car { | ||
private static final Random random = new Random(); | ||
private static final int moveThreshold = 4; | ||
private int position; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- position을 생성할때 0이 아닌 다른 값을 넣을수도 있는거 아닐까요?
- 말씀하신 함수는 +1 만 하기 때문에 굳이 넣을 필요는 없어보여요~
즉, 저는 생성시 문제가 생길수도 있다! 라는 말을 하고 싶었습니다!
src/main/java/Car.java
Outdated
public int move() { | ||
return move(new RandomNumberGenerator()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- move를 할때마다 RandomNumberGenerator를 만들 이유가 있을까요?
- 아래의 함수와 차이가 무엇인가요?
src/main/java/Car.java
Outdated
public int move() { | ||
return move(new RandomNumberGenerator()); | ||
} | ||
|
||
public int move(NumberGenerator numberGenerator) { | ||
if (isMovable(numberGenerator)) incrementPosition(); | ||
return position; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이부분에 대한 질문의 의도를 잘 파악하지 못했습니다 ㅠ 혹시 가능하실때 슬랙 DM주시면 구두로 이야기 드릴게요!
src/main/java/InputView.java
Outdated
import java.util.Scanner; | ||
|
||
public class InputView { | ||
private final Scanner scanner = new Scanner(System.in); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아... 테스트 코드를 그렇게 짜셨군요 그럼 일단 이건 현행 유지하시죠!
@Override | ||
public int generate() { | ||
return random.nextInt(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10이라는 값을 상수를 통해 의미 전달을 해볼까요?
src/test/java/CarRaceTest.java
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재는 어떻게 보면 통합테스트만 존재한다고 볼수 있습니다.
Car나 CarGroup과 같은 메서드들의 단위 테스트를 추가해보시면 어떠실까요?
src/main/java/CarGroup.java
Outdated
import java.util.List; | ||
import java.util.stream.Collectors; | ||
|
||
public class CarGroup { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
대부분 collection은 Cars라는 복수를 사용하고 있습니다.
src/main/java/Car.java
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
폴더 구조도 아키텍쳐를 이해하는데에 있어 가장 좋은 방안입니다.
지금은 하나의 폴더에 모든 파일들이 있는데 아키텍쳐에 맞게 폴더(패키지)구조를 짜보시면 어떨까요?
안녕하세요. 이번 미션 리뷰도 잘 부탁드려요 🥰