Conversation
🔗 Resolves: #296
|
PR 이전보다 구체적으로 작성해서 좋습니다 ~! 👍👍 |
| public ResponseEntity<Void> logout(@CurrentUser User user, HttpServletRequest request) { | ||
| HttpSession session = request.getSession(false); | ||
| if (session != null) { | ||
| // redis의 세션도 삭제 | ||
| session.invalidate(); | ||
| } | ||
| service.logout(user); | ||
| return ResponseEntity.ok().build(); |
There was a problem hiding this comment.
회원 정보 수정 기능을 개선하는데, 로그아웃 요청 시에 HttpServletRequest가 아닌 User로 수정하는 것과 어떤 연관이 있나요?
There was a problem hiding this comment.
기존 로그아웃은 해당 회원의 세션만 삭제해서 로그인 여부를 판단했습니다 하지만 CurrentUser를 이용해 redis에 사용자를 캐싱하게되므로써 캐싱된 회원 객체도 삭제시켜줘야한다는 필요성이 생겼습니다 그래서 서비스 로직에서 redis에 저장된 해당 회원객체를 삭제해야했습니다!
추후 인가부분의 재정리가 필요해 정리할 예정입니다!
| redisTemplate.opsForValue().set(cacheKey, user, Duration.ofMinutes(30)); | ||
| } | ||
|
|
||
| private User getPersistUser(User user){ |
There was a problem hiding this comment.
이 메소드를 userRepository 쪽으로 옮기면 어떨까요?
There was a problem hiding this comment.
해당 메서드를 userRepository에 옮긴다는게 무슨 말인지 잘 모르겠습니다...
영속적인 객체를 사용하기 위해 findbyId로 찾았고 모든 업데이트 메서드에 존재하고 없는 회원에 대한 예외때문에 따로 메서드로 빼서 사용중이었습니다
구체적인 예시를 알려주실수 있으실까요,,?🥲
There was a problem hiding this comment.
userRepository에 default 메소드로 빼면 좋을 것같아요
There was a problem hiding this comment.
별 차이가 없다고 느껴집니다
혹시 서비스 로직에서 사용되는 것과 repository에서 default 메서드로 선언되는 것이랑 무슨 차이가 있을까요??
repository에서는 데이터에 접근 할뿐 이에 대한 예외처리는 어짜피 서비스 로직에서 해야한다고 생각됩니다!
왜 default 메서드로 빼면 좋을지! 설명 부탁드립니다!
There was a problem hiding this comment.
다른 곳에서도 유저를 사용해서 레포지토리에 있다면 반복 코드를 쓰지 않을 것 같아서 입니다.
누군가는 elseThrow를 할거니 그것의 책임을 아예 repository로 가져가는거라고 생각했습니다.
기존에 findById는 optional로 감싸서 만들었기때문에 이부분을 interface 부분에서 처리를 하는 거죠!
There was a problem hiding this comment.
해당 부분은 @currentuser로 인해 다른 도메인의 서비스 로직에서 User를 직접 조회해서 사용하는 경우는 없다고 생각했습니다!
근데 다른 도메인의 코드를 살펴보니 review 서비스 175번줄에서 userRepository의 findById를 사용한다는 것을 인지하게 되었습니다!
만약 해당 default 메서드를 생성한다면 review 서비스 로직에서 사용하실 의향이 있으신가요오??
There was a problem hiding this comment.
사용할 것 같습니다.! 추후에 내 자신이 아닌 다른 사용자를 확인 할때 필요할 수 도 있을 것 같습니다.
| @@ -67,13 +67,15 @@ public boolean checkPW(RecoveryPWRequest request){ | |||
| } | |||
|
|
|||
| public void updatePW(User user, UserNewPasswordRequest request){ | |||
There was a problem hiding this comment.
아래에 사용자 정보 업데이트 메서드들이 동일한 패턴을 반복하고 있는데 패턴을 추상화해서 중복을 제거하는것 어떻게 생각하시나요?
There was a problem hiding this comment.
업데이트에 대한 각각의 구현체로 만들라는 말씀이실까요오??
There was a problem hiding this comment.
밑의 코드들이
- getPersistUser(user) 호출
- 유저 정보 업데이트
- updateUserCache(persistUser) 호출
의 정형화된 패턴을 띄고 있는데 이것들을 하나로 묶어서
public void update메서드 (User user, UserRequest request) {
업데이트 실행 메서드 (user, persistUser -> persistUser.update (request.업데이트할 것()));
}
이런식으로 할 수 있지 않을까 해서요.
There was a problem hiding this comment.
진호님 말 처럼 개별적으로 수정이 들어가는데
전화 번호, 이름, 비밀번호, 이메일 동의 이렇게 4개를 한번에 바꾸면 4개 메소드가 불려오는 방식인건가요 controller 4번 호출 같이 되는 건가요?
개별적으로 로직 처리가 있어서 개별적인 메소드 처리라면 방법이 떠오르진 않지만
통합해서 방법을 생각해봐야 할 것 같아요
There was a problem hiding this comment.
지금 현재 피그마 상으로 각각 개별적으로 수정이 이루어져 해당 수정시마다 개별적인 컨트롤러가 호출 되고 있습니다
그래서 개별적인 서비스로직이 존재하게 되었습니다,,
중복되는 코드에 대해선 개선하고 알려드리겠습니다!
🔗 Resolves: #296
🔗 Resolves: #296
4 participants
🔗 Resolves: #296
🚀 어떤 기능을 구현했나요 ?
🔥 어떤 문제를 마주했나요 ?
✨ 어떻게 해결했나요 ?
📝 어떤 부분에 집중해서 리뷰해야 할까요?
📚 참고 자료 및 회고