Skip to content

[5주차] 커스텀 에러코드 및 예외 추가#101

Open
rmsxo200 wants to merge 1 commit intohanghae-skillup:rmsxo200from
rmsxo200:week5
Open

[5주차] 커스텀 에러코드 및 예외 추가#101
rmsxo200 wants to merge 1 commit intohanghae-skillup:rmsxo200from
rmsxo200:week5

Conversation

@rmsxo200
Copy link

@rmsxo200 rmsxo200 commented Feb 9, 2025

작업 내용

커스텀 오류 코드 추가
커스텀 오류 추가

이번 주차에서 고민되었던 지점이나, 어려웠던 점을 알려 주세요.

  • 예외처리 시 계층간 오류를 전달하는 방법이 많이 고민되었습니다.
  • Custom Error Code를 사용하기 위해 CustomException을 추가하였습니다.
  • 하지만 도메인 계층에는 의존성 문제로 CustomException을 사용하기 어려웠습니다.
  • 그래서 모든 apllication 계층에서 한번 더 예외 처리를 하게 끔 하였습니다.
  • 문제는 해결된 듯 했지만 코드가 너무 복잡해진 것 같습니다.

리뷰 포인트

계층간 오류처리 방법에 문제가 있는지 궁금합니다!

기타 질문

계층간 오류를 전달을 효율적으로 할 수 있는 방법이 있는지 궁금합니다!



그동안 고생 많으셨습니다!
이번 과정을 통해 많이 성정하고 배워간 것 같습니다!
감사합니다!!

@ann-mj
Copy link

ann-mj commented Feb 13, 2025

@rmsxo200 안녕하세요~ 5주차까지 열심히 참여해주셔서 감사합니다. 리뷰 시작하겠습니다 :)

if(response.success()) {
return ResponseEntity.status(HttpStatus.OK).body(response); //조회 성공
} else {
return ResponseEntity.status(HttpStatus.TOO_MANY_REQUESTS).body(response); // 조회 실패
Copy link

Choose a reason for hiding this comment

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

getShowingMovieSchedules 가 실패하면 모두 TOO_MANY_REQUESTS 로 처리하는게 맞는방법인지, 이렇게 하신 의도가 있었을까요?
조회 실패와 TOO_MANY_REQUESTS 는 다른 의미일 수도 있을 것 같아요.


@ExceptionHandler(HttpMessageNotReadableException.class)
public ResponseEntity<ApiResponse<Void>> handleHttpMessageNotReadableException(HttpMessageNotReadableException e) {
log.error("HttpMessageNotReadableException occurred: {}", e.getMessage(), e); // 로그 추가
Copy link

Choose a reason for hiding this comment

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

// 로그 추가 등의 의미없는 주석은 제거해도 좋을 것 같습니다.

T data
boolean success, // 성공여부
String message, // 응답메시지
ErrorCode errorCode, // 오류 발생시 오류 코드 및 메시지
Copy link

Choose a reason for hiding this comment

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

errorCode 보다는 HttpStatusCode 그대로 뒀어도 괜찮았을 것 같습니다.
message 에 error 메세지가 담겨도 충분할 것 같다는 생각이 듭니다.



> ⚠ `domain`계층에서는 `CustomException`를 사용하지 않는다. ⚠
> * 의존성 최소화를 위해 `domain`계층에서 발생한 오류는 `CustomException`를 사용하지 않고 `application`계층에서 처리
Copy link

Choose a reason for hiding this comment

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

domain 에서 customException 을 사용하는데 네이밍 자체도 헷갈리 수 있어서 저는 별도의 DomainException 을 하나 두는것도 괜찮을 것 같아요.
domain 계층에서 domainException , ErrorCode 를 두고, adapter 에서 domainException 을 핸들링 하는 식으로요.

Copy link

Choose a reason for hiding this comment

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

의존성 최소화에만 초점이 되어있지만, 솔직히 불필요한 코드를 추가하지 않는 관점에서 보면 domain 계층에서 아예 사용하지 않는다는 과할 수 있을 것 같습니다.


//완료 메시지 전송 (비동기)
messagePort.sendMessage("영화 예매가 완료 되었습니다.");
messagePort.sendMessage("영화 예매가 완료되었습니다.");
Copy link

Choose a reason for hiding this comment

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

위, 아래의 메세지 포멧이 다른데 통일하고 상수로 빼는게 좋을 것 같습니다

}
} else {
throw new IllegalStateException("좌석에 대한 락을 획득할 수 없습니다: " + seat);
throw new CustomRequestException("현재 좌석을 다른 사용자가 예매 처리 중입니다." + seat, ErrorCode.SEAT_NOT_AVAILABLE);
Copy link

Choose a reason for hiding this comment

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

요런 message 변수들도 한곳에서 관리해두면 좋을 것 같습니다.

@ann-mj
Copy link

ann-mj commented Feb 13, 2025

계층간 오류를 전달을 효율적으로 할 수 있는 방법이 있는지 궁금합니다!

Unchecked Exception (RuntimeException 계열) 을 활용하면,
도메인 계층에서 발생한 예외가 자동으로 상위 계층까지 전달될 수 있으니, 불필요하게 Exception 으로 감싸지 않아도 될것 같습니다.

@ann-mj
Copy link

ann-mj commented Feb 13, 2025

5주차까지 열심히 구현해주셔서 감사합니다. 👍 고생많으셨습니다.

좋았던 점

  • PR 의 단위자체가 적절해서 리뷰하기 좋았습니다. 💯
  • 계층간 의존성 최소화를 하기 위해 고민하신 부분이 좋았습니다.
  • 보통 예외처리에 대해서 깊게 생각하지 않는 사람들이 많은데, 어떻게 하면 최상위 계층까지 전달할지에 대해서 고민하신 게 느껴져서 좋았습니다 👍

아쉬운 점

  • magic string 이 너무 많이 사용된 것 같습니다.
  • 예외처리 흐름을 처리하신 로직에 복잡도(추가적으로 감싼 형태) 가 있어서 조금 더 수정하면 좋을 것 같습니다.
  • 코드 읽는데 오히려 주석이 너무 많아서 정리가 필요할 것 같습니다. 명확한 코드에서는 주석을 제거해주시는 게 좋습니다.

추가적인 의견

  • checked, unchecked exception 처리에 대한 학습을 해보시는 걸 추천합니다.
  • 의존성을 헤치지 않을 것 인가, 불필요한 중복을 막을 것인가에 대해서 고민해보시는 것을 추천합니다.
    • 실제로 저희팀에서는 불필요한 중복자체가 문제라고 생각을 하시는 분들이 많이 계십니다. 의존성을 조금 더 가져가더라도 직관적으로 이해할 수 있는 흐름을 개인적으로 선호하는 편입니다.

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