-
Notifications
You must be signed in to change notification settings - Fork 0
feat: 회원 가입 기능 구현 #19
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
base: dev
Are you sure you want to change the base?
Conversation
naekang
commented
Dec 4, 2022
- 사용자 회원 가입 기능 구현
- 회원 가입 테스트 코드 작성
|
bokyung95
left a comment
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.
코드 깔끔하게 잘 작성해주셨네요!
한 가지 저의 의견 남겨봅니다. 확인 부탁드려요:)
| .birth(LocalDate.of(2022, 02, 15)) | ||
| .gender(Gender.MALE) | ||
| .isEmailAuth(true) | ||
| .build(); |
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.
SignUpRequest를 생성하는 부분이 여러 번 반복되고 있는 것 같아
따로 메서드로 추출하면 중복코드가 줄어들 수 있을 것 같습니다!
younghoondoodoom
left a comment
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.
주말에도 고생하셨습니다! 제 생각 코멘트로 남겼으니 참고 부탁드릴게요
그리고 제 생각에는 불필요한 공백줄이 조금 있는것 같은데 커밋하시기 전에 option+command+L로 포멧팅하고 올려주시는게 어떠실까요??
|
|
||
| @JsonProperty("isEmailAuth") | ||
| private boolean isEmailAuth; | ||
|
|
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.
dto에서 validation을 넣으면 좋을 것 같습니다
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 class MemberServiceImpl implements MemberService { | ||
|
|
||
| private final PasswordEncoder passwordEncoder; | ||
| private final EmailService emailService; |
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.
저번에 말씁드리려다가 못 드렸는데 MemberService에서 다른 Service 계층에 의존하는 것이 조금 어색한 것 같습니다. 일반 Component로 등록하는 방식은 어떠신가요?
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.
클래스명 변경없이 어노테이션의 수정만을 말씀하시는게 맞을까요?
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.
클래스 명도 수정 하는 것이 좋을 것 같습니다! 이름을 보면 service 계층이구나 할 수 있기 때문에요
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 (!signUpRequest.getPassword().equals(signUpRequest.getPasswordCheck())) { | ||
| throw new NotMatchPasswordException(); | ||
| } |
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.
이 부분은 Dto에서 Validation처리를 통해 할 수 있을 것 같습니다.