Skip to content

[자동차 경주] 김난슬 미션 제출합니다.#9

Open
seulnan wants to merge 6 commits intoJava-JavaScript-Language-Stuty:mainfrom
seulnan:seulnan
Open

[자동차 경주] 김난슬 미션 제출합니다.#9
seulnan wants to merge 6 commits intoJava-JavaScript-Language-Stuty:mainfrom
seulnan:seulnan

Conversation

@seulnan
Copy link
Copy Markdown

@seulnan seulnan commented Feb 14, 2025

리뷰하러 와주셔서 감사합니다 ^0^ 같이 파이팅해봐요 부릉부릉 🚗 🚗

🌟 내가 신경써서 구현한 부분

  • 저번보다 service나 util, validator등으로 분리하여 책임을 분리하고자 했습니다.
  • model로 분리하는것보다 domain을 활용하는 것이 "객체가 자신의 행동을 스스로 수행하는 것"에 맞춰서 더 객체지향적이라고 생각했습니다.
  • DI (의존성 주입)을 활용하여 테스트 가능성을 높이고, mock을 활용하려했으나...이번주에 개인사정이슈로 너무 바빠서ㅠㅠㅠ리팩토링때 진행할 에정입니다.

❤️‍🔥 피드백이 필요한 부분

🚨🚨 이번주에...제가...너무..바빠서..코드퀄리티가 좀 떨어져도..최선을 다했습니다...이쁘게 봐주세요..

  • 책임 분리를 해야하는 것이 좀 많아보이는데 같이 짚어주시면 감사하겠습니다.
  • 무조건 stream API를 사용하는 것보다 간단한 반복은 forEach를 사용하는 것이 더 가독성이 좋은 것 같은데 의견 남겨주시면 좋을 것 같아요😄😄
  • 테스트 커버리지 확장이 충분한 지 확인 부탁드립니다. (추가적으로 검증해야 할 시나리오가 있는지)

🔍 내가 고려한 예외 케이스

검증 항목 고려한 예외
자동차 이름 입력 빈 값 입력 시 예외 발생
중복된 자동차 이름 입력 시 예외 발생
자동차 이름이 5자 초과일 경우 예외 발생
자동차 이름에 특수문자가 포함된 경우 예외 발생
시도 횟수 입력 입력값이 숫자가 아닐 경우 예외 발생
시도 횟수가 0 이하일 경우 예외 발생

Added a list of features to be implemented in README.md.
This serves as a roadmap for the upcoming development tasks.
  - 입력값을 InputValidator를 통해 검증하여 유효성을 체크.
  - trim()을 사용해 불필요한 공백 제거
- 자동차 이름 검증
    - 입력값이 비어있는 경우
    - 자동차 이름이 중복되지 않도록
    - 자동차 이름이 5자 이하인지 검증

- 시도 횟수 검증
    - 숫자가 아닌 경우
    - 입력값이 1 이상인지 검증 (0 이하인 경우 예외 발생)
- 모델 대신 서비스와 도메인으로 로직을 분리
  - service: 게임 전체 흐름 관리
  - domain: car & race 객체가 데이터+동작이 포함되게하여 개별적으로 작동하게 구현
- util
  - 자동차가 움직일 확률 결정하는 랜덤 숫자 생성기 구현
- DI 적용 및 의존성 주입
- 중복된 코드 및 가독성이 떨어지는 로직을 별도 메서드로 분리
- 자동차 이름이 영어 또는 숫자가 아닐 경우 예외 추가
- 시도 횟수가 숫자가 아닐 경우 예외 추가
- 유닛 테스트 및 통합 테스트 코드 추가
Copy link
Copy Markdown

@Regyung Regyung left a comment

Choose a reason for hiding this comment

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

리뷰를 하다 보니 전부 질문 글이네요 허허... 처음 보는 이름이나 기법(?)이 많이 보여서 전부 공부할 리스트에 넣었습니다. 코드 너무 멋졌어요!


public void printRaceStatus(List<Car> cars) {
String raceStatus = cars.stream()
.map(car -> car.getName() + " : " + "-".repeat(Math.max(0, car.getPosition())))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

'-'를 반복할 때 그냥 getPosition()을 하는 게 아니라 Math.max(0, car.getPosition())을 하신 이유가 있으신가요?

Copy link
Copy Markdown
Author

@seulnan seulnan Feb 17, 2025

Choose a reason for hiding this comment

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

음..그저..범위를 제한설정해둬서 getPosition값이 음수일 경우라면 0으로 처리되게 하기위한 안전장치로 심어뒀습니다 @Regyung

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

음수일 경우를 신경써주신 이유가 따로 있으실까요?

Copy link
Copy Markdown
Author

@seulnan seulnan Feb 18, 2025

Choose a reason for hiding this comment

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

물론 음수일 경우를 신경쓴 의도가 있긴하지만, 정확히는 0부터 position값이 시작한다는 걸 명시화함으로써 가독성을 올리려는 목적이었습니다! @szoon2426

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

랜덤값을 반환하는 파일을 따로 만드신 이유가 뭔가요?

Copy link
Copy Markdown
Author

@seulnan seulnan Feb 17, 2025

Choose a reason for hiding this comment

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

앗 책임분리를 하기위해서였습니다. (SRP적용) 랜덤값 생성은 자동차객체에 영향을 주는 것도 아니고, 레이싱 결과자체에 직접적인 영향을 주는것도 아닌 과정중의 하나라고 생각했습니다. 그래서 util로 분리하면 이 로직에 대한 테스트코드도 작성이 가능한데 시간부족이슈로 못했습니다.. @Regyung

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

에러 메세지를 Enum을 이용해서 정리하고 상수로 표현하는 것도 괜찮아 보여요!

throw new IllegalArgumentException("[ERROR] 시도 횟수는 1 이상이어야 합니다.");
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

이렇게 예외케이스 메소드를 구분해서 하니까 가독성이 좋은 것 같아요

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

동의합니다. 많은 예외를 고려하고, 이를 가독성있는 코드로 작성하는 것이 대단하네요!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

저는 예외를 많이 고려하지 않았는데 이것 또한 많이 배워갑니다!...

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

자동차 이름 중복은 생각 못했었는데 또 배워갑니다..!

public int getAttemptCount() {
System.out.println("시도할 회수는 몇회인가요?");
String input = Console.readLine();
InputValidator.validateAttemptCount(input);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

validator를 만든게 이렇게 가독성을 올려주네요 ㄷㄷ

}

public void moveCars() {
cars.forEach(Car::move);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

확실히 스트림보단 보기 깔끔하네요
스트림은 가공, 변환할 때 쓰는 용도이니 단순 반복이라면 forEach를 쓰는게 좋아보여요

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

forEach를 처음 봐서 어떤건지 찾아봤는데 for문 보다 간단한 것 같아요! 새로운 것을 배워가요 감사합니다:)

.filter(car -> car.getPosition() == maxPosition)
.map(Car::getName)
.collect(Collectors.toList());
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

저는 for문 돌려서 maxPosition이랑 같은 값은 갖는 car를 찾았는데 filter로 간편하게 구할 수 있군요.. 배워갑니다!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

저도 배워갑니다!..


public void printRaceStatus(List<Car> cars) {
String raceStatus = cars.stream()
.map(car -> car.getName() + " : " + "-".repeat(Math.max(0, car.getPosition())))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

음수일 경우를 신경써주신 이유가 따로 있으실까요?

public class RandomNumberGenerator {
private static final int MIN = 0;
private static final int MAX = 9;
private static final int MOVE_THRESHOLD = 4;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MOVE_THRESHOLD를 변수로 만들어서 직관적으로 판단하기 더 좋아진 것 같습니다! 난슬님 코드를 처음 분석해봤는데 정말 많이 배워갑니다..

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

동의합니다. static과 final의 쓰임새도 이해해가고 있어요. 코드가 직관적이어 좋네요!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

저도 상수화를 해서 보완을 해야겠네요! 저는 0, 9로만 하였는데 배워갑니다!...

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

저도 0,9로만 했었는데 배워가요! final도 처음 보는데 다음 미션때 필요하다면 써봐야겠어요!!

Copy link
Copy Markdown

@KIM-GOING KIM-GOING left a comment

Choose a reason for hiding this comment

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

코드가 압도적으로 가독성있게 들어오네요... 곧 따라잡겠습니다. 수고하셨어요!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

도메인과 모델의 차이가 뭘까요.. MVC를 공부하며 모델을 이해했는데 도메인이 같은 역할로 사용되고 있는 거 같아서요..!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

저도 구현하면서 공부했는데 주 포인트는 비즈니스 로직의 포함여부입니다! 도메인 객체는 비즈니스 로직이 포함되어있는 일종의 개념단위?로 보시면 될 것 같아요! @KIM-GOING

모델 대신 도메인 객체를 사용한 이유는 자동차 객체 자체에 자동차의 이동도 같이포함시켜야 객체의 더 책임도 명확해지고, 유지보수도 쉬워질거라 판단했습니다. 물론 DTO방식으로 단순한 모델로 사용해도 되지만, 그럼 핵심동작인 이동같은 로직을 service에서 따로 처리해야하므로 자동차객체가 스스로 움직일 수 없어서 오히려 불편할 것 같다고 생각했습니다.

도메인, 도메인 객체, 모델 차이

public class RandomNumberGenerator {
private static final int MIN = 0;
private static final int MAX = 9;
private static final int MOVE_THRESHOLD = 4;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

동의합니다. static과 final의 쓰임새도 이해해가고 있어요. 코드가 직관적이어 좋네요!

throw new IllegalArgumentException("[ERROR] 시도 횟수는 1 이상이어야 합니다.");
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

동의합니다. 많은 예외를 고려하고, 이를 가독성있는 코드로 작성하는 것이 대단하네요!

Copy link
Copy Markdown

@piamin04 piamin04 left a comment

Choose a reason for hiding this comment

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

난슬님 코드를 처음리뷰해보는데 너무.. 대단하신 것 같아요..! 리뷰라기 보단 모르는 것 찾고 배우기만 한거같아요..! 난슬님 코드를 더 뜯어보면서 공부해봐야할것같아요! 감사합니다:)

}

public void moveCars() {
cars.forEach(Car::move);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

forEach를 처음 봐서 어떤건지 찾아봤는데 for문 보다 간단한 것 같아요! 새로운 것을 배워가요 감사합니다:)

public class RandomNumberGenerator {
private static final int MIN = 0;
private static final int MAX = 9;
private static final int MOVE_THRESHOLD = 4;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

저도 0,9로만 했었는데 배워가요! final도 처음 보는데 다음 미션때 필요하다면 써봐야겠어요!!

throw new IllegalArgumentException("[ERROR] 시도 횟수는 1 이상이어야 합니다.");
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

자동차 이름 중복은 생각 못했었는데 또 배워갑니다..!

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.

7 participants