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

feat: implement core module #9

Merged
merged 8 commits into from
Jan 25, 2024
Merged

feat: implement core module #9

merged 8 commits into from
Jan 25, 2024

Conversation

songyi00
Copy link
Member

@songyi00 songyi00 commented Jan 23, 2024

Issue Number #7

close: #7

작업 내역

구현 내용 및 작업 했던 내역

  • exceptionHandler, customException 구현
  • logback 로깅 설정
  • gradle jar 태스크 정리
  • package 경로 정리

변경사항

  • 의존성 목록
    api-server, batchcore 모듈을 의존

PR 특이 사항

PR을 볼 때 주의깊게 봐야하거나 말하고 싶은 점

  • 로깅 설정은 core 모듈 구성하면서 추가로 넣어놓았는데 세부 설정 (롤링 정책 등)은 추후 논의해보면 좋을 것 같습니다!

@songyi00 songyi00 requested a review from chominho96 as a code owner January 23, 2024 12:24
@songyi00 songyi00 self-assigned this Jan 23, 2024
@songyi00 songyi00 added the enhancement New feature or request label Jan 23, 2024
Copy link
Member

@chominho96 chominho96 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다~! 추가로 논의하면 좋을 것 같은 부분들 코멘트 남겼어요!!

@@ -0,0 +1,7 @@
package nexters.dividend.core.exception;

public record ErrorResponse(
Copy link
Member

Choose a reason for hiding this comment

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

record로 선언한게 인상깊네요!!

Copy link
Member Author

Choose a reason for hiding this comment

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

불변 객체이면서 보일러플레이트 코드를 줄여준다는 점에서 DTO 로 사용하면 좋을 것 같더라구요ㅎㅎ
kotlin에서도 유사하게 DTO 로 class 보다는 data class를 주로 사용해요!!

Copy link
Member

Choose a reason for hiding this comment

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

오 그러네요 data class를 찾아봤는데 record랑 거의 똑같은 문법이군요!!

Copy link
Member Author

@songyi00 songyi00 Jan 25, 2024

Choose a reason for hiding this comment

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

불변 객체, 상속 가능 여부를 제외하고는 비슷한 용도로 사용하기에 좋은 것 같아요! 😸

Copy link
Member

Choose a reason for hiding this comment

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

HttpMessageNotReadableException (잘못된 형식으로 request를 요청할 경우), MissingServletRequestParameterException (필수 request parameter가 누락되었을 때),
MethodArgumentNotValidException (validation 오류가 발생했을 때)
이 정도 예외는 꽤 빈번하게 발생할 수 있을거 같아서 global exception handler에서 핸들링하는건 어떠신가요??

Copy link
Member

Choose a reason for hiding this comment

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

추가로 예외 처리를 빠뜨리거나 등등 이유로 controller 단에서 잡히지 않은 exception에 대해서 클라이언트에게 알려주기 위해 Exception.class를 global exception handler에서 잡는 것은 어떠신가요??
그냥 500 에러 발생하는 것보다 메시지라도 띄워주는게 어떤가 해서요!!

Copy link
Member Author

Choose a reason for hiding this comment

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

헉 500 에러를 놓쳤군용
누락된 HttpMessageNotReadableException, MissingServletRequestParameterException 도 추가하는게 좋을 것 같아요!!
꼼꼼한 리뷰 감사합니다ㅎㅎ 수정해놓을게요!!

Copy link
Member

@chominho96 chominho96 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다~!

@songyi00 songyi00 merged commit ec79365 into develop Jan 25, 2024
1 check passed
@songyi00 songyi00 deleted the feat/#7 branch January 25, 2024 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants