-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[자동차 경주] 김정아 미션 제출합니다. #1461
base: main
Are you sure you want to change the base?
[자동차 경주] 김정아 미션 제출합니다. #1461
Changes from all commits
65d4451
ec38a02
b06fd84
6a6a76f
d5cb4c2
d0cbccf
f51f72d
2fe989a
32b49b9
48b2d8e
500a325
8cf39a5
dd98b05
82611c7
e8d8a9e
66e76bf
4deefb8
f3d23f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,3 +33,10 @@ out/ | |
|
||
### Mac OS ### | ||
.DS_Store | ||
|
||
### gradle ### | ||
/build/ | ||
/.gradle/ | ||
/gradle/ | ||
/gradlew | ||
/gradlew.bat |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,28 @@ | ||
# java-racingcar-precourse | ||
|
||
### 주요 기능 | ||
|
||
- 자동차 이름을 입력받는 기능: | ||
|
||
사용자가 경주할 자동차들의 이름을 쉼표로 구분하여 입력받는다. | ||
|
||
- 자동차 전진 판단 기능: | ||
|
||
랜덤 수 생성기를 사용하여 4 이하인 경우 전진하고, 그렇지 않은 경우 0을 출력한다. | ||
|
||
- 전진 상황 출력 기능: | ||
|
||
각 회차별로 자동차들이 얼마나 전진했는지 출력한다. | ||
|
||
- 최종 우승자 선정 기능: | ||
|
||
전진 회차가 종료된 후 가장 멀리 전진한 자동차를 선정한다. 동점자가 있을 경우 공동 우승자로 인정한다. | ||
|
||
|
||
### 예외처리 기능 | ||
|
||
- 이름 입력, 전진 횟수 입력에서 null이나 빈칸이 입력으로 들어왔을 경우 `IllegalArgumentException`을 발생시킨다. | ||
- 이름 입력에서 자동차의 이름이 5자가 넘어갈 경우 `IllegalArgumentException` 을 발생시킨다. | ||
- 이름 입력에서 동일한 이름이 입력 될 경우 `IllegalArgumentException` 을 발생시킨다. | ||
- 전진 횟수 입력에서 숫자가 아닌 값이 들어올 경우 `IllegalArgumentException` 을 발생시킨다. | ||
- 전진 횟수 입력에서 양의 정수가 아닌 값이 입력되면 `IllegalArgumentException` 을 발생시킨다. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,14 @@ | ||
package racingcar; | ||
|
||
import racingcar.controller.RacingGame; | ||
import racingcar.view.InputView; | ||
import racingcar.view.OutputView; | ||
|
||
public class Application { | ||
public static void main(String[] args) { | ||
// TODO: 프로그램 구현 | ||
InputView inputView = new InputView(); | ||
OutputView outputView = new OutputView(); | ||
RacingGame racingGame = new RacingGame(inputView, outputView); | ||
racingGame.run(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
package racingcar.common; | ||
|
||
public enum IOMessage { | ||
|
||
INPUT_CAR_NAMES("경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)"), | ||
INPUT_ROUND_COUNT("시도할 횟수는 몇 회인가요?"), | ||
RESULT_NOTICE("실행 결과"), | ||
WINNER_ANNOUNCEMENT("최종 우승자 : "), | ||
CAR_POSITION_FORMAT("%s : %s"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 다른 분도 비슷한 코멘트를 달아주셔서 생각을 해 봤는데, 차지하는 크기(효율)를 생각하면 static으로 유틸리티 클래스를 만들기가 베스트인것 같고, 상수라는 느낌을 강하게(?) 명확하게(?) 주는것은 enum이 베스트인것 같습니다! 이 경우에서는 규모가 작아서 유틸리티 클래스를 쓰는게 나은 것 같아요! 좋은 의견이에요😊 |
||
|
||
private final String text; | ||
|
||
IOMessage(String text) { | ||
this.text = text; | ||
} | ||
|
||
public String getText() { | ||
return text; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
package racingcar.common; | ||
|
||
public class Limit { | ||
public static final int MOVE_VALUE = 1; | ||
public static final int MOVE_CRITERIA_VALUE = 4; | ||
public static final int MIN_RANDOM_VALUE = 0; | ||
public static final int MAX_RANDOM_VALUE = 9; | ||
public static final int MAX_NAME_LENGTH = 5; | ||
public static final int MIN_NAME_LENGTH = 1; | ||
|
||
|
||
private Limit() { | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 추상 클래스는 서브클래스에서 추가적으로 구현하도록 하는 용도로 사용되는 것으로 알고 있어서 추상 클래스를 사용할 경우 자칫하면 추가적으로 구현할, 혹은 구현 될 항목이 있다 라는 의미로 비춰져서 리뷰어 혹은 동업자에게 혼란을 줄 수 있고, Limit가 제공하는 역할에 맞지 않다고 생각했습니다. 그래서 생성자 제한을 두어서 상수만을 제공하는 클래스(유틸리티 클래스와 비슷한 구조)로 만들었습니다. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
package racingcar.common; | ||
|
||
public enum Symbol { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 위에서도 비슷한 의견이 나왔어요! 추상 클래스는 서브클래스에서 추가적으로 구현하도록 하는 용도로 사용되는 것으로 알고 있어서 추상 클래스를 사용할 경우 자칫하면 추가적으로 구현할, 혹은 구현 될 항목이 있다 라는 의미로 비춰져서 리뷰어 혹은 동업자에게 혼란을 줄 수 있고, Limit가 제공하는 역할에 맞지 않다고 생각했습니다. 그래서 생성자 제한을 두어서 상수만을 제공하는 클래스(유틸리티 클래스 비슷한 구조)로 만들었습니다. |
||
SEPARATE_MARKER(","), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 변수들의 의미를 갖는 이름이 좋은 것 같습니다 |
||
POSITION_MARKER("-"); | ||
|
||
private final String symbol; | ||
|
||
Symbol(String symbol) { | ||
this.symbol = symbol; | ||
} | ||
|
||
public String getSymbol() { | ||
return symbol; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
package racingcar.common; | ||
|
||
import java.util.List; | ||
|
||
public class Validator { | ||
|
||
private Validator() { | ||
|
||
} | ||
|
||
public static void validateCarNames(List<String> carNames) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 자동차 이름을 검증하는 역할은 Car나 CarGroup에게 넘겨서 객체를 생성할때 확인해줘도 좋은 방법일 것 같습니다. 그러면 Car와 관련된 모든 로직이 한곳에 응집됨으로써 도메인의 응집성을 높일 수 있습니다. 그래서 저는 Validator 클래스를 따로 만들지 않고 검증에 대한 메소드는 각 도메인에 구현하는 편입니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 예외처리를 따로 구현해서 책임 분리를 확실하게 하고, 한 곳에소 좀 더 편리하게 유지보수 하자! 가 목표였는데, 이런 작은 프로젝트에서 Validator를 분리하는 것은 확실히 과투자라는 생각이 드네요... 좋은 의견이에요! 혹시 클래스 내부에 선언할때 클래스 안에 static으로 Validator를 선언하여 예외 로직 부분을 분리하는것은 어떻게 생각하세요?? 이 방법과 따로 파일을 분리하는것을 고민하다가 이렇게 했습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. static으로 선언하려는 이유는 뭔가요? |
||
if (carNames == null || carNames.isEmpty()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isEmpty 대신 isBlank를 사용하면 공백 처리까지 할 수 있을 것 같습니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 원래 |
||
throw new IllegalArgumentException(); | ||
} | ||
if (hasDuplicated(carNames)) { | ||
throw new IllegalArgumentException(); | ||
} | ||
for (String name : carNames) { | ||
if (name.length() < Limit.MIN_NAME_LENGTH || name.length() > Limit.MAX_NAME_LENGTH) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분도 hasDuplicated처럼 별도의 메서드로 분리할 수 있을 것 같습니다. 혹은, 검증과 예외 처리를 하나의 메서드로 묶어 checkCarNameLength 같은 이름으로 구현해도 좋을 것 같습니다. 그러면 validateCarNames 함수가 어떤 검증들을 하는지 조금 더 쉽게 파악할 수 있을 것 같네요! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 원래는 다른 검증 메서드를 여러개 만든 후 한 메서드에서 모두 선언해서 예외 처리 메서드를 한번 호출하는것으로 모든 예외검증이 가능하도록 할 생각이였는데, 이상하게 구현한걸 이제 깨달았네요... 다음에는 의도한대로 신경써서 만들어 봐야겠네요! 감사합니다! |
||
throw new IllegalArgumentException(); | ||
} | ||
} | ||
} | ||
|
||
private static boolean hasDuplicated(List<String> items) { | ||
return items.size() != items.stream().distinct().count(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 제가 봤던 중복 확인 코드 중에 가장 깔끔하네요. 잘 배우고 갑니다! |
||
} | ||
|
||
public static void validateRoundCount(int roundCount) { | ||
if (roundCount < 1) { | ||
throw new IllegalArgumentException(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exception에 메세지를 담으면 이게 무슨 에러인지 리뷰어 입장에서 와닿을 것 같아요 |
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
package racingcar.controller; | ||
|
||
import racingcar.domain.Car; | ||
import racingcar.domain.CarGroup; | ||
import racingcar.view.InputView; | ||
import racingcar.view.OutputView; | ||
|
||
public class RacingGame { | ||
|
||
private final InputView inputView; | ||
private final OutputView outputView; | ||
|
||
public RacingGame(InputView inputView, OutputView outputView) { | ||
this.inputView = inputView; | ||
this.outputView = outputView; | ||
} | ||
|
||
public void run(){ | ||
CarGroup carGroup = new CarGroup(inputView.readCars()); | ||
int roundCount = inputView.readRoundCount(); | ||
for(int i = 0; i<roundCount; i++){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cargroup에게 책임을 할당하고 결과를 한 번에 출력하는 방법은 어떨까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 분리 해야지 해야지 해놓고 시간이 모자라서 분리를 못 했네요ㅠㅠ |
||
carGroup.moveCars(); | ||
outputView.printRoundResult(carGroup); | ||
} | ||
|
||
outputView.printWinners(carGroup.getWinners()); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 함수만 봐도 자동차 게임의 진행 흐름을 쉽게 파악할 수 있어, 정말 깔끔하고 좋은 코드인 것 같습니다! |
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
package racingcar.domain; | ||
|
||
import camp.nextstep.edu.missionutils.Randoms; | ||
import racingcar.common.Limit; | ||
|
||
import java.util.random.RandomGenerator; | ||
|
||
public class Car { | ||
private final String name; | ||
private int position = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 분리를 할지 말지 고민을 했었는데, 규모가 작아서 분리를 하면 오히려 과투자 일 것 같아서 분리를 하지 않았습니다. |
||
|
||
public Car(String name) { | ||
this.name = name; | ||
} | ||
|
||
public void move() { | ||
if (isMove()) { | ||
position += Limit.MOVE_VALUE; | ||
} | ||
} | ||
|
||
private boolean isMove() { | ||
int number = Randoms.pickNumberInRange(Limit.MIN_RANDOM_VALUE, Limit.MAX_RANDOM_VALUE); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 랜덤한 숫자 생성을 private 메서드로 넣으면 테스트하기 힘들 수도 있지 않을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 저는 이런 경우 private 메소드가 관여되어 있는 move() 메소드를 통해 간접적으로 테스트를 하곤 합니다 |
||
return number >= Limit.MOVE_CRITERIA_VALUE; | ||
} | ||
|
||
public int getPosition(){ | ||
return position; | ||
} | ||
|
||
public String getName(){ | ||
return name; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
package racingcar.domain; | ||
|
||
import racingcar.common.IOMessage; | ||
import racingcar.common.Symbol; | ||
|
||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 일급 컬렉션 비슷한것을 의도한것은 맞습니다! 하지만 주된 이유는 분리를 하면 테스트에 용이하다는 것이였는데, 그와 관련된 테스트 작성을 못했습니다... 만약 완벽하게 일급컬렉션으로 선언하려면 반환하려는 리스트를 방어적 복사를 한 후 결과값을 반환하는 로직이 추가로 필요합니다. final로 선언을 한 것으로는 문제가 없을 줄 알았는데 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저도 다른 분 코드 보면서 알게된거라! 다음에는 활용해보려구요! |
||
private final List<Car> cars; | ||
|
||
public CarGroup(List<String> carNames) { | ||
this.cars = carNames.stream() | ||
.map(Car::new) | ||
.toList(); | ||
} | ||
|
||
public void moveCars() { | ||
for (Car car : cars) { | ||
car.move(); | ||
} | ||
} | ||
|
||
public List<Car> getWinners() { | ||
int maxPosition = getMaxPosition(); | ||
return cars.stream() | ||
.filter(car -> car.getPosition() == maxPosition) | ||
.toList(); | ||
} | ||
|
||
public void printCarPositions() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CarGroup 클래스에서 OutputView 기능을 맡고 있는 것 같은데 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 코딩을 하던 도중, getter를 쓰면 캡슐화가 깨진다는 글을 보고 심취해서 이렇게 했었는데, 막상 하고나니 제 구조에는 맞지 않는 선택이였던 것 같습니다. 심지어 글을 잘못 이해한 것이였습니다.., 만약 다시 리펙토링한다면 CarGroup에서는 Position에 대한 계산만 하고, 출력부는 OutputView로 넘길 것 같습니다. |
||
for (Car car : cars) { | ||
String positionMarker = Symbol.POSITION_MARKER.getSymbol().repeat(car.getPosition()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. repeat 함수를 사용해 반복되는 문자를 깔끔하게 구현하셨네요! Java API 활용을 정말 잘하시는 것 같아 많이 배워갑니다. |
||
System.out.printf(IOMessage.CAR_POSITION_FORMAT.getText() + "%n", car.getName(), positionMarker); | ||
} | ||
} | ||
|
||
private int getMaxPosition() { | ||
return cars.stream() | ||
.mapToInt(Car::getPosition) | ||
.max() | ||
.orElse(0); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
package racingcar.view; | ||
|
||
import camp.nextstep.edu.missionutils.Console; | ||
import racingcar.common.IOMessage; | ||
import racingcar.common.Symbol; | ||
import racingcar.common.Validator; | ||
|
||
import java.util.Arrays; | ||
import java.util.List; | ||
|
||
public class InputView { | ||
|
||
public List<String> readCars() { | ||
System.out.println(IOMessage.INPUT_CAR_NAMES.getText()); | ||
List<String> carNames = tokenizeCarsName(Console.readLine()); | ||
Validator.validateCarNames(carNames); | ||
return carNames; | ||
} | ||
|
||
private List<String> tokenizeCarsName(String carNamesString) { | ||
return Arrays.stream(carNamesString.split(Symbol.SEPARATE_MARKER.getSymbol())) | ||
.map(String::trim) | ||
.toList(); | ||
} | ||
|
||
public int readRoundCount(){ | ||
System.out.println(IOMessage.INPUT_ROUND_COUNT.getText()); | ||
int num = Integer.parseInt(Console.readLine()); | ||
Validator.validateRoundCount(num); | ||
return num; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
package racingcar.view; | ||
|
||
import racingcar.common.IOMessage; | ||
import racingcar.domain.Car; | ||
import racingcar.domain.CarGroup; | ||
|
||
import java.util.List; | ||
import java.util.stream.Collectors; | ||
|
||
public class OutputView { | ||
public void printRoundResult(CarGroup carGroup) { | ||
System.out.println(IOMessage.RESULT_NOTICE.getText()); | ||
carGroup.printCarPositions(); | ||
System.out.println(); | ||
} | ||
|
||
public void printWinners(List<Car> winners) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 여기서도 Car 객체를 인자로 받기보다 원시 자료형 String의 리스트를 받는 게 어떨까 생각합니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이렇게 하면 의존성도 줄고, 테스트 하기도 편하겠네요! 감사합니다!! |
||
String winnerNames = winners.stream() | ||
.map(Car::getName) | ||
.collect(Collectors.joining(", ")); | ||
System.out.println("최종 우승자 : " + winnerNames); | ||
} | ||
|
||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
혹시 상수가 아닌 Enum 클래스를 사용한 이유가 따로 있을까요?
개인적인 생각으론 인자가 문자열 하나만 가지고 있다면, 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.
하나의 데이터를 저장하기에는 enum은 차지하는 크기가 static 상수에 비해 크기 때문에 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.
InputView와 OutputView 모두 기본 메시지(입력 멘트, 출력 멘트)가 필요하기 때문에 enum으로 한 곳에서 관리할 수 있다면 좋겠다고 생각했는데, 생각해보니까 enum 말고 static으로 띄우면 되는거더라고요🤣🤣 Limit에는 잘 적용을 해놨는데 제출을 하고 아 여기도 쓰면 되는구나 깨달았습니다... 집어주셔서 제가 실수한게 맞다는걸 다시 한번 깨달아서 좋네요ㅠㅠ
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.
그런데 상수 선언이라는 의미를 명확하게 드러내기 위해선 enum을 쓰는것도 좋은 것 같아요!!