[FIX] 빵집 상세보기 리팩터링 #145
[FIX] 빵집 상세보기 리팩터링 #145
Conversation
| @@ -79,10 +79,10 @@ public BakeryDetailResponseDTO getBakeryDetail(Long memberId, Long bakeryId) { | |||
| bakeryRepository | |||
| .findById(bakeryId) | |||
| .orElseThrow(() -> new NotFoundException(ErrorType.NOT_FOUND_BAKERY_EXCEPTION)); | |||
| List<Menu> bakeryMenu = menuRepository.findAllByBakery(bakery); | |||
There was a problem hiding this comment.
매개변수 전달만을 위한 변수 선언
-> 변수를 선언한 코드가 더 가독성이 좋다면 변수 선언 하고 매개변수로 전달하는게 좋다고 생각합니다
getXXX() 이런식으로 값을 얻어오는 메서드의 경우 별다른 변수 선언 없이 바로 매개변수에 넣어줘도 가독성이 크게 나빠지지는 않는다고 생각해요~ 현재 코드로 봤을 땐 변경 전 코드가 (제기준) 더 좋아보이긴 합니다
| @@ -81,7 +82,7 @@ public BakeryDetailResponseDTO getBakeryDetail(Long memberId, Long bakeryId) { | |||
| List<Menu> bakeryMenu = menuRepository.findAllByBakery(bakery); | |||
| BreadTypeResponseDTO breadType = getBreadType(bakery); | |||
| boolean isBookMarked = isBookMarked(memberId, bakeryId); | |||
| List<MenuResponseDTO> menuList = new ArrayList<>(); | |||
| List<MenuResponseDTO> menuList = MenuMapper.INSTANCE.toMenuResponseDTOList(bakeryMenu); | |||
| String address = | |||
| bakery.getState() | |||
| + " " | |||
There was a problem hiding this comment.
요거 띄어쓰기 BLANK_SPACE 이런 식으로 상수화 처리 어떠실까욜
There was a problem hiding this comment.
추가로 address 가져오는 부분 함수로 빼는건 어떠실까욜 매개변수가 너무 많을까요??
There was a problem hiding this comment.
오옹 상수화는 생각도 못했네요 좋습니다! address 가져오는 부분도 함수로 빼는거 좋습니다!
| .phoneNumber(bakery.getPhoneNumber()) | ||
| .menuList(menuList) | ||
| .build(); | ||
| return BakeryMapper.INSTANCE.toBakeryDetailResponseDTO( |
| @@ -82,6 +82,14 @@ public BakeryDetailResponseDTO getBakeryDetail(Long memberId, Long bakeryId) { | |||
| BreadTypeResponseDTO breadType = getBreadType(bakery); | |||
There was a problem hiding this comment.
BreadTypeResponseDTO로 변환하는 매퍼 만들었던걸로 기억하는데 있으면 매퍼로 처리하는것도 괜찮아 보입니다~
There was a problem hiding this comment.
매퍼로 동일하게 처리하고 리팩토링하면서 다른 곳에서 쓰이면 같이 수정하겠습니다~
There was a problem hiding this comment.
제가 이미 쓰긴합니다 하암ㅋㅋ 매퍼 코드 있으니 함 생각해보세요~
| + " " | ||
| + bakery.getTown() | ||
| + " " | ||
| + bakery.getAddressRest(); | ||
|
|
||
| for (Menu menu : bakeryMenu) { |
There was a problem hiding this comment.
메뉴 리스트 가져오는 것도 함수로 빼도 괜찮을 것 같습니다~
There was a problem hiding this comment.
가장 최근 commit에 이친구도 Mapper로 처리했습니다 !!
🍞 PR 타입
🍞 반영 브랜치
🍞 변경 사항
🍞 테스트 결과
🍞 To Reviewer