Skip to content
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

로그인, 회원가입 시큐리티 코드리뷰 : 서용현 #39

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

zjdtm
Copy link

@zjdtm zjdtm commented Aug 17, 2023

코드마다 리뷰를 작성했었는데 한번에 작성하는게 더 보기 좋을거 같아서 정리해서 PR로 따로 작성하였습니다. 부족한 저의 실력으로 조금이나마 코드 리뷰? 를 한 번 해보겠습니다. 🙇(꾸벅)

코드 리뷰 정리

  1. User 클래스에 RefreshToken 필드가 존재하는데 RefreshToken 테이블을 따로 만들어서 관리하는 것은 어떠실까요 ??

  2. TODO 로 적어두신 @notblank 는 empName, position email 은 @email, password 는 @pattern 애노테이션 사용하시면 좋을 것 같습니다!!

  3. UserLoginRequestDto, UserRegisterRequestDto 에도 @email, @pattern 같은 validation 넣는 것도 어떠실까요 ??

  4. 개인적으로 ApiUtils 로 공통응답 처리 한 거 한 수 배우고 갑니다!

  5. JwtTokenProvider 너무 잘 짜신것 같습니다!!

@ho30archive
Copy link
Contributor

아 댓글 달아야지 했는데 깜박했네요;;; 리뷰해주셔서 감사합니당 ㅎㅎ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants