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.

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

Merged
merged 5 commits into from
Oct 31, 2023
Merged

[REFACTOR] 윤한 담당 API 리팩토링 #167

merged 5 commits into from
Oct 31, 2023

Conversation

unanchoi
Copy link
Contributor

@unanchoi unanchoi commented Oct 9, 2023

PR이 늦었습니다... 작업하면서 애매한 부분이 생겨서 처음에는 작업을 다 완료한 뒤에 PR을 올리고, 수정을 해보려고 했는데 너무 코드 변경이 많아져서 우선 일차적으로 PR을 올렸습니다.
논의해보고 수정한다음에 다시 작업진행하겠습니다.

Related issue 🚀

Work Description 💚

  • 진행하기로 한 API 리팩토링 방향대로 API 리팩토링을 진행했습니다.
  • request DTO의 경우 파라미터 네이밍을 request, response DTO의 경우 네이밍을 response로 변경했습니다
  • 한개의 service에서 해당 entity의 repository만을 의존하도록 코드를 변경했습니다.
  • method 이름을 컨벤션에 맞춰서 변경했습니다.
  • 연관관계 엔티티를 조회하는 경우에 id가 아니라 entity로 조회하도록 메소드를 변경했습니다.
  • BetaAuthController는 사용하지 않기 때문에 @Deprecated 어노테이션을 붙여놨습니다.
  • 미사용 import 문 제거 및 코드 스타일 정리를 진행했습니다.

의논사항

#166
여기에 작성했습니다.
논의사항 Comment 해주시면 맞춰서 코드 수정한 뒤에 다시 push하겠습니다 이후에 리뷰 해주시면 감사하겠습니다~!

고려사항

  • AppleService와 KakaoService의 API 요청 방법 통일은 추가로 리팩토링 진행하도록 하겠습니다.

API Test 스크린샷 결과 첨부

  1. Social Login Test
image
  1. Member 학습 계획 설정 API
image
  1. Token 재발급 API Test
image
  1. Member 닉네임 중복검사 API Test
image
  1. Member 유저네임 설정 API
image
  1. Push 알람 동의여부 수정 API Test
image
  1. 사용자 정보 조회 API Test
image
  1. 로그아웃 API Test
image
  1. 회원 탈퇴 API Test
image

@unanchoi unanchoi added API API 관련 UNAN🐻 Unan 작업 labels Oct 9, 2023
@unanchoi unanchoi self-assigned this Oct 9, 2023
@unanchoi unanchoi requested a review from thguss October 9, 2023 09:01
Copy link
Member

@thguss thguss left a 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
Copy link
Member

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));
Copy link
Member

Choose a reason for hiding this comment

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

Entity명을 제외하기로 했으니 getProfile이라는 메소드명은 어떨까요??

Copy link
Contributor Author

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) {
Copy link
Member

Choose a reason for hiding this comment

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

해당 Entity 이름은 메소드명에서 제외하기로 했으니 getProfile로 명명해도 좋을 것 같아요!

Copy link
Contributor Author

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)));
Copy link
Member

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);

Copy link
Contributor Author

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) {
Copy link
Member

Choose a reason for hiding this comment

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

복수형은 All 워딩을 사용하는 방식 좋은 것 같아요 :)

Copy link
Member

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());
Copy link
Member

Choose a reason for hiding this comment

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

굳!

Copy link
Member

@thguss thguss left a 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;
Copy link
Member

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 테스트 작업해주시면 감사하겠습니다!

학습 계획이 설정되지 않은 경우를 표현하기 위해서 에러메세지를 변경했습니다.
@unanchoi unanchoi merged commit dd2412e into develop Oct 31, 2023
1 check passed
@unanchoi unanchoi deleted the unan_#165 branch October 31, 2023 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API 관련 UNAN🐻 Unan 작업
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CHORE] API convention에 맞춰 리팩토링 작업 진행
2 participants