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] 3차 리팩토링 진행 #209

Merged
merged 8 commits into from
Feb 27, 2024
Merged

Conversation

unanchoi
Copy link
Contributor

Related issue 🚀

Work Description 💚

3차 리팩토링을 진행했습니다.

  • DTO method 이름 from()으로 교체 및 Service Paramater 교체
  • 소셜로그인 관련 Client 요청 방법 RestClient로 교체

@unanchoi unanchoi requested a review from thguss February 23, 2024 16:39
@unanchoi unanchoi self-assigned this Feb 23, 2024
@unanchoi unanchoi added REFACTOR New feature or request UNAN🐻 Unan 작업 labels Feb 23, 2024
@unanchoi unanchoi changed the base branch from develop to refactor_#198 February 27, 2024 13:49
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.

수고하셨습니다~
우선 큰 문제는 없는 것 같아서 approve 해둘게요!
추가적으로 리뷰 관련하여 논의되면 좋을 것 같아요 :)

} catch (AuthException exception) {
log.error("error : ", exception);
final String token = getJwtFromRequest(request);
if (StringUtils.hasText(token) && tokenValidator.validateToken(token) == VALID_JWT) {
Copy link
Member

Choose a reason for hiding this comment

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

if문 내부 코드가 별도 메소드로 분리되면 좋을 것 같아요 :)

...
if (isValidToken(token)) {
 ...
}
...
private boolean isValidToken(String token) {
 return StringUtils.hasText(token) && tokenValidator.validateToken(token) == VALID_JWT;
}

log.error("error : ", exception);
final String token = getJwtFromRequest(request);
if (StringUtils.hasText(token) && tokenValidator.validateToken(token) == VALID_JWT) {
Long userId = tokenValidator.getUserFromJwt(token);
Copy link
Member

Choose a reason for hiding this comment

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

null로 들어오는 경우가 있나요?? 래퍼타입(Long)이라 잘 몰라서 물어봅니닷!

import lombok.AccessLevel;
import lombok.Builder;

@Builder(access = AccessLevel.PRIVATE)
Copy link
Member

Choose a reason for hiding this comment

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

컨벤션 통일을 위해 ENUM 타입은 import static으로 변경되면 좋을 것 같아요!

Comment on lines 32 to 34
return badges.stream()
.map(badge -> BadgeBaseServiceResponse.of(badge, memberBadges))
.toList();
Copy link
Member

Choose a reason for hiding this comment

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

컨벤션 논의 필요

Comment on lines 23 to 24
// 첫 글자가 공백인 경우
return !(username.charAt(0) == ' ');
return !(Objects.equals(username.charAt(0),' '));
Copy link
Member

Choose a reason for hiding this comment

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

코드로 충분히 파악할 수 있다고 생각하여, 주석은 제거해도 좋을 것 같다고 생각해요!

@@ -164,7 +164,7 @@ private void updateGoalType(Member member, GoalType goalType) {
}
}

private void updatePushAlarmConsent(Member member, Boolean hasAlarm) {
private void updatePushAlarmConsent(Member member, final Boolean hasAlarm) {
Copy link
Member

Choose a reason for hiding this comment

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

hasAlarm은 null이 들어올 수 있는 데이터인가용??

Copy link
Member

Choose a reason for hiding this comment

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

Entity 내부 메소드에서 null 체크 로직 포함하는 것으로 논의

Comment on lines 66 to 70
return appleKeyList.keys().stream()
.filter(appleKey ->
Objects.equals(decodedAppleKey.alg(), appleKey.alg()) &&
Objects.equals(decodedAppleKey.kid(), appleKey.kid())
)
Copy link
Member

Choose a reason for hiding this comment

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

Objects.equals(decodedAppleKey.alg(), appleKey.alg()) &&
                        Objects.equals(decodedAppleKey.kid(), appleKey.kid())

해당 코드도 별도 메소드로 추출하면 직관적으로 가독성이 더 좋아질 것 같아요!


public String getKakaoData(final String accessToken) {
try {
RestClient restClient = RestClient.create();
Copy link
Member

Choose a reason for hiding this comment

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

RestClient는 val 타입으로 지정하지 않은 이유가 있을까요??

@unanchoi unanchoi force-pushed the refactor_#198_unan3 branch from 1537d31 to a595bf9 Compare February 27, 2024 15:01
@unanchoi unanchoi merged commit 0f41821 into refactor_#198 Feb 27, 2024
@unanchoi unanchoi deleted the refactor_#198_unan3 branch February 27, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REFACTOR New feature or request UNAN🐻 Unan 작업
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REFACTOR] 3차 리팩토링
2 participants