Skip to content

[Refactor] 플레이스 리팩토링#131

Merged
hyobin-yang merged 10 commits intodevfrom
refactor/#130
Feb 16, 2025
Merged

[Refactor] 플레이스 리팩토링#131
hyobin-yang merged 10 commits intodevfrom
refactor/#130

Conversation

@hyobin-yang
Copy link
Contributor

@hyobin-yang hyobin-yang commented Feb 15, 2025

#130

  • CategoryCreateRequest, CategoryUpdateRequest를 삭제하고 CategoryTypeRequest로 통일

  • MagazineCreateRequest, MagazineUpdateRequest를 삭제하고 MagazineRequest로 통일

  • PlaceCreateRequest dto 내 모든 필드가 not null 제한 한 번에 걸 수 있도록 커스텀 애노테이션 구현(@AllFieldsNotNull)
    • AllFieldsNotNullValidator가 애노테이션의 검증기
    • 커스텀 애노테이션 작동은 테스트 코드로 정상 작동 확인 완료

  • NPE 방지 위한 예외처리 코드 추가

yoonjiseok
yoonjiseok previously approved these changes Feb 15, 2025
Comment on lines +76 to +82
return PlaceSearchResponse.from(new PlaceSearchResponseParams(
place,
placeImagesUrl,
savedCategory.map(Category::getName).orElse(null),
savedMagazine.map(Magazine::getTitle).orElse(null),
savedMagazine.map(Magazine::getIconUrl).orElse(null))
);
Copy link
Contributor

Choose a reason for hiding this comment

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

따봉 드리겠습니다.

Comment on lines +31 to 50
public static PlaceSearchResponse from(PlaceSearchResponseParams params){
Place place = params.place();
return PlaceSearchResponse.builder()
.placeId(place.getId())
.name(place.getName())
.shortDescription(place.getShortDescription())
.latitude(place.getLatitude())
.longitude(place.getLongitude())
.createdAt(place.getCreatedAt())
.updatedAt(place.getUpdatedAt())
.placeImageUrl(params.placeImageUrls())
.categoryName(params.categoryName())
.magazineTitle(params.magazineTitle())
.instagramLink(place.getInstagramLink())
.naverLink(place.getNaverplaceLink())
.iconUrl(params.iconUrl())
.placeImageId(place.getPlaceImages().stream().map(PlaceImage::getId).toList())
.build();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

좋은 방법 알아갑니다~

sese2204
sese2204 previously approved these changes Feb 15, 2025
category.getName(),
magazine.getTitle(),
magazine.getIconUrl())
);
Copy link
Contributor

Choose a reason for hiding this comment

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

파라미터 많아서 싫다고 하셨는데 이런 방식이면 파라미터 똑같이 5개 아닌가요?
뭔가 괜히 new PlaceSearchResponseParams만 추가된 느낌인데 어떠신가요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

넹 여기서 말했던 것과 같이 그 지점에 대해서도 고민했고 일단은 처음 생각한 대로 구현했습니다. 이유는 서비스 계층에서는 어쨌든 파라미터가 5개로 똑같지만 dto 내부에서는 PlaceSearchResponse 객체를 생성할 때 파라미터가 묶여서 하나로 들어갔으면 좋겠다는 생각이었습니다. 근데 아마 대부분 오빠처럼 불필요하다고 생각할 것 같어요 저도 어느정도 과하다는 거에 동의하고!
요 부분은 다시 바꾸겠습니당

Copy link
Contributor

@sese2204 sese2204 Feb 16, 2025

Choose a reason for hiding this comment

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

아뇨 다시 바꾸기보단 PlaceSearchResponseParams 조립을 따로 서비스에서 진행하고 from 메소드에는 조립된 객체만 넣는 식으로 하면 가독성이 훨씬 좋을 듯 해요

new PlaceSearchResponseParams(필요한 필드들..); 
return PlaceSearchResponse.from(params);

저도 서비스 전용 dto를 만드는 것을 선호해서 좋은 방법이라 생각합니다. 본인 의견 밀고 나가세요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아하 그런 의미였군요
근데 이미 바꿨어요 푸하하

Copy link
Contributor Author

Choose a reason for hiding this comment

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

다시 만듦

String magazineTitle = (placeMagazine != null) ?
placeMagazine.getMagazine().getTitle() : "매거진 없음";
String magazineTitle = (placeMagazine != null) ? placeMagazine.getMagazine().getTitle() : "매거진 없음";

Copy link
Contributor

Choose a reason for hiding this comment

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

null값 내려주는 것보다 String으로 "매거진 없음" 주는 것이 클라이언트에서 더 처리하기 편할까요?

그리고 매거진 정보 응답에 추가하는 부분에서 제가 dto에 MagazineInfo 추가해서 주변 장소 탐색에 사용하고 있는데,
해당 dto 사용하는 건 어때요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오호 그러네요 내려줄 거면 Null값을 반환하는 게 맞는 것 같습니다.

근데 하나 의문인 게 현재 우리 서비스는 place 객체의 카테고리와 매거진 정보가 반드시 유효한 값(null X)이어야 하는 구조 아닌가 하는 겁니다. 맞다면 아예 예외를 던져버리는 게 맞는 게 아닌가 하는 생각이 드네요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오 MagazineInfo 있는지 지금 알았어요 완전 굿이다

yoonjiseok
yoonjiseok previously approved these changes Feb 15, 2025
- CategoryCreateRequest, CategoryUpdateRequest를 삭제하고 CategoryTypeRequest로 통일
- PlaceCreateRequest dto 내 모든 필드가 not null이도록 커스텀 애노테이션 구현
- NPE 방지 위한 예외처리 코드 추가
@hyobin-yang hyobin-yang dismissed stale reviews from yoonjiseok and sese2204 via f848fe5 February 16, 2025 03:00
seungwon7934
seungwon7934 previously approved these changes Feb 16, 2025
@hyobin-yang hyobin-yang merged commit ad8f667 into dev Feb 16, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Refactor] 플레이스 관련 로직 리팩토링

4 participants