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

[Week5] 고양이 장난감가게 만들기 - 심화 #61

Merged
merged 50 commits into from
Jun 27, 2022

Conversation

csh9066
Copy link

@csh9066 csh9066 commented May 30, 2022

이번 주 목표는 일일 리뷰 요청입니다! 잘 부탁드립니다 :)

csh9066 added 30 commits May 30, 2022 15:45
@@ -66,6 +67,27 @@ void setUp() {
}));
}

@Nested
@DisplayName("유효하지 요청 데이터가 오면")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

유효하지 않은 요청 데이터도 좋지만 값을 가진 데이터라는 구체적인 예시를 적는 것이 좋을 것 같아요!

Copy link
Author

@csh9066 csh9066 May 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분도 제가 제대로 이해를 못한거 같아요 ㅜㅜ
빈 값을 가진 요청 데이터가 오면 이렇게 이런 식으로 구체적인 값이 뭔지 예시를 적는게 맞을까요?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

맞아요!
특정 데이터들이 빈 문자열을 가지는 경우이기에 그것들을 명시하면 좋겠어요!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

또한 현재는 모든 데이터가 빈 문자열일 때지만
각 변수마다 빈 값이 허용될 수도 있으므로 테스트를 나눠보는 것도 좋겠어요!

Comment on lines 128 to 135
@DisplayName("user를 삭제한다.")
void it_deletes_user() {
userService.deleteUser(id);

assertThatThrownBy(() -> userRepository.findById(id).get())
.isInstanceOf(NoSuchElementException.class);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

설명과 테스트가 다른 것 같아요!

Copy link
Author

@csh9066 csh9066 May 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

설명과 테스트가 다른 것 같아요!

삭제를 하고 user를 조회해서 제대로 삭제 되어있는지 검증 하는 로직인데

이 부분에 대해서 제대로 이해가 안돼서 그런데 ㅜㅜ 혹시 추가 설명좀 해주실수 있나요??

Copy link

@ssisksl77 ssisksl77 May 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

user를 삭제하는 것을 테스트하는 것이 아니라 예외를 테스트하고 있기 때문입니다!

@NotBlank
private final String name;

@NotBlank

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋아요! NotBlank는 아주 유용하죠!
실행활에서는 좀 더 복잡한 유효성검사를 테스트해보기도 해요!

한번 password 변수는 커스텀 Validator를 만들어서 접목해보는 시도를 하면 좋은 경험이될거에요!

// 특수 문자, 영문, 숫자 3종류를 조합해야 한다.
@Override
public boolean isValid(String value, ConstraintValidatorContext context) {
Pattern pattern = Pattern.compile("^(?=.*[A-Za-z])(?=.*\\d)(?=.*[@$!%*#?&])[A-Za-z\\d@$!%*#?&]{8,16}$");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

똑같은 인스턴스를 매번 쓸 것 같아요! static으로 변경해도 좋을 것 같아요!

assertThat(myPasswordValidator.isValid(value, context)).isFalse();
}
}
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: eof에 빈칸을 추가해주세요!

@Getter
public class UserSignupData {

@Email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

실패시 리턴하는 메시지를 커스텀 해보면 좋은 경험이 될 것 같아요!

private Pattern pattern;
@Override
public void initialize(MyPassword constraintAnnotation) {
pattern = Pattern.compile("^(?=.*[A-Za-z])(?=.*\\d)(?=.*[@$!%*#?&])[A-Za-z\\d@$!%*#?&]{8,16}$");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋아요! 더 나아가서 final static으로 만들어도 좋겠어요!
아래 이미지는 이메일유효성검증관련 코드에요!

image

@ssisksl77
Copy link

한번 재미삼아 패스워드 유효성이 틀렸을 때 리턴하는 메시지를 조금바꿔보는 것은 어떨까요?!

물론 리턴하는 값에 rejectedValue 도 있지만 우리는 메시지에 새로운 인자를 동적으로 추가할 수 있는 것 자체에 집중하죠!

요구사항 : 패스워드 유효성검증이 틀렸을 때, 입력한 문자열만큼의 길이의 "x" 로 치환하여 메시지를 만든다.
예시는 아래와 같습니다.

image

@hannut91 hannut91 merged commit 261e96b into CodeSoom:csh9066 Jun 27, 2022
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.

3 participants