-
Notifications
You must be signed in to change notification settings - Fork 0
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
[DELETE] 회원 탈퇴 #108
[DELETE] 회원 탈퇴 #108
Conversation
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.
수고하셨습니다~
논의 완료 및 리뷰 반영 후 재언급 부탁드려요!
@@ -7,4 +7,5 @@ public interface AuthService { | |||
|
|||
SignInResponse signIn(String socialAccessToken, SignInRequest request); | |||
void signOut(Long memberId); | |||
void withdraw(Long memberId); |
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.
오늘 논의한대로 notNull을 보장하기 위해 long 타입을 사용하면 더 좋을 것 같아요!
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.
이 부분은 제가 Long 타입을 사용한게 너무 많아서 추후에 이슈 파서 한 번에 리팩토링하겠습니다!
src/main/java/com/soptie/server/auth/service/AuthServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/soptie/server/auth/service/AuthServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/soptie/server/auth/service/AuthServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/soptie/server/memberDoll/service/MemberDollServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/soptie/server/auth/service/AuthServiceImpl.java
Outdated
Show resolved
Hide resolved
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.
소현이 코드리뷰 맛있다 저도 배워갑니다 고생하셨어요~~~
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.
고생하셨어요!
개인적인 의견 기반으로 추가 리뷰 달았어요 :)
자잘한 논의점이 전부라 우선 승인하겠습니다! 리뷰 보시고 반영할 점 반영해서 병합해주시면 좋을 것 같아요. 수고하셨습니다~~
private void deleteCompletedMemberDailyRoutine(Member member) { | ||
completedMemberDailyRoutineService.deleteCompletedMemberDailyRoutine(member); | ||
} |
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.
CompletedMemberDailyRoutine도 한 회원이 여러 개를 가질 수 있기 때문에,
메서드명은 복수형을 활용하여 deleteCompletedMemberDailyRoutines 를 사용해도 좋을 것 같아요!
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.
아 맞네요 중복 리뷰 죄송합니다ㅠㅠ
public void deleteCompletedMemberDailyRoutine(Member member) { | ||
completedMemberDailyRoutineRepository.deleteAllByMember(member); | ||
} |
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.
@Override
를 붙이지 않은 이유가 있나요??
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.
@OverRide와 @transactional 누락했습니다...ㅠ 수정했습니다
deleteMemberDoll(member); | ||
deleteMemberDailyRoutines(member); | ||
deleteMemberHappinessRoutine(member); | ||
deleteCompletedMemberDailyRoutine(member); | ||
deleteMember(member); |
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.
이번 리뷰를 달다가 든 생각인데,
각 delete 메서드에 Member를 넘기기보다는 해당 값
(예를 들면, deleteMemberDoll(member) -> deleteMemberDoll(memger.getMemberDoll())
을 인자로 넘기는 방법을 사용하면 더 직관적일 것이라고 생각되는데 어떤가요??
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.
뭔가 모든 메서드가 member를 넘기다보니까 member를 넘기게 됐습니다...ㅋㅋㅋ
원래 리뷰해주신 거처럼 메서드를 작성했다가 오히려 다시 member를 넘기는 걸로 고쳤는데 ㅠㅠ
생각해보니 처음부터 memberDoll을 넘겨서 뒤에서 member.getMemberDoll()을 두 번 호출할 일을 줄이는게 좋은 거 같아 리뷰 반영해두었습니다!
✨ Related Issue
📝 기능 구현 명세
🐥 추가적인 언급 사항
메서드 분리할게 되게 많고 authServiceImpl에서 다른 service를 많이 불러왔어야했는데 한 번 더 좋은 방법이 있는지 생각해주시면 감사하겠습니다.
또한 멤버가 인형, 데일리 루틴, 행복 루틴을 가지고 있을 때에도 삭제해주는 코드를 작성하긴 했으나 테스트를 해보지는 않아서 이후에 계정 인증 문제가 해결되거나 소현이가 숙소에 복귀하면 테스트하고 문제가 있다면 이슈 생성 후 수정하겠습니다.