-
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
[SKRB-146] feat: user 도메인 생성 및 검증 로직 테스트 #10
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.
커밋이 9개로 PR단위가 크다는 생각이 듭니다..!
|
||
private void validate(String nickname) { | ||
Assert.hasText(nickname, "닉네임을 입력해 주세요."); | ||
Assert.isTrue(!nickname.contains(" "), "닉네임에 공백을 포함할 수 없습니다."); |
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.
전체 공백인 경우 (ex. " ")는 막아야하지만 닉네임의 경우 중간 공백은 자연스럽다는 생각이 들었습니다! (ex. "바다의 왕자)
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.
아 굳... 수정할게요
입력 받은 값 trim 해서 10글자 제한으로 설정할게요
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.
(중간 enter 설정 가능)
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.
중간 공백은 필요하다고 생각합니다
Assert.isTrue(isInValidRange(nickname), "닉네임은 2자 이상 10자 이하로 입력해 주세요."); | ||
} | ||
|
||
private boolean isInValidRange(String nickname) { |
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.
isInvalidRange
가 자연스러운 것 같습니다.
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.
invalid가 아니라 유효한 범위 안인지 확인하는 is in valid range 입니다 :)
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.
valid의 반대말인 invalid와 혼동의 여지가 있을 것 같은데 checkValidRange
같은 메서드 명은 어떠신가요?
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.
그냥 유효한지 검사하기 위한 메소드라면 isValid나 isValidRange 는 어떠신가요?
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.
is valid range => isValidRange로 수정할게요
private Long userid; | ||
|
||
@Embedded | ||
private RequiredInfo requiredInfo; |
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.
RequiredInfo
라는 뜻이 애매한 것 같습니다. UserInfo
는 어떨까요?
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.
UserInfo는 email도 user info인데, 필수적으로 입력받아야 하는 값 (닉네임, 전화번호) 만 설정한거에요
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.
나머지도 결국 다 필수적으로 입력되는 값인데 사용자에게 입력받는 값을 나타내는 이름(ex. input이 들어간)이면 좋을 것 같다는 생각이 듭니다..! (참고 의견입니다.)
@Column(nullable = false, unique = true) | ||
private String oauthUserName; |
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.
잘 감이 오지 않는데 식별 값의 예시가 있나요?
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.
커스텀하게 만들어 줘야하는값인데, 카카오에서 넘어오는 id값이랑 + provider를 조합해서 만들면 될것 같아요
e.g) kakao_12345678
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.
수고하셨습니다~ 코멘트 몇 개 남겼어요
public static final int MIN_NICKNAME_LENGTH = 2; | ||
public static final int MAX_NICKNAME_LENGTH = 10; |
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 으로 열어둔 이유가 있나요?
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 validate(String nickname) { | ||
Assert.hasText(nickname, "닉네임을 입력해 주세요."); | ||
Assert.isTrue(!nickname.contains(" "), "닉네임에 공백을 포함할 수 없습니다."); |
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.
👍
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.
LGTM
3f937c9
to
924b843
Compare
💠 Jira 티켓 링크
🖥️ 작업 내용
✅ PR시 확인 사항
📢 리뷰어 전달 사항