Skip to content
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

[LBP] 송하은 자동차 경주 미션 1,2,3단계 제출 #89

Merged
merged 2 commits into from
Feb 16, 2025

Conversation

haeunsong
Copy link

@haeunsong haeunsong commented Feb 13, 2025

리뷰어: 방예혁
페어: 현정빈

질문사항

  1. 자동차 이름이 5글자를 넘어가면 예외를 발생시키고, 재입력할 수 있도록 코드를 짜고자 하였으나 indent(인덴트, 들여쓰기) depth를 2를 넘지 않도록 구현한다 요구사항을 지키면서 짜는 방법을 생각해내지 못하였습니다.

while(true) 와 try,catch 문을 사용하는 것을 떠올렸지만, 요구사항을 충족하지 못하여
다른 어떤 방법이 있는지 궁금합니다.

  1. 처음에는 하나의 자바 클래스 안에 대부분의 코드를 집어넣고, 이후에 리팩토링을 진행했는데 현재 코드의 구조에 대해 어떻게 생각하시는지요.

  2. 결과 출력은 잘 되지만, 코드 로직에 문제가 없는지 봐주시면 감사하겠습니다.

  3. 더 고려하고 생각해보아야할 사항도 알려주시면 감사하겠습니다.

감사합니다!

Copy link

@YehyeokBang YehyeokBang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 하은! 😄

이번 미션도 고생하셨습니다! 전체적으로 메서드명은 이해하기 쉬웠던 것 같아요. 그러나 그 메서드명에 맞는 기능을 구현했는가(추가로 하나의 역할만 수행하고 있는가)를 다시 확인해 보시면 좋을 것 같습니다!

자동차 이름이 5글자를 넘어가면 예외를 발생시키고, 재입력할 수 있도록 코드를 짜고자 하였으나 indent(인덴트, 들여쓰기) depth를 2를 넘지 않도록 구현한다 요구사항을 지키면서 짜는 방법을 생각해내지 못하였습니다.

  • 우선 기능 요구 사항을 완료한 이후에 인덴트와 같은 프로그래밍 요구 사항을 지키려고 하면 더 좋을 것 같습니다!
  • 메서드가 하나의 일만 수행하도록 작성하면, 경계가 뚜렷해지고, 재입력 요구 사항을 지킬 수 있을 것 같아요. 우선 메서드가 하나의 일만 하는지 검토하고, 재입력 기능을 구현해본 이후에 줄일 수 있는 방법을 찾아보면 더 빠르게 학습할 수 있을 것 같아요.

처음에는 하나의 자바 클래스 안에 대부분의 코드를 집어넣고, 이후에 리팩토링을 진행했는데 현재 코드의 구조에 대해 어떻게 생각하시는지요.

  • 서로 다른 클래스가 RacingGame 객체를 각자 가지고 있거나, InputView가 여러 객체를 가지고 있는 것이 변경에 취약할 것 같습니다.
  • 코드에는 정답이 없기 때문에 그렇게 작성한 이유가 되게 중요한 것 같아요. 각 객체가 어디까지 책임지고 수행해야 하는지 스스로의 기준을 세워보고, 이후 그 기준과 함께 다시 질문해 주시면 좋을 것 같아요.

요약

  • 해당 클래스(또는 메서드)가 어디까지 책임져야 하는지
  • 클래스의 선언된 필드가 꼭 그 객체가 가지고 있어야 하는 상태인지
  • 해당 구조로 구성하게 된 근거
  • 코드 컨벤션

Comment on lines 12 to 16
public void move(int randomValue){
if(randomValue >= MOVE_TRIGGER){
position++;
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move() 메서드는 하나의 정수를 받아서 4보다 큰 경우 position을 증가시키는 기능을 수행하는 것 같아요.
이때 매개변수로 들어오는 값이 randomValue 즉, 랜덤한 값이라는 것을 꼭 알아야 할까요?

하나의 정수만 받아서 조건에 충족하면 움직이는 기능만 수행하는 것은 어떨까요? (들어오는 값이 랜덤인지 아닌지 알 필요가 없도록요!)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로 if와 조건문 사이에 공백을 추가해주시면 좋을 것 같아요!

// 예시
if (조건) {
    // 코드
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리팩토링할 때 수정해보겠습니다!

Comment on lines 7 to 10
public Car(String name){
this.name = name;
this.position = 0;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public Car(String name){
this.name = name;
this.position = 0;
}
public Car(String name) {
this.name = name;
this.position = 0;
}

공백도 컨벤션입니다! IDE의 코드 정렬 단축키를 활용하면 좋을 것 같아요.

Comment on lines 15 to 27
public void inputCarNames() throws Exception {

System.out.println("경주할 자동차 이름을 입력하세요(이름은 쉼표(,)를 기준으로 구분).");
String inputValue = scanner.nextLine();
String[] carsName = inputValue.split(",");

validateName(carsName);

for (int i = 0; i < carsName.length; i++) {
cars.add(new Car(carsName[i]));
}
racingGame.setCars(cars);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사용자의 입력을 위해 문구를 출력하기, 표준 입출력을 활용하여 문자열 형태로 입력받기, 입력받은 값을 ,로 구분하기, 검증하기, Car 객체를 생성하여 자동차를 레이싱 게임에 등록하기

메서드명은 자동차 이름을 입력받는 것으로 보이지만, 위 모든 일을 수행하는 것 같아요. 어떻게 개선해볼 수 있을까요? 아래는 생각해볼 수 있도록 힌트를 작성해두었습니다!

  • 자동차 이름을 받는 것이지만, 자동차 객체를 만들고 등록하는 일까지 수행한다.
  • 사용자에게 입력을 받아서 ,로 나누는 것을 메서드로 분리해보기? 등

Comment on lines 5 to 9
public class InputView {

private final RacingGame racingGame;
private final Scanner scanner = new Scanner(System.in);
private final List<Car> cars = new ArrayList<>();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InputView에서 RacingGame과 Car 목록을 필드로 가지고 있어야 하는 이유가 있나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InputView 는 클래스 이름처럼 입력을 담당하는 역할만 수행하도록 리팩토링하겠습니다!

Comment on lines 29 to 40
public void validateName(String[] carsName) throws Exception {
for (String carName : carsName) {
// 자동차 이름 검증 (5자 이하)
validateInput(carName);
}
}

public void validateInput(String carName) throws Exception {
if (carName.length() > 5) {
throw new Exception("자동차 이름은 5자 이하여야합니다.");
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이름 길이의 규칙이 5에서 6으로 변경된다면 코드에서 수정해야 하는 부분이 3곳(주석, 조건문, 예외 메시지)이나 있는 것 같아요. 특히 수정하다가 빼먹은 경우 원하는대로 동작하지 않을 수 있겠네요. 어떻게 바꿔볼 수 있을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private static final int MAX_NAME_LENGTH = 5; 이런식으로 최대 이름 길이를 상수로 두는 방법이 있군요. 리뷰 감사합니다!

Comment on lines 13 to 19
public void gameStart(int rounds) {
Random random = new Random();
while (rounds-- > 0) {
moveCars(random);
System.out.println();
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

라운드마다 moveCars() 메서드에게 random 객체를 전달하는 것 같아요. 더욱 직관적으로 랜덤 객체가 아닌 랜덤한 값을 넘기는 것은 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리팩토링에 반영해보겠습니다!

Comment on lines 31 to 35
public List<Car> getWinner() {
int maxPosition = cars.stream().mapToInt(Car::getPosition).max().orElse(0);
winnerCars = cars.stream().filter(car -> car.getPosition() == maxPosition).collect(Collectors.toList());
return winnerCars;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • .이 너무 많고 길어져서 한번에 이해하기 힘든 것 같아요. 메서드로 분리하거나 개행을 추가하는 것은 어떨까요?
  • collect(Collectors.toList())로 반환된 리스트는 변경 가능할까요? 불변일까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

collect(Collectors.toList())로 반환된 리스트는 변경 가능합니다. Collectors.toUnmodifiableList()로 불변 리스트로 사용하는 방법도 있군요! 감사합니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

또는, collect() 대신 toList()도 사용 가능해요!

public String findWinnerName() {
return winnerCars.stream().map(car -> car.getName()).collect(Collectors.joining(", "));
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

해당 클래스 파일의 가장 아래에는 공백을 추가해 주면 이 표시가 사라져요 이에 대해서 검색해보시면 좋을 것 같아요! eof 관련 포스팅

Comment on lines 3 to 11
public static void main(String[] args) throws Exception {
RacingGame racingGame = new RacingGame();
InputView inputView = new InputView(racingGame);
ResultView resultView = new ResultView(racingGame);

inputView.inputCarNames();
int rounds = inputView.inputRaceRounds();
resultView.printView(rounds);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inputViewinputCarNames() 메서드를 호출하지만 내부적으로 모든 로직을 수행하기 때문에 어색한 것 같아요.

InputView라는 이름에 맞게 입력에만 집중할 수 있게 하면 어떨까요? (예: view에서 입력을 받아 값을 반환하도록 하고, 다른 곳에서 그 값을 사용할 수 있는 흐름)

Comment on lines 1 to 4
public class ResultView {

private final RacingGame racingGame;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RacingGame 객체가 여러 곳에 있는 것 같아요. 어떤 문제가 발생할 수 있을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. RacingGame 의 필드인 cars 리스트가 여러 곳에서 동시에 수정될 가능성이 있습니다. 이는 객체지향 프로그래밍의 캡슐화를 위반하는 사항입니다.
  2. RacingGame 이 여러 클래스와 강하게 결합되어 있기에, 하나의 로직을 변경하면 모든 곳에서 해당 변경을 반영해야합니다. 더불어 버그 수정도 어려워집니다.

Copy link

@YehyeokBang YehyeokBang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다!! 😄

몇몇 수정되지 않은 부분은 4단계에서 함께 작업하시면 될 것 같습니다! 또한, 유의미한 코드 리뷰를 위해, 납득이 된 부분은 바로 수정하고, 그렇지 않은 부분은 다른 근거를 기반으로 논의하면 더 얻어가는 것이 많을 것 같아요. 납득했는데도 바꾸지 않는다면, 논리적으로 맞지 않는 말일 테니까요! 😄

다음 단계도 잘 부탁드립니다~ 👍 👍

@boorownie boorownie merged commit 3cf2b02 into next-step:haeunsong Feb 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants