Conversation
junslog
left a comment
junslog
left a comment
There was a problem hiding this comment.
고생하셨습니다 ㅎ
코드에 대한 제 생각 남겨봤어요.
함수형 인터페이스를 통한 입력 및 이벤트 도메인 구성이 인상깊었습니다.
| Reservation reservation = Reservation.of(date, orders); | ||
| MatchingEvents matchingEvents = reservation.createMatchingEvents(); | ||
| ResultDto resultDto = ResultDto.of(orders, matchingEvents); | ||
| outputView.printResult(resultDto); |
There was a problem hiding this comment.
view 메시지가 출력될 떄마다 개행을 해주면 좀 더 읽기 쉬워질 것 같아요,
메서드명과 클래스 이름이 직관적이지만 아무래도 몰려있다보니 프로그램의 흐름이 한눈에 들어오지 않습니다
There was a problem hiding this comment.
개행해주셔두 좋구요, 아니면 View와 Model을 유의미한 단위로 묶어서 메서드 추출해주셔도 좋을 것 같네요 👍👍
There was a problem hiding this comment.
개행해주셔두 좋구요, 아니면 View와 Model을 유의미한 단위로 묶어서 메서드 추출해주셔도 좋을 것 같네요 👍👍
시간이 없어서 이부분 리팩터링은 생각도 못하고 구현에만 집중했던 것 같습니다!
뷰와 모델을 유의미한 단위로 묶어서 메서드 추출 !! 해보겠습니다 :)
| } | ||
|
|
||
| private boolean isEqualName(String name) { | ||
| return this.name().equals(name); |
There was a problem hiding this comment.
MenuType을 인자로 받아서, this.name == other.name 과 같은 식으로 비교한다면,
MenuType의 name에 대한 getter의 사용을 줄일 수 있을 것 같습니다.
There was a problem hiding this comment.
이 메서드가 OrderItem 생성 시(OrderItem.of())에만 사용되는데
이 생성자는 MainController 에서만 사용되고, 사용자로부터 메뉴 이름을 받아와서 그 이름을 바로 넣게 되고 따로 getName()을 호출하지 않는 상태여서요
혹시 이런 경우에도 말씀하신 것처럼 수정이 필요하려나요?
public static OrderItem of(String menuName, int quantity) {
validateQuantity(quantity);
Menu menu = Menu.findMenuByName(menuName);
return new OrderItem(menu, quantity);
}
//mainController
private Orders createOrders() {
return readUserInput(() -> {
List<OrderItemDto> orderItemDtos = inputView.readOrderItemDtos();
List<OrderItem> orderItems = orderItemDtos.stream()
.map(dto -> OrderItem.**of**(dto.getName(), dto.getQuantity()))
.toList();
return Orders.from(orderItems);
});
}
| private final MatchingEventsDto matchingEventsDto; | ||
| private final long totalBenefitAmount; | ||
| private final long finalAmount; | ||
| private final String badgeName; |
There was a problem hiding this comment.
너무 많은 인스턴스 변수를 가지고 있으니,
해당 dto를 쪼개는 것도 좋을 것 같습니다.
junpakPark
left a comment
junpakPark
left a comment
There was a problem hiding this comment.
안녕하세요 지수님! 5기 준팍입니다.
프리코스가 끝났는데도 열정적이시군요 👍👍👍
지수님 덕분에 다른 분들도 추가로 리뷰할 마음이 생겼습니다.
그분들은 지수님께 감사해야할지도...?
리뷰로 돌아와서... 어떤 식으로 리뷰를 해야할까 고민 많이했습니다
제한된 시간내에 학습하신 내용을 잘 적용해주셨으니 칭찬 위주로 가야할까
아니면 프리코스가 끝나도 리뷰를 요청하실 만큼 열정적이신데
풀파워로 리뷰를 해야할까...
하다가 후자로 갔습니다.
혹시 리뷰가 너무 날카롭다면 미리 사과드립니다...
저는 프리코스 때 5시간만에 이런 코드 못짰을 것 같아요...
굉장히 잘 하셨는데 그래서 하나라도 더 알려드리고 싶은 마음에서
쓴 리뷰들이니까요 ㅎㅎ 잘한 점도 많았는데 하나라도 더 알려드리고 싶어서
잘한건 따로 언급 안했구나~ 그렇게 생각해주시면 감사하겠습니다 ㅎㅎ
그리고 제 리뷰를 다 반영하셨다면
언제든지 질문 혹은 추가적인 피드백 요청 주셔도 되니까요
너무 부담갖지마시구 재밌게 코딩하셨으면 좋겠습니다 ㅎㅎ
(혹시 설계 자체를 뒤엎으실 경우엔 별도의 PR 주소로 올리신 다음에 DM 보내주시면 리뷰하겠습니다 ㅎㅎ)
수고많으셨습니다!
| this.outputView = outputView; | ||
| } | ||
|
|
||
| public static MainController create() { |
There was a problem hiding this comment.
정적 팩터리 메서드를 쓰셨군요?
그리고 View의 클래스들은 싱글턴으로 만들어주셨구요
이렇게 구현했을 때, 어떤 장점이 있어서 이렇게 구현해주셨나요?
만약 예상 되는 문제점이 있다면 어떤것일까요?
There was a problem hiding this comment.
뷰는 필요한 데이터만 받아서 생성해주는 역할이라 객체가 매번 생성될 필요가 없다고 생각해서 싱글톤으로 만들었고,
정적 팩터리 메서드는 new로 생성하는 것보다 메서드에 이름이 있으면 나중에 찾기 좋을 것 같아 사용하였습니다,,
그렇지만 InputView, OutputView 를 MainController에서 직접 호출하기에 나중에 뷰 관련 구체 클래스 변경이 필요할 때 MainController 코드의 수정이 필요하여 OCP를 지키지 못한 것 같습니다.
어차피 외부에서 받아오는 매개변수가 필요하지 않은 지금과 같은 상황에서는 정적 팩터리 메서드를 사용하지 않아도 되었을 것 같다는 생각을 지금... 했습니다.
제 생각이 어떤지요..?? 준팍님(@junpakPark)의 생각이 궁금합니다!!
There was a problem hiding this comment.
좋은 답변입니당 👍👍
지금 생각하신 거 코드에 적용하신 다음에 리뷰 요청 보내시면
피드백 드리겠습니다 ㅎㅎ
| Reservation reservation = Reservation.of(date, orders); | ||
| MatchingEvents matchingEvents = reservation.createMatchingEvents(); | ||
| ResultDto resultDto = ResultDto.of(orders, matchingEvents); | ||
| outputView.printResult(resultDto); |
There was a problem hiding this comment.
개행해주셔두 좋구요, 아니면 View와 Model을 유의미한 단위로 묶어서 메서드 추출해주셔도 좋을 것 같네요 👍👍
| OrderItemValidator.validatePositive(quantity); | ||
| OrderItemValidator.validateSize(quantity); |
There was a problem hiding this comment.
모든 원시값과 문자열을 포장(wrap)한다.
이런 경우엔 Validator를 두기보다는 Quantity를 VO로 만들어주는 게 어떨까요?
There was a problem hiding this comment.
오 VO 사용을 잘 못하였는데 감사합니다!! 적용해볼게요 :)
|
날카로운 리뷰 사랑합니다 ❤︎
아니아니,,, 저 정말 혹시나 싶어서 댓글 달았는데 리뷰해주신다 하셔서 저 뛸듯이 기뻤습니다! 설계를 바꿀 예정이라,, 바꾼 후 다시 DM 드리겠습니다!! 저 리뷰 내용과 별개로 질문이 생겼습니다.. 혹시,,, 어떤 방향성으로 공부해가는 것이 좋을까요? |
|
음... 우테코 커리큘럼을 기준으로 말씀드리면
이런 순서로 갑니다. 이걸 기준으로 말씀드리자면,
5기 프로젝트 중 코드리뷰 사이트가 있는데요 하고 계신 미션에서 배울만한 거 다 배우셨다면 |
4 participants
거의 5시간 소요