-
Notifications
You must be signed in to change notification settings - Fork 452
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
[1단계 - 자동차 경주 구현] 몰리(김지민) 미션 제출합니다. #719
Conversation
- 이름의 길이 초과 시 예외 발생 - 이름들 중 중복 발생 시 예외 발생 - 이름 중 영어가 아닌 문자 포함 시 예외 발생
- 이동한 자동차가 없을 경우 출력 메서드 추가
- ValidatorTest 에 안쓰는 임포트문 제거
- 이동 내역 출력 시 줄바꿈 수정
- 이름 목록을 String[] 에서 List<String> 으로 변경
- throws IOException 제거 - 초기값 상수로 변경:
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.
안녕하세요 몰리 👋
코드가 굉장히 깔끔하시네요.
첫 미션 고생 많으셨고, 다음 단계에 코멘트 반영해주세요!
boolean nameInput = false; | ||
while (!nameInput) { | ||
nameInput = getCarNames(); |
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.
에러가 날 경우 다시 입력받기 위해서 flag를 사용해주셨네요. 재귀를 사용하는 방식보다 훨씬 안전한 것 같아요.
- nameInput이란 변수명이 어울리는지 고민해보셔도 좋을 것 같아요.
- 에러가 나면 그냥 에러가 나게 두는 것도 방법입니다. 특별한 요구사항이 없다면 에러가 나게 둬도 괜찮아보여요.
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.
안녕하세요 범블비 :) 리뷰 감사합니다🙏🙏
nameInput이라는 변수명은 충분히 고민해보지 못했던 것 같아요..!
더 적절한 네이밍을 고민해서 수정하도록 하겠습니다!
에러가 나게 두는 것도 방법..! 이에 대해서는 생각해보지 못했는데, 그럴수도 있겠네요!
앞으로 참고하겠습니다!
|
||
import java.util.List; | ||
|
||
public class Validator { |
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.
일단 프로그램 규모가 크지 않고 검증 로직도 많이 없어서 하나의 클래스에 모든 검증 로직을 두었는데,
검증해야 할 클래스의 수가 늘어나고, 검증에 대한 로직이 많아진다면 유지보수하기 힘들 것 같아요.
또 각 객체가 자신의 데이터에 대한 검증의 책임을 가지고 있는 것이 맞다는 생각이 들었습니다! 좋은 리뷰 감사합니다!
public class CarGroup { | ||
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.
일급컬렉션을 활용해주셨군요 😀
일급컬렉션을 활용하면 어떤 장점이 있을까요?
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 List<Car> findWinners() { | ||
int maxPosition = findMaxPosition(); | ||
return cars.stream() | ||
.filter((car -> car.getPosition() > 0 && car.getPosition() == 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.
지금 로직도 훌륭하지만 조금 더 극단적으로 getter를 사용하지 않는 훈련을 해봐도 좋을 것 같아요.
Car 객체에 적당한 책임을 부여하는 방식을 사용하면 getter를 사용하지 않아도 될 것 같아보여요.
두 Car 중에 winner가 누군지 찾는 로직, 두 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.
getter를 사용해 car의 필드값에 접근하는 것보다 isWinner 메서드를 사용해서 객체에 물어보거나 Comparble를 사용해 정렬하는 방식으로 변경해도 좋을 것 같다는 생각이 드네요!
참고해서 수정하겠습니다 :)
@Override | ||
public String toString() { | ||
StringBuilder sb = new StringBuilder(); | ||
for (Car car : cars) { | ||
sb.append(car.toString()); | ||
sb.append("\n"); | ||
} | ||
|
||
return sb.toString(); | ||
} |
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.
이 toString은 콘솔에서 출력하기 위해 사용되는 것 같아요.
출력을 위한 로직은 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.
이 부분도 놓친 부분이네요..
반영해서 수정하겠습니다🤣
List<String> names = winnerGroup.stream() | ||
.map(Car::getName) | ||
.toList(); | ||
|
||
String winnerNames = String.join(", ", 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.
java8의 여러 API에 익숙하신 것 같네요 👍
@DisplayName("생성된 난수가 이동 기준을 만족하지 않을 때 위치는 변하지 않는다.") | ||
void notMoveTest() { | ||
final Car car = new Car("몰리"); | ||
final int randomNumber = 2; |
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.
앗 이 부분은 놓친 것 같네요!
수정하겠습니다!
안녕하세요! 6기 백엔드 몰리입니다 :)
처음으로 페어를 진행하면서 제가 생각하지 못한 부분들에 대해 의견을 들어볼 수 있어서 좋았습니다~
동시에 의견을 공유하면서, 객체 도출이나 메서드 추출 등등에 대해 나만의 기준을 세우는 것이 중요하다를 많이 느낀 것 같아요.
리뷰 잘 부탁드립니다!