-
Notifications
You must be signed in to change notification settings - Fork 179
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
[Spring 장바구니 - 1단계] 더즈(이동규) 미션 제출합니다. #18
Conversation
@ExceptionHandler | ||
public ResponseEntity<String> loginExceptionHandler(RuntimeException exception) { | ||
return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body(exception.getMessage()); | ||
} |
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.
ExceptionHandler를 따로 만들어서 처리하는게 좋을거 같습니다!
아울러 ExptionResponse DTO를 따로 만들어도 좋을거 같아요!
또한 RuntimeException은 너무 포괄적일거 같은데 좀더 타입을 좀히면 좋을거 같네요. 사이드 이펙트가 발생할듯 해서요.
@Getter | ||
public class Email { | ||
|
||
private static final String regex = "^[a-zA-Z0-9_!#$%&'*+/=?`{|}~^.-]+@[a-zA-Z0-9.-]+$"; |
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 Customer signUp(CustomerRequest request) { | ||
validateEmailDuplicated(request.getEmail()); | ||
Password password = new Password(request.getPassword()); |
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.
요청이 들어왔을때 password를 암호화해서 하면 어떨까요?
지금은 개발자가 로그를 찍으면 해당 사용자의 패스워드를 볼 수 잇는 상태인데요.
필터 단에서 하면 되지 않을까 합니다.
엄청 중요한건 아니니 다음 미션에 시간 나시면 적용해주셔도 됩니다 :)
@Getter | ||
@RequiredArgsConstructor | ||
@EqualsAndHashCode | ||
public class EncryptedPassword { |
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.
Password를 받아서 이 객체가 생성돌때 Encrypted가 되는것도 좋을거 같은데 혹시 밖에서 하는 이유가 있나요?
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.
DB에서 가져올 때는 암호화된 비밀번호를 가져오는데 이 때는 다시 한 번 더 암호화를 하는 게 아니라 그대로 넣어서 Customer를 생성해야 합니다. 그래서 밖에서 암호화해서 넣어주거나 그대로 넣어주거나 2가지 생성 방법을 선택했었습니다.
private static final int MIN_LENGTH = 2; | ||
private static final int MAX_LENGTH = 10; | ||
private final String value; |
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.
상수와 필드는 분리해주세요!
MessageDigest digest = MessageDigest.getInstance("SHA"); | ||
digest.update(input.getBytes(UTF_8)); | ||
|
||
StringBuilder encrypted = new StringBuilder(); | ||
for (byte each : digest.digest()) { | ||
encrypted.append(String.format("%02X", each)); |
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.
매직넘버를 상수화 시켜보면 어떨까요?
assertAll( | ||
() -> assertThat(response.getNickname()).isEqualTo("does"), | ||
() -> assertThat(response.getAccessToken()).isEqualTo("access-token"), | ||
() -> verify(customerService).findByEmail(email), |
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.
👍
안녕하세요 럿고! 4기 더즈입니다. 레벨 2 마지막 미션에서 만나게 되었네요 잘 부탁드려요ㅎㅎ
프론트와 협의한 API는 docs 하위 리드미에 있습니다.
그리고 이번 미션은 레거시 코드를 제공받고 시작했는데요. 1단계에선
auth
패키지 하위의Customer
관련 기능과 로그인 기능만 손대고shoppingcart
패키지 부분은 깨졌던 테스트만 통과시키도록 수정만 하였습니다. 1단계에선auth
패키지 중심으로 피드백 주시면 감사하겠습니다!!