-
Notifications
You must be signed in to change notification settings - Fork 1
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
[BUILD SUCCESS] [BUILD SUCCESS] 프로필 조회 기능 구현 #56
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.
빌드에 성공했습니다.
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.
바쁘신데 고생하셨습니다!
@GetMapping("/{memberId:[0-9]+}") | ||
public ResponseEntity<ProfileResponse> get(@PathVariable("memberId") Long targetMemberID, | ||
@RequestHeader("Authorization") String authorization) { | ||
long requestMemberID = cafegoryTokenManager.getIdentityId(authorization); |
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.
[제안] 규칙으로 정하진 않았지만, ID 대신에 기존에 작성된 방식인 Id로 하는건 어떨까요?
@@ -38,7 +38,8 @@ public enum ExceptionType { | |||
CAFE_NOT_FOUND(NOT_FOUND, "없는 카페입니다."), | |||
CAFE_INVALID_BUSINESS_TIME_RANGE(BAD_REQUEST, "영업시간이 허용된 범위를 벗어났습니다."), | |||
CAFE_NOT_FOUND_DAY_OF_WEEK(INTERNAL_SERVER_ERROR, "현재 요일과 일치하는 요일을 찾을 수 없습니다."), | |||
STUDY_MEMBER_NOT_FOUND(NOT_FOUND, "없는 스터디멤버입니다."); | |||
STUDY_MEMBER_NOT_FOUND(NOT_FOUND, "없는 스터디멤버입니다."), | |||
PROFILE_GET_PERMISSION_DENIED(FORBIDDEN, "프로필을 조회할 권한이 없는 상대입니다."); |
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.
[질문] 예외 메시지를 좀 더 구체적으로 작성하는것에 대해 어떻게 생각하시나요?
수빈님이 적어주신 예외 메시지도 구체적이라고 생각은 듭니다.
"(예시)스터디 시작전에는 카공장만 프로필을 조회할 권한이 있습니다" 이런식으로요
예외 메시지는 예외상황에서 빠르게 오류를 찾기 위해 적는것이라고 요즘에 생각이 드는데,
예외 메시지를 어디까지 구체적으로 적어야 해야할지 고민이 되더라구요. 구현 세부사항을 드러낼 수도 있다고 생각이 들어서요
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.
[질문] 예외 메시지를 좀 더 구체적으로 작성하는것에 대해 어떻게 생각하시나요? 수빈님이 적어주신 예외 메시지도 구체적이라고 생각은 듭니다. "(예시)스터디 시작전에는 카공장만 프로필을 조회할 권한이 있습니다" 이런식으로요 예외 메시지는 예외상황에서 빠르게 오류를 찾기 위해 적는것이라고 요즘에 생각이 드는데, 예외 메시지를 어디까지 구체적으로 적어야 해야할지 고민이 되더라구요. 구현 세부사항을 드러낼 수도 있다고 생각이 들어서요
현재 요구사항이 3가지의 서로 관련 없는 조건 중 하나 이상에 해당할 경우 요청이 성공하는 것이기 때문에 말씀하신 것 처럼 어떤 사유로 실패했는지 특정하는 것이 불가능합니다.
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.
[질문] default get을 구현하신 이유와 get을 따로 선언해놓으신 이유가 있을까요 ?
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.
엇 Controller 에서 좀 더 편하게 메서드 호출하기 위함인데, 정작 Controller 에서 사용하지 않는군요. 수정해둘게용
} | ||
|
||
private boolean isOwnerOfProfile(Long requestMemberID, Long targetMemberID) { | ||
return Objects.equals(requestMemberID, targetMemberID); |
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.
[질문] equals 대신에 Objects.equals를 쓴 이유가 있나요?
null-safe 하다라는 장점이 있어서 사용했을거라는 생각은 드는데,
Consequently, if both arguments are null, true is returned
둘다 null이라면 true를 반환하네요
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.
null Safe 때문인데, null 이 있으면 예외 발생하게 해야겠네요
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.
null Safe 때문인데, null 이 있으면 예외 발생하게 해야겠네요
예외보단 어차피 실패해야하니 false를 반환하는게 좋겠어요.
@Override | ||
public ProfileResponse get(Long requestMemberID, Long targetMemberID, LocalDateTime baseDateTime) { | ||
if (isOwnerOfProfile(requestMemberID, targetMemberID)) { | ||
MemberImpl member = memberRepository.findById(targetMemberID).orElseThrow(); |
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.
[제안] 중복코드가 보이네요. 처리하는 예외가 다르지 않다면 메서드로 빼는것이 어떨까요?
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.
빌드에 성공했습니다.
반영 브랜치
main
변경 사항
close #54