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

3rd seminar : 기본과제 + 심화과제 #3

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

YuSuhwa-ve
Copy link
Collaborator

기본과제

세미나 때 코드 그대로 입니당.
ApiResponseDto와 ControllerAdvice를 새로 배웠고 편하게 에러처리를 할 수 있어서 좋았습니다.

심화과제

기본과제에

  • user 도메인의 제약조건 추가(nickname, email unique)
  • post 도메인에 유저 fk 설정(userId를 fk로 두었습니다.)
  • user 조회 api 추가
  • post 생성 api 추가
  • post 조회 api 추가
  • 에러처리

를 했습니다.

Copy link
Member

@jiyeoon00 jiyeoon00 left a comment

Choose a reason for hiding this comment

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

수화 수고했어ㅎㅎ 뭔가 리뷰하면서 공부를 엄청 하게 만들어 주는 코드라 아주 좋았어👍

Comment on lines +18 to +21

public static ApiResponseDto success(SuccessStatus successStatus) { //왜 스태틱? : 이유 response에 대한 객체가 계속 생성될 필요가 없음
return new ApiResponseDto<>(successStatus.getHttpStatus().value(), successStatus.getMessage());
}
Copy link
Member

@jiyeoon00 jiyeoon00 May 4, 2023

Choose a reason for hiding this comment

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

P4) response에 대한 객체가 계속 생성될 필요가 없음 => 이 말이 좀 이해가 안가는게..! 나는 결국 리턴 값으로 new 해서 response 객체를 계속 생성한다고 생각했어!

static메서드로 정의한 이유는 사용부 코드에서 바로 new ApiResponseDto(successStatus)이렇게 생성하는 게 아니라 ApiResponseDto.success(successStatus) 이런식으로 메서드를 호출해서 그 안에서 객체를 생성해서 리턴해주는 패턴을 사용하기 때문에 static 메서드를 사용한거라 생각했어!!

그리고 저 패턴을 처음 봐서 사용 이유를 생각해봤는데, success, error 처럼 메서드 이름으로 생성자의 의미를 명확이 보여줘서 좋은 거 같다고 생각했는데, 수화 생각은 어때?!

Comment on lines +24 to +28
@Autowired
public PostService(UserRepository userRepository, PostRepository postRepository){
this.userRepository = userRepository;
this.postRepository=postRepository;
}
Copy link
Member

Choose a reason for hiding this comment

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

@requiredargsconstructor 의미가 final붙은 필드 생성자 주입해주는 거라 여기 없어도 되지 않을까?

Copy link
Member

@ddongseop ddongseop May 5, 2023

Choose a reason for hiding this comment

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

P3) 나도 헷갈려서 찾아봤는데, @requiredargsconstructor@Autowired가 하는 의존성 주입도 다 해준대! 생성자 빼도 될듯!!

Comment on lines +26 to +37
protected ApiResponseDto SQLIntegrityConstraintViolationException(final SQLIntegrityConstraintViolationException e){

if (e.getMessage().contains("'user_id' cannot be null")){
return ApiResponseDto.error(ErrorStatus.VALIDATION_REQUEST_UNVALID_USERID);
}
else if(e.getMessage().contains("Duplicate entry")){
if (e.getMessage().contains("user.UK_ob8kqyqqgmefl0aco34akdtpe")) {
return ApiResponseDto.error(ErrorStatus.CONFLICT_EMAIL_EXCEPTION);
} else if (e.getMessage().contains("user.UK_n4swgcf30j6bmtb4l4cjryuym")) {
return ApiResponseDto.error(ErrorStatus.CONFLICT_NICKNAME_EXCEPTION);
}
}
Copy link
Member

@jiyeoon00 jiyeoon00 May 4, 2023

Choose a reason for hiding this comment

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

P4) SQLIntegrityConstraintViolationException로 다 잡아서 그 안에서 메세지?를 꺼내서 어떤 에러인지 확인해주는 것 같은데, user.UK_ob8kqyqqgmefl0aco34akdtpe➡️이런문자열이 무슨 의미인지도 잘 모르겠고 만약 다른 DB로 바꾸더라도 저 값이 똑같을까...? 라는 생각이 들었어,,

찾아보니까 스프링은 SQLException을 한번 포장해서 DataAccessException로 던져준다고 하는데 이 키워드로 검색해봐도 좋을 것 같아! 나도 여기 좀 공부해보고 피드백 추가할 부분 있으면 추가할게!

Copy link
Member

@ddongseop ddongseop May 5, 2023

Choose a reason for hiding this comment

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

P4) 우와.. 나도 처음에는 unique 제약조건이 위배됐을 때 SQLIntegrityConstraintViolationException이 뜨길래 이걸 활용해서 제어해볼까 했는데, 저 message가 너무 복잡한 것 같아서 포기했었거든.. 근데 나름 규칙성이 있었구나?? 대단대단!

근데 내 생각에도 이렇게 하면 DB에 의존적인 코드가 되기도 하고, 더욱 다양한 예외를 처리하기 어려울 것 같아! DataAccessException도 찾아봤는데, 아직 예외가 명확하게 추상화되어있지 않다고 하네...

Copy link
Member

@ddongseop ddongseop left a comment

Choose a reason for hiding this comment

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

심화과제까지 하느라 수고 많았어!! 이번주 세미나도 화이팅~~

Comment on lines +26 to +37
protected ApiResponseDto SQLIntegrityConstraintViolationException(final SQLIntegrityConstraintViolationException e){

if (e.getMessage().contains("'user_id' cannot be null")){
return ApiResponseDto.error(ErrorStatus.VALIDATION_REQUEST_UNVALID_USERID);
}
else if(e.getMessage().contains("Duplicate entry")){
if (e.getMessage().contains("user.UK_ob8kqyqqgmefl0aco34akdtpe")) {
return ApiResponseDto.error(ErrorStatus.CONFLICT_EMAIL_EXCEPTION);
} else if (e.getMessage().contains("user.UK_n4swgcf30j6bmtb4l4cjryuym")) {
return ApiResponseDto.error(ErrorStatus.CONFLICT_NICKNAME_EXCEPTION);
}
}
Copy link
Member

@ddongseop ddongseop May 5, 2023

Choose a reason for hiding this comment

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

P4) 우와.. 나도 처음에는 unique 제약조건이 위배됐을 때 SQLIntegrityConstraintViolationException이 뜨길래 이걸 활용해서 제어해볼까 했는데, 저 message가 너무 복잡한 것 같아서 포기했었거든.. 근데 나름 규칙성이 있었구나?? 대단대단!

근데 내 생각에도 이렇게 하면 DB에 의존적인 코드가 되기도 하고, 더욱 다양한 예외를 처리하기 어려울 것 같아! DataAccessException도 찾아봤는데, 아직 예외가 명확하게 추상화되어있지 않다고 하네...

Comment on lines +24 to +28
@Autowired
public PostService(UserRepository userRepository, PostRepository postRepository){
this.userRepository = userRepository;
this.postRepository=postRepository;
}
Copy link
Member

@ddongseop ddongseop May 5, 2023

Choose a reason for hiding this comment

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

P3) 나도 헷갈려서 찾아봤는데, @requiredargsconstructor@Autowired가 하는 의존성 주입도 다 해준대! 생성자 빼도 될듯!!

*/
CONFLICT_EMAIL_EXCEPTION(HttpStatus.CONFLICT, "이미 등록된 이메일입니다."),
CONFLICT_NICKNAME_EXCEPTION(HttpStatus.CONFLICT, "이미 등록된 닉네임입니다."),

Copy link
Member

@ddongseop ddongseop May 5, 2023

Choose a reason for hiding this comment

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

P3) 유저나 게시물 조회할 때에 대한 예외처리도 하면 좋을 것 같아!!

} else if (e.getMessage().contains("user.UK_n4swgcf30j6bmtb4l4cjryuym")) {
return ApiResponseDto.error(ErrorStatus.CONFLICT_NICKNAME_EXCEPTION);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

다양하고 구체적인 에러처리 좋아요 ㅎㅎ

private String nickname;

@Column(nullable = false)
@Column(nullable = false, unique = true)
private String email;

Copy link
Member

Choose a reason for hiding this comment

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

중복되지 않는 기능을 추가했네요!!!

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

Successfully merging this pull request may close these issues.

4 participants