Refactor: findMemberIdAfterSaveMember 메서드와 관련된 로직과 도메인을 분리 #68
Conversation
메서드 명이 직관적이지 않다는 피드백을 받았습니다. 이에 After를 Or로 변경합니다.
AuthService, MemberService, AuthDomainService 간 의존성 제거를 위해 Facade Layer 추가
AuthDomainServiceImpl -> MemberServiceImpl 위치 변경 public -> private 변경 saveAuthData -> saveNewMember 메서드명 변경
Facade Layer 추가와 AuthDomainService의 제거로 인해 전체적인 코드가 변경되었습니다. 따라서 기존에 작성했던 테스트 코드를 변경 사항에 맞게 재작성했습니다. 또한, 기존 AuthServiceImpl -> AuthMemberFacadeImpl 로 클래스 명이 변경됨으로 인해 AuthServiceImplTest -> AuthMemberFacadeImplTest 로 변경했습니다.
sjhjack
added
Type: ♻️ Refactor
📄 Jacoco Coverage Report
|
📄 Jacoco Coverage Report
|
| import com.wypl.googleoauthclient.domain.AuthMember; | ||
| import com.wypl.wyplcore.auth.data.response.AuthTokensResponse; | ||
|
|
||
| public interface AuthMemberFacade { |
There was a problem hiding this comment.
개인적으로 뒤에 Service를 붙여주면 좋을 것 같아요!
There was a problem hiding this comment.
Facade는 필요한 Service를 조합해서 사용하기 때문에 Facade Layer와 Service Layer를 명확히 구분하기 위해서 Service는 붙이지 않았습니다!
이 부분에 대해서는 어떻게 생각하시나요?
application/wypl-core/src/main/java/com/wypl/wyplcore/facade/AuthMemberFacade.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| @Transactional | ||
| public void quitMember(AuthMember authMember) { | ||
| // Todo : 회원 탈퇴 로직 논의 | ||
| deleteToken(authMember); | ||
| memberService.deleteMember(authMember); | ||
| } |
There was a problem hiding this comment.
해당 메서드는 인증관련 서비스가 아닌 회원관련 서비스로 이동이 필요해 보입니다!
There was a problem hiding this comment.
해당 메서드는 deleteToken()을 비동기 처리하기로 했어서 별도의 Issue를 생성해서 수정할 생각이었습니다.
우선 현재 Issue에 맞게 해당 메서드를 회원 서비스로 이동시킨 후, 비동기 처리는 Merge 후에 수정하겠습니다.
There was a problem hiding this comment.
추가 코멘트 남깁니다!
해당 메서드를 MemberService로 옮기게 되면, 비동기 처리를 하더라도 TokenService에 의존성이 생기게 됩니다.
서비스간 의존 관계를 해소하기 위해 Facade Layer를 추가한 것인데, 그 존재 의의가 미약해집니다.
현재 AuthMemberFacadeImpl에서 TokenService와 MemberService를 주입받아 로직을 처리하고 있기 때문에 현재의 위치가 더 좋다는 생각이 듭니다.
그렇다고 똑같이 TokenService와 MemberService를 주입받는 MemberTokenFacadeImpl을 생성하는 것은 바람직하지 않다고 생각합니다.
의견 부탁드립니다 😄
application/wypl-core/src/main/java/com/wypl/wyplcore/member/service/MemberServiceImpl.java
Outdated
Show resolved
Hide resolved
application/wypl-core/src/main/java/com/wypl/wyplcore/token/service/TokenServiceImpl.java
Outdated
Show resolved
Hide resolved
|
전체적으로는 잘 작성하셨습니다! 하지만 아직 한 메서드(quitMember)은 분리가 안되어 있는 것 같아요! |
인터페이스가 없는 클래스의 클래스 명에서 impl을 제거합니다.
📄 Jacoco Coverage Report
|
KIMSEI1124
changed the title
KIMSEI1124
changed the title
| @Transactional(readOnly = true) | ||
| @RequiredArgsConstructor | ||
| @Component | ||
| public class AuthMemberFacadeImpl implements AuthMemberFacade { |
There was a problem hiding this comment.
| public class AuthMemberFacadeImpl implements AuthMemberFacade { | |
| public class AuthMemberFacadeService implements AuthMemberService { |
어떠한 기능을 하는 서비스가 있는데 해당 서비스를 구성한 디자인패턴과 같은.. 내용을 클래스에 붙힘
| AuthTokensResponse generateToken(final String provider, final String code); | ||
| AuthTokensResponse reissueToken(final String accessToken, final String refreshToken); | ||
| void logout(final AuthMember authMember); | ||
| void quitMember(final AuthMember authMember); |
There was a problem hiding this comment.
quitMember은 인증 관련 서비스가 아닌 회원 관련 서비스로 옮겨야 할 것 같습니다!
📑 개요
✅ PR 체크리스트
🚀 상세 작업 내용
findMemberIdAfterSaveMember->findMemberIdOrSaveMember메서드 명 변경기존 코드에서 서비스 레이어 간 의존성이 있었습니다. 이를 해결하기 위해 Facade 레이어를 추가합니다.
AuthMemberFacade작성AuthServiceImpl->AuthMemberFacadeImpl클래스 명 변경AuthDomain도메인 구조 변경 및 삭제saveAuthData:MemberServiceImpl로 위치 변경 및 private 메서드로 변경saveAuthData->saveNewMember메서드 명 변경TokenRepository관련 메서드들의 위치를TokenServiceImpl로 변경현재 삭제 예정인 파일들은 모두 임시로 주석 처리한 상태입니다.
코드 리뷰 완료 후 이상 없다고 판단되면 모두 삭제하겠습니다.
📁 ETC
closed: #67