-
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
[REFACTOR] 윤한 담당 API 리팩토링 #167
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.
수고하셨습니다~
개인적으로 궁금한 점이나 의견은 리뷰로 달아두었으니 merge 전에 확인 한 번 해주시면 좋을 것 같아요~!
추가로, deleteAll
, deleteAllByMember
메소드명이 공존하는 것 같은데, 이 부분도 통일되면 더 좋을 것 같아요! (By~
를 포함하거나 없애거나)
@@ -23,6 +23,7 @@ | |||
@RequiredArgsConstructor | |||
@RequestMapping("/api/beta") | |||
@Tag(name = "BetaAuth", description = "베타 테스트용 인증 API") | |||
@Deprecated |
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.
굳입니다~
@@ -45,7 +45,7 @@ public ResponseEntity<ApiResponse> updateUserProfile(Principal principal, @Reque | |||
, content = @Content(schema = @Schema(implementation = MemberGetResponseDTO.class)))}) | |||
@GetMapping("/me") | |||
public ResponseEntity<ApiResponse> getUserProfile(Principal principal) { | |||
MemberGetResponseDTO response = memberService.getMember(getMemberId(principal)); | |||
MemberGetResponseDTO response = memberService.getMemberProfile(getMemberId(principal)); |
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.
Entity명을 제외하기로 했으니 getProfile
이라는 메소드명은 어떨까요??
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.
좋은 것 같아요 수정하겠습니다!
.minute(0) | ||
.build(); | ||
return MemberGetResponseDTO.of(goal, member, trainingTimeResponseDTO, BadgeResponseDTO.of(getBadge(memberId))); | ||
public MemberGetResponseDTO getMemberProfile(Long memberId) { |
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.
해당 Entity 이름은 메소드명에서 제외하기로 했으니 getProfile
로 명명해도 좋을 것 같아요!
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.
오오 감사합니다!
// 기본 시간 설정 | ||
if (trainingTimeService.getAllByMemberId(memberId).isEmpty()) { | ||
TrainingTimeResponseDTO trainingTimeResponseDTO = TrainingTimeResponseDTO.of("", 22, 0); | ||
return MemberGetResponseDTO.of(goal, member, trainingTimeResponseDTO, BadgeResponseDTO.of(memberBadgeService.getBadgeByMemberId(memberId))); |
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.
별 거 아니긴 한데, BadgeResponseDTO.of(memberBadgeService.getBadgeByMemberId(memberId))
인자를 다른 한 줄로 따로 빼면 가독성이 더 좋아질 것 같아요! (예시 아래)
badgeResponseDTO = BadgeResponseDTO.of(memberBadgeService.getBadgeByMemberId(memberId));
return return MemberGetResponseDTO.of(goal, member, trainingTimeResponseDTO, badgeResponseDTO);
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.
좋은 것 같아요 수정하겠습니다!
trainingTimeRepository.save(trainingTime); | ||
} | ||
|
||
protected List<TrainingTime> getAllByMemberId(Long memberId) { |
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.
복수형은 All
워딩을 사용하는 방식 좋은 것 같아요 :)
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.
deleteAll
과 다르게 ByMemberId
를 추가로 붙인 이유가 궁금해요!
try { | ||
URL url = new URL(valueConfig.getAPPLE_URL()); | ||
HttpURLConnection connection = (HttpURLConnection) url.openConnection(); | ||
connection.setRequestMethod("GET"); | ||
connection.setRequestMethod(HttpMethod.GET.name()); |
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.
수고하셨습니다~ 리팩토링 하신 부분 API 테스트 한 번씩만 부탁드려요 :)
@@ -40,6 +40,7 @@ public class MemberService { | |||
private final MemberBadgeService memberBadgeService; | |||
|
|||
private final ValueConfig valueConfig; | |||
private Member member; |
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.
오 이거 좋네요! 그런데 Member는 final이 안 붙은 이유가 있나요?? (@RequireArgsConstructor 어노테이션이 private final
만 인식할 수 있는 것으로 알아서 물어봅니다~)
혹시 모르니 배포 전에 관련 Member API 테스트 작업해주시면 감사하겠습니다!
학습 계획이 설정되지 않은 경우를 표현하기 위해서 에러메세지를 변경했습니다.
PR이 늦었습니다... 작업하면서 애매한 부분이 생겨서 처음에는 작업을 다 완료한 뒤에 PR을 올리고, 수정을 해보려고 했는데 너무 코드 변경이 많아져서 우선 일차적으로 PR을 올렸습니다.
논의해보고 수정한다음에 다시 작업진행하겠습니다.
Related issue 🚀
Work Description 💚
@Deprecated
어노테이션을 붙여놨습니다.의논사항
#166
여기에 작성했습니다.
논의사항 Comment 해주시면 맞춰서 코드 수정한 뒤에 다시 push하겠습니다 이후에 리뷰 해주시면 감사하겠습니다~!
고려사항
API Test 스크린샷 결과 첨부