-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
[FEAT/#71] 내 프로필 조회 API 구현 #72
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다 ~! 코멘트 확인해 주세요
고민하셨던 지점에 대한 제 생각을 남겨보자면,
mapper
를 사용할 경우 서비스는 데이터 조회에 집중하고, 변환 로직은 mapper
가 담당하여 역할이 명확해진다는 장점이 있을 것 같아요
변환 부분이 mapper
로 이동하며 세부 로직이 숨겨질 것을 걱정하셨지만, 오히려 이런 변환 로직을 숨기는 추상화를 하기 위해 mapper
를 사용하는 거라고 저는 생각합니당.
다만 저희 코드에서 엔티티 <-> 도메인을 위한 mapper
를 제외하고는 대부분의 DTO 변환이 mapper
보다는 서비스 로직 상에서 builder
를 이용해 이뤄지고 있기 때문에, 지금은 현재 상태로 두는 게 맞는 것 같네요
나중에 폴리싱 위크 때 한 번에 다 바꿔봅시다잉 !!
@Builder | ||
@JsonInclude(Include.NON_NULL) | ||
public record ProfileResponse( | ||
@NotNull |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1:
생각해 보니까 Response
에서 NotNull
을 검증하는 Valid
는 다 빼는 게 맞는 것 같네요
이 어노테이션들을 붙였다고 해도 컨트롤러에서 해당 객체 앞에 @Valid
어노테이션이 붙어있을 때만 해당 어노테이션이 제대로 작동하기 때문입니당
또한 @NotNull
어노테이션이 붙어있는 필드들 모두 nullable
이 false
하도록 DB 혹은 애플리케이션 단에서 방어 로직들이 작성되어 있기도 하구요,
만일 NotNull
이어야 하는 필드가 null
이라고 하더라도 READ
를 요청하는 해당 API를 호출한 클라이언트의 귀책이 아니라 서버에 귀책 사유가 있기 때문에 공통 에러를 반환하는 Valid
처리를 하는 건 잘못된 방식인 것 같아요 !!
동일하게 Response
에서 Valid
를 진행하고 있는 다른 DTO
들에 대해서는 제가 수정해 놓을 테니 해당 DTO에서만 수정해 주시면 감사하겠습니당
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3949c1c
좋은 의견 감사합니다!
} | ||
Optional<MemberEntity> optionalMemberEntity = memberRepository.findBySocialTypeAndSocialId(socialType, | ||
socialId); | ||
MemberEntity memberEntity = optionalMemberEntity.orElseGet(() -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P5:
좋은 리팩토링이네요 👍
.socialType(socialType) | ||
.socialId(socialId) | ||
.leftAcornCount(25) | ||
.nickname(UUID.randomUUID().toString()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1:
UUID.randomUUID().toString()
는 총 36자일 뿐만 아니라 -
를 사용해 저희 닉네임 규칙을 위반합니다. (1~16자 이내)
다른 방식으로 닉네임을 할당해야 할 것 같아요 ~! 해당 부분은 TODO로 남겨주시면 프로필 수정 API 구현 때 제가 수정해 보겠습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 감사합니다!
dfa3499
.verifiedArea(verifiedAreaEntityList.stream() | ||
.map(verifiedAreaEntity -> new ProfileResponse.VerifiedArea(verifiedAreaEntity.getId(), | ||
verifiedAreaEntity.getName())) | ||
.collect(Collectors.toList())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1:
JAVA 16 이상부터는 아래와 같이 써도 무방합니다.
.collect(Collectors.toList()))
-> .toList())
이렇게 쓸 경우, List.of()
처럼 작동하여 가변 리스트가 아닌 불변 리스트를 반환하기 때문에 데이터 구조도 안전하고(수정이 불가능), 더 간결할 뿐더러 불필요한 복사 과정이 줄어 더 빠릅니다.
(.collect(Collectors.toList())
는 내부적으로 새로운 ArrayList를 생성하고 .add()
를 통해 요소를 추가하므로 .toList())
에 비해 상대적으로 느림.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정했숨다
1d3e024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다 어푸어푸 🌊🌊
base 문제로 #74 먼저 머지하고 이거 머지해 주세요 ~!
[FEAT/#73] 토큰 재발급 / 로그아웃 / 회원 탈퇴 기능 구현
💡 Issue
📸 Screenshot
📄 Description
💬 To Reviewers
memberEntity와 verifiedEntity를 기반으로 profileResponse 를 변환할 때 Mapper를 사용할까 고민을 했어요.
하지만 막상 구현을 했을 때 Mapper에 여러 함수들이 생기고 여러 로직들이 숨겨진다고 생각했어요. 예를 들어 birthDate가 null일 땐 string으로 바꾸지 않기 위해 convertBirthDate 함수를 만들거나, verifiedAreaList 처리를 만들기 위해 mapVerifiedAreas 함수를 또 구현한다거나 등등 .. 그런데 이런 부분이 코드가 줄어들긴해도 어떻게 변환하는지 명시적이지 않아서 Mapper로 또 이동해서 확인하게 되어 번거롭더라구요.
그래서 저는 아래 Builder 코드가 어떻게 변환이 되는지 더 직관적이라고 느꼈어요. 이 부분에 대해서 어떻게 생각하시는지 궁금해요!