-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
차한솔_11월20일_스터디용 코드 #2384
base: main
Are you sure you want to change the base?
차한솔_11월20일_스터디용 코드 #2384
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 List<Car> getWinners(List<Car> racingCars) { | ||
List<Car> winners = new ArrayList<>(); | ||
int maxLocation = findMaxLocation(racingCars); | ||
for (Car thisCar : racingCars) { | ||
if (thisCar.getLocation() == maxLocation) { | ||
winners.add(thisCar); | ||
} | ||
} | ||
return 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.
getLocation을 통해 maxLocation과 비교하는 것은 3주차 미션 피드백에 맞지 않아보여요!
https://tecoble.techcourse.co.kr/post/2020-04-28-ask-instead-of-getter/
를 참고해보시는건 어떤가요?
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 void validateName(String newCarName) { | ||
if (!(newCarName.length() <= CAR_NAME_MIN_SIZE.getNumber())) { | ||
throw new IllegalArgumentException(CAR_NAME_ERROR.getMessage()); | ||
} | ||
} |
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.
enum과 util 등 도움을 주기 위한 파일을 만드신게 좋은 코드인거 같습니다.
고생하셨습니다!
public List<Car> getCars() { | ||
return this.cars; | ||
} |
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.
이 getCars는 객체의 필드의 cars의 참조를 그대로 반환합니다. 이는 클래스 외부에서 cars 리스트를 수정할 수 있어 무결성을 위협할 수 있어 캡슐화에 위반 될 수 있습니다.
List는 불변이 아니므로 변경될 여지가 있어 위험할 수 있으니 이를 방지하기 위해 불변 리스트로 만들거나 리스트의 복사본을 제공하는 것으로 바꾸어 외부에서 수정할 수 없도록 하는게 어떨까 싶습니다.
@DisplayName("이름이 6자인 경우 예외 발생") | ||
@Test | ||
void nameLenTest() { | ||
assertThatThrownBy(() -> new Car("qwertt")) | ||
.isInstanceOf(IllegalArgumentException.class); | ||
} |
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.
테스트 케이스는 성공, 실패 모든 경우에 작성한 게 좋습니다.
우테코 피드백 3주차에 해당 내용이 있습니다
@DisplayName("전진 stop 메세지 테스트") | ||
@Test | ||
void racingNow() { | ||
RacingCars racingCars = new RacingCars(List.of("asdf", "qwer")); | ||
racingCars.getCars().get(0).move(5); | ||
|
||
outputView.racingNow(racingCars); | ||
assertThat(outMessage.toString()) | ||
.contains("asdf : -") | ||
.contains("qwer :"); | ||
|
||
} |
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.
입력과 출력 부분에 테스트 코드 작성 안 해도 된다고 본 거 같습니다.
3주차 과제인 Lotto에 추가된 요구 사항에 있습니다.
따끔한 리뷰 부탁드립니다. :b