-
Notifications
You must be signed in to change notification settings - Fork 10
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
[20기_이한슬, 최서지] spring-vote 미션 제출합니다. #8
base: angel-bridge
Are you sure you want to change the base?
Conversation
feat: 로그인 및 멤버 관련 API 구현
feat: vote 전체 기능
fix: 파트장 후보를 developer 엔티티로 분류
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.
마지막 과제하느라 고생하셨습니다🔥
프로젝트를 진행할 때 머지 방식을 정하면 변경 사항을 추적하기도 쉽고, 커밋 내역이 더욱 깔끔하게 유지될 수 있다는 점을 말씀드리고 싶네요. 머지 방식과 관련된 블로그 글 남겨 둘게요.
@PostMapping | ||
public CommonResponse<Void> createDeveloper(@RequestBody DeveloperRequestDto developerRequestDto) { | ||
|
||
developerService.createDeveloper(developerRequestDto); | ||
return new CommonResponse<>("개발자 생성 완료"); | ||
} |
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.
HTTP 상태 코드를 200으로 해도 괜찮지만 201 CREATED이 더 적합해보여요! 200 OK는 요청이 성공적으로 처리되었음을 나타내지만, 생성과 같이 새로운 리소스를 생성하는 경우에는 201 Created가 더 의미가 있기 때문에 이를 통해 리소스가 성공적으로 생성되었음을 명확하게 전달할 수 있어요!
TeamType type = null; | ||
if (developerRequestDto.team().equals("포토그라운드")) | ||
type = TeamType.PHOTOGROUND; | ||
else if (developerRequestDto.team().equals("엔젤브릿지")) | ||
type = TeamType.ANGELBRIDGE; | ||
else if (developerRequestDto.team().equals("페달지니")) | ||
type = TeamType.PEDALGENIE; | ||
else if (developerRequestDto.team().equals("케이크웨이")) | ||
type = TeamType.CAKEWAY; | ||
else if (developerRequestDto.team().equals("커피딜")) | ||
type = TeamType.COFFEEDEAL; |
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.
TeamType type = null; | |
if (developerRequestDto.team().equals("포토그라운드")) | |
type = TeamType.PHOTOGROUND; | |
else if (developerRequestDto.team().equals("엔젤브릿지")) | |
type = TeamType.ANGELBRIDGE; | |
else if (developerRequestDto.team().equals("페달지니")) | |
type = TeamType.PEDALGENIE; | |
else if (developerRequestDto.team().equals("케이크웨이")) | |
type = TeamType.CAKEWAY; | |
else if (developerRequestDto.team().equals("커피딜")) | |
type = TeamType.COFFEEDEAL; | |
TeamType type = TeamType.getTeamType(developerRequestDto.team()); |
이 역할은 TeamType에서 가지는게 낫지 않을까요?
그리고 Enum의 values()
사용하면 else if 를 제거할 수 있을 것 같아요!
+)그러고 보니 아래 PartType은 잘 하셨네용
|
||
String refreshToken = authService.extractRefreshToken(request); | ||
authService.validateRefreshToken(refreshToken); | ||
|
||
String newAccessToken = authService.reissueAccessToken(refreshToken); | ||
Cookie RefreshTokenCookie = authService.createRefreshTokenCookie(refreshToken); | ||
|
||
authService.setNewTokens(response, newAccessToken, RefreshTokenCookie); |
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.
이 부분 모두 비즈니스 로직이기 때문에 컨트롤러 레이어보다는 서비스 레이어에 있는게 적합하지 않을까용?🤔
만약 HttpServletRequest과 HttpServletResponse 서비스 레이어로 넘겨주는 것에 대해 고민 중이라면, 이 부분 한 번 읽어보시면 도움이 될 수도 있을겁니다.
if (request.part().equals("프론트엔드")) { | ||
part = PartType.FRONTEND; | ||
} else if (request.part().equals("백엔드")) { | ||
part = PartType.BACKEND; | ||
} else { | ||
throw new ApplicationException(INVALID_PART_TYPE); | ||
} |
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.
이 부분도 위에서 언급했던 TeamType과 같은 방식을 사용할 수 있겠죠.
public String extractRefreshToken(HttpServletRequest request) { | ||
Cookie[] cookies = request.getCookies(); | ||
if (cookies != null) { | ||
for (Cookie cookie : cookies) { | ||
if (cookie.getName().equals("refreshToken")) { | ||
return cookie.getValue(); | ||
} | ||
} | ||
} | ||
throw new ApplicationException(NOT_FOUND_REFRESH_TOKEN); | ||
} |
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.
depth를 줄이는 방법은 if 문을 반대로 생각해보는건데요. cookies
가 null이면 ApplicationException가 발생하기 때문에 null이 아닌 경우를 처리하기보다는 반대로 cookies가 null인 경우에 예외를 던지도록 변경하면 depth를 하나 줄일 수 있겠죠.
다음으로 cookie에서 꺼내오는 getName에 항상 값이 있다고 보장할 수 있는건가요? 그게 아니라면 세션에서 말씀드린 것과 같이 비교하는 두 요소의 위치를 변경하는 것을 권장드려요. 이렇게 하면 상수를 기준으로 비교하게 되어 NPE 발생을 방지해줘요.
public String extractRefreshToken(HttpServletRequest request) { | |
Cookie[] cookies = request.getCookies(); | |
if (cookies != null) { | |
for (Cookie cookie : cookies) { | |
if (cookie.getName().equals("refreshToken")) { | |
return cookie.getValue(); | |
} | |
} | |
} | |
throw new ApplicationException(NOT_FOUND_REFRESH_TOKEN); | |
} | |
public String extractRefreshToken(HttpServletRequest request) { | |
Cookie[] cookies = request.getCookies(); | |
if (cookies == null) { | |
throw new ApplicationException(NOT_FOUND_REFRESH_TOKEN); | |
} | |
for (Cookie cookie : cookies) { | |
if ("refreshToken".equals(cookie.getName())) { | |
return cookie.getValue(); | |
} | |
} | |
throw new ApplicationException(NOT_FOUND_REFRESH_TOKEN); | |
} |
if (type.equals(PartType.BACKEND)) { | ||
if (loginMember.isVoteBack()) |
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 문 내부 로직을 하나의 메서드로 추상화할 수 있지 않을까요?
- enum을 equals로 비교할 때 장점은 무엇인가요?
throw new ApplicationException(ALREADY_VOTE_DEVELOPER); | ||
loginMember.voteToFront(); | ||
} | ||
developer.voteToMe(); |
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.
�메서드 이름만 봤을 때 예외 요구 사항처럼 자신한테 투표하는 것이라 느껴지네요
Team team = teamRepository.findById(teamId) | ||
.orElseThrow(() -> new ApplicationException(INVALID_TEAM_TYPE)); | ||
// 같은 팀에게 투표할 수 없음 | ||
if (loginMember.getTeam() == team) |
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.
enum 비교를 equals로 할지 ==로 할 지 팀 내 표준을 만들어보는 것도 좋을 것 같아요!
@Transactional | ||
void deleteByRefresh(String refresh); | ||
|
||
@Transactional | ||
void deleteByUserId(String userId); |
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.
리포지토리에 @Transactional
을 사용하신 이유가 있나요?
|
||
@Operation(summary = "회원 기본 정보 조회", description = "회원의 기본 정보를 조회하는 API") | ||
@GetMapping | ||
public CommonResponse<MemberResponseDto> getMemberInfo(@AuthenticationPrincipal CustomUserDetails userDetails) { |
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.
여기에서는 @Login
이 아니라 @AuthenticationPrincipal
을 사용하신 이유가 있나요?
fix: email 필드 제거
feat: 프론트 배포 주소 추가
No description provided.