-
Notifications
You must be signed in to change notification settings - Fork 4
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
[자동차 경주] 이원희 미션 제출합니다. #3
base: main
Are you sure you want to change the base?
[자동차 경주] 이원희 미션 제출합니다. #3
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.
고생하셨습니다아아앗
public void printRoundResult(){ | ||
System.out.print(name.trim() + " : "); | ||
for(int i = 0; i < position; i++){ | ||
System.out.print("-"); | ||
} | ||
System.out.println(); | ||
} |
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의 영역이다 보니까, 이 부분에서는 필드인 name과 position의 값을 중점으로 반환하는 게 더 좋을 듯 합니다!
Map<String, Integer> 타입이나 따로 DTO를 만들어서 반환한 후, View 쪽에서 각 값들을 조립하여 출력 메시지를 만드는 게 더 좋은 구조일 것 같습니다...!
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.
이 부분은 취향차이일 수도 있는데, Name의 검증과 관련된 부분은 Car 객체를 생성하는 과정에 넣는 게 더 좋지 않나 싶습니다!
그럴 일은 잘 없겠지만 Controller에서 NameValid를 사용하는 것을 까먹고 바로 Car를 생성할 수도 있는 거고, Car에 해당 검증 로직을 넣으면 원터치로 검증이 가능하기 때문에 좋아보입니다.
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.
Validation은 Controller에서 제외하는 것이 좋다는 것에 동의합니다. Controller는 Request를 넘겨주는 entry point의 역할만 수행하도록 만드는게 좋다고 생각합니다.
import static racingcar.view.ResultView.printWinner; | ||
|
||
public class RacingCar { | ||
private static final ArrayList<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.
static 키워드를 붙이면 멀티쓰레드 환경에서 필드의 일관성이 보장되지 않기 때문에 당장 생각할 문제는 아니지만 여러 명이 게임을 플레이한다고 가정했을 때 문제가 될 수 있을 것 같습니다! 제거를 고려해보시면 좋을 것 같아유
int maxPosition = 0; | ||
for (Car car : cars) { | ||
maxPosition = Math.max(maxPosition, car.getPosition()); | ||
} |
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.
별 거 아니긴 한데 이 부분도 최댓값을 구하는 기능의 메서드로 분리하면 더 좋을 것 같습니다!
public static void moveCars() { | ||
for (Car car : cars) { | ||
car.move(); | ||
car.printRoundResult(); |
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.printRoundResult()
도 View의 역할이기 때문에 자동차가 움직인 결과를 DTO 등을 통해 View에게 출력하도록 책임을 넘겨주는 것도 생각해보시면 좋을 것 같습니다.
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.
추가 리뷰
import java.util.Arrays; | ||
|
||
public class NameValid { | ||
final ArrayList<String> names; |
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.
리스트의 불변성 관련 검색 키워드
- unmodifiableList
- (를 이기는) 방어적 복사
for (int i = 0; i < position; i++) { | ||
System.out.print("-"); | ||
} |
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.
-> System.out.print("-".repeat(position));
으로 대체 가능!
public static void startGame() { | ||
InputController.createRacingCar(); | ||
int round = InputController.setRoundNum(); | ||
ResultView.printResult(); | ||
for (int i = 0; i < round; i++) { | ||
RacingCar.moveCars(); | ||
System.out.println(); | ||
} | ||
RacingCar.getWinners(); | ||
} |
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.
Controller 객체를 만들지 않고 Application.java에서 바로 시작한 이유가 있나요?
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.
Validation은 Controller에서 제외하는 것이 좋다는 것에 동의합니다. Controller는 Request를 넘겨주는 entry point의 역할만 수행하도록 만드는게 좋다고 생각합니다.
// 자동차들을 이동시킨다. | ||
public static void moveCars() { | ||
for (Car car : cars) { | ||
car.move(); | ||
ResultView.printRoundResult(car.getName(), car.getPosition()); | ||
} | ||
} | ||
|
||
// 우승자를 반환한다. | ||
public static void getWinners() { | ||
ArrayList<String> winners = new ArrayList<>(); | ||
int maxPosition = getMaxPosition(); | ||
for (Car car : cars) { | ||
if (car.getPosition() == maxPosition) { | ||
winners.add(car.getName()); | ||
} | ||
} | ||
ResultView.printWinner(winners); | ||
} |
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.
Model에서 View를 호출하지 않고 Controller에서 결합하도록 하는 것이 좋다고 생각합니다!
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.
반영하겠습니다~
// 라운드 결과 출력 | ||
public static void printRoundResult(String name, int position) { | ||
System.out.print(name + " : "); | ||
System.out.print("-".repeat(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.
repeat 메서드를 사용하는 방법이 있다는 것을 처음 알았네요! 좋은 방법 같습니다
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.
수고하셨습니다!
} | ||
} | ||
} | ||
} |
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.
감사합니당~
// 최대 이동값 | ||
private static int getMaxPosition() { | ||
int maxPosition = 0; | ||
for (Car car : cars) { | ||
maxPosition = Math.max(maxPosition, car.getPosition()); | ||
} | ||
return maxPosition; | ||
} | ||
} |
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.
감사합니당~
내 코드(라고 쓰고 파일명이라 읽는다)를 소개합니다 ✨
(커밋을 자주 안해서 커밋 로그가 🐕판일 수 있어요.)
(1학년인데 뉴비 베네핏 적용해주세요)