Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[FIX] 빵집 상세보기 리팩터링 #145

Merged
merged 4 commits into from
Aug 14, 2023
Merged

[FIX] 빵집 상세보기 리팩터링 #145

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
85ec9f4
[FIX] BakeryMapper를 사용하여 toBakeryDetailResponseDTO를 통한 리팩토링 구현해라
sung-silver Aug 14, 2023
7aba394
[FIX] MenuMapper를 작성하고 BakeryService 내 getBakeryDetail 로직에 적용해라
sung-silver Aug 14, 2023
2f2a4ef
[FIX] 매개변수 전달을 위한 단순 변수 선언으로 생각되는 로직 생략해라
sung-silver Aug 14, 2023
a3f906d
[FIX] 코드리뷰 반영
sung-silver Aug 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions api/src/main/java/com/org/gunbbang/service/BakeryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

매개변수 전달만을 위한 변수 선언
-> 변수를 선언한 코드가 더 가독성이 좋다면 변수 선언 하고 매개변수로 전달하는게 좋다고 생각합니다
getXXX() 이런식으로 값을 얻어오는 메서드의 경우 별다른 변수 선언 없이 바로 매개변수에 넣어줘도 가독성이 크게 나빠지지는 않는다고 생각해요~ 현재 코드로 봤을 땐 변경 전 코드가 (제기준) 더 좋아보이긴 합니다

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

참고하겠습니다 감사티비!!

BreadTypeResponseDTO breadType = getBreadType(bakery);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BreadTypeResponseDTO로 변환하는 매퍼 만들었던걸로 기억하는데 있으면 매퍼로 처리하는것도 괜찮아 보입니다~

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

매퍼로 동일하게 처리하고 리팩토링하면서 다른 곳에서 쓰이면 같이 수정하겠습니다~

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 이미 쓰긴합니다 하암ㅋㅋ 매퍼 코드 있으니 함 생각해보세요~

boolean isBookMarked = isBookMarked(memberId, bakeryId);
List<MenuResponseDTO> menuList = MenuMapper.INSTANCE.toMenuResponseDTOList(bakeryMenu);
List<MenuResponseDTO> menuList =
MenuMapper.INSTANCE.toMenuResponseDTOList(menuRepository.findAllByBakery(bakery));
String address =
bakery.getState()
+ " "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요거 띄어쓰기 BLANK_SPACE 이런 식으로 상수화 처리 어떠실까욜

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로 address 가져오는 부분 함수로 빼는건 어떠실까욜 매개변수가 너무 많을까요??

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오옹 상수화는 생각도 못했네요 좋습니다! address 가져오는 부분도 함수로 빼는거 좋습니다!

Expand Down