Uh oh!
There was an error while loading. Please reload this page.
🚀 2단계 - 수강신청(도메인 모델) - #715
Conversation
wooobo
left a comment
There was a problem hiding this comment.
안녕하세요!
2단계도 잘진행해주셨습니다 😄
몇가지 코멘트 남겨놓았어요~
확인 후 재요청 부탁드립니다!
그럼 화이팅입니다!
| this.sessionCapacity = sessionCapacity; | ||
| } | ||
| public Enrollment(SessionCapacity sessionCapacity) { |
There was a problem hiding this comment.
PR요청시 사용하지 않는 메소드는 과감히 제거하는것이 좋다고생각합니다.
혹시 추후 참고해야되는 코드라면, 적절한 commit메시지로 남겨 놓는것도 하나의 방법일것 같습니다 🤔
| } | ||
| private boolean canApply(LocalDate enrollDate, Payment payment) { | ||
| if (!state.canRecruit()) { |
There was a problem hiding this comment.
| if (!state.canRecruit()) { | |
| if (state.cannotRecruit()) { |
!표현 보다는 있는 그대로 메소드명으로 표현하는게 가독성에 좋을수도 있다고생각되네요 🤔
| public void increase() { | ||
| capacity++; | ||
| isValidCapacity(); | ||
| } |
There was a problem hiding this comment.
| publicvoidincrease() { | |
| capacity++; | |
| isValidCapacity(); | |
| } | |
| publicvoidincrease() { | |
| capacity++; | |
| isValidCapacity(); | |
| } |
해당 코드를 보고 의문점이 드는게 한가지 있는데요,
try {
SessionCapacitysessionCapacity = newSessionCapacity(10, 5);
sessionCapacity.increase();
} catch (CannotEnrollExceptione) {
System.out.println(e.getMessage());
}
// blabla위 테스트 코드는 10어야 할까요 11이어야할까요?
그렇다면 무엇이 문제일까요?
publicvoidincrease() {
isValidCapacity();
capacity++;
}위치를 조정해보거나, capacity 를 불변객체로 관점 전환 후 해결해볼수도 있을겄같네요 🤔
| private void checkValidSessionImage() { | ||
| if (this.width < MIN_WIDTH || this.height < MIN_HEIGHT) { | ||
| throw new InvalidImageException("이미지의 width는 300픽셀, height는 200픽셀 이상이어야 한다."); |
There was a problem hiding this comment.
조건이 변경될때마다 예외 메시지까지 함께 변경해야하는것은 유지보수시 리스크가 있지 않을까요?
| @@ -0,0 +1,11 @@ | |||
| package nextstep.courses; | |||
| public class InvalidImageException extends RuntimeException { | |||
There was a problem hiding this comment.
단순히 메시지만 받는것이라면,
java 기본 excpetion으로 사용할수 있었을 텐데,
custom exception을 만든 이유가 무엇인지 궁금합니다!
There was a problem hiding this comment.
현재 도메인별로 custom exception으로 예외처리를 하여 이와 같게 진행하였습니다..!
| this.startDate = startDate; | ||
| this.endDate = endDate; |
There was a problem hiding this comment.
endDate가 startDate보다 이전이면 객체가 생성안되게 하는게 더 확실하지 않을까요? 아니면, endDate가 startDate보다 이전이라도 생성되어야하는 경우가 있나요?
| if (payment.isSameAmount(price)) | ||
| return true; | ||
| return false; |
There was a problem hiding this comment.
| if (payment.isSameAmount(price)) | |
| returntrue; | |
| returnfalse; | |
| returnpayment.isSameAmount(price); |
쉽게 표현할수있는것은 쉽게 표현해도 좋을것 같아요~
| FreePaymentStrategy freePaymentStrategy = new FreePaymentStrategy(); | ||
| assertTrue(freePaymentStrategy.payable(new Payment())); | ||
| } | ||
There was a problem hiding this comment.
무료일경우 결제금액이 있는 Payment가 입력되어도 수행되는게 안전할까요?
@Testpublicvoidfree2(){
FreePaymentStrategyfreePaymentStrategy = newFreePaymentStrategy();
assertTrue(freePaymentStrategy.payable(newPayment("1", 1L, 1L, 1000L)));
}There was a problem hiding this comment.
FreePaymentStrategy에서는 모두 true를 반환하기 때문에 괜찮다고 생각하였습니다!
| - [x] 강의는 시작일과 종료일을 가진다. | ||
| - [x] 강의는 강의 커버 이미지 정보를 가진다. | ||
| - [ ] 강의는 무료 강의와 유료 강의로 나뉜다. | ||
| - [ ] 무료 강의는 최대 수강 인원 제한이 없다. |
안녕하세요. 이번 단계 리뷰 잘부탁드립니다.
감사합니다!