-
Notifications
You must be signed in to change notification settings - Fork 122
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
부산대 BE_정재빈_step3 #206
base: jaebin2019
Are you sure you want to change the base?
부산대 BE_정재빈_step3 #206
Conversation
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.
commit message를 기능단위로 하라고 했는데, 그에 맞게 commit 을 남기지 못한 것 같습니다. 어떻게 해야 좀 더 적절한 commit 과 message를 작성할 수 있을 지가 궁금합니다
=> 기능 단위로 branch를 만들고, 짧게 짧게 commit 을 남기는 형태로 할 수 있었던 것 같은데, 적절히 해내지 못해 아쉬운 것 같습니다
이 부분은 제 기준이지만 크게 신경 안 쓰셔도 될 것 같습니다. 현업 처럼 commit 단위가 기능단위로 확실하려면 이미 기존 시스템이 있고 그 안에서 기능단위로 명확한 수정이 이루어지기 때문입니다. 예를 들어 조회 방식을 SQL 1회 호출에서 2회 호출로 수정하는 것처럼 명확한 기준이 존재하면 메시지 작성 또한 쉽습니다. 메소드 하나에 이름을 붙이고 메소드 하나 별로 커밋을 붙이셔도 됩니다. 사실 너무 많으면 squash merge 하면 됩니다.
알고 있는 한에서 최대한 적용시킬 수 있는 모든 것을 적용하여 코드를 작성하였는데, 여기서 좀 더 나아가기 위해 어떤 것들을 추가하고 공부해야하는 지 또는 어떤 기초가 부족하고 무엇을 보충해야하는 지가 궁금합니다.
Spring 내부에서의 개발 구조는 깔끔해보입니다. 다만 이건 제 생각이지만, 현재 개발 과제 자체는 그리 복잡하지도 않고 DB 트랜잭션이나 외부 호출, 멀티 스레딩이 없습니다. 실제로 우리가 많은 수정이나 모니터링이 이루어지는 부분은 그런 부분이죠. 저는 @transactional이나 공통 로그를 남기기 위한 AOP에 대한 고찰, 테스트 코드 이런 쪽을 조금 집중해보시는 게 어떤가 제안을 드립니다 :) 앞으로는 리뷰어로 추가해주시면 제가 메일로 확인 가능하니 잘 부탁드립니다
import org.springframework.web.bind.annotation.ExceptionHandler; | ||
import org.springframework.web.bind.annotation.RestControllerAdvice; | ||
|
||
@RestControllerAdvice |
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.
Advice랑 Handler 까지 구현하셨군요... 빠르시네요
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.
혹시 ResultCode 에서 현재는 status 값을 int 로 관리하고 있는데,
이 값을 HttpStatus 로 관리하는 편이 더 좋을까요?
현재 status 값이 사용되는 곳은 ResponseEntity.status(resultCode.getStatus()) 이고
public static BodyBuilder status(HttpStatusCode status) {
Assert.notNull(status, "HttpStatusCode must not be null");
return new DefaultBuilder(status);
}
public static BodyBuilder status(int status) {
return new DefaultBuilder(status);
}
------
public DefaultBuilder(int statusCode) {
this(HttpStatusCode.valueOf(statusCode));
}
public DefaultBuilder(HttpStatusCode statusCode) {
this.headers = new HttpHeaders();
this.statusCode = statusCode;
}
------
static HttpStatusCode valueOf(int code) {
Assert.isTrue(code >= 100 && code <= 999, () -> {
return "Status code '" + code + "' should be a three-digit positive integer";
});
HttpStatus status = HttpStatus.resolve(code);
return (HttpStatusCode)(status != null ? status : new DefaultHttpStatusCode(code));
}
과정을 거쳐서 ResponseEntity 가 생성되고 있습니다
- int 로 관리하는 경우
- 코드 값의 역할에 대한 직관성이 떨어질 수 있다
- 불필요한 valueOf 작업이 필요하다
- HttpStatus 로 관리하는 경우
- 코드 값에 대한 직관성이 분명하다
- 불필요한 함수 호출이 없다
=> 따라서, HttpStatus 로 수정하는 것이 좋다. 고 생각합니다
혹시, 이에 대해서는 어떻게 생각하시나요?
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.
이 서비스 기준으로 정답은 사실 다양합니다. 왜냐하면 서비스가 크지 않은 상태에서 포맷팅은 쓸데 없이 코드가 복잡해지는 건 사실이거든요.
다만 현업 기준으로 가면 HttpStatus가 맞습니다. 메소드가 천 개가 넘어가는 서비스 안에서 HTTP Code를 int로 관리하면 문제가 많을 겁니다. 예를 들어 외부 통신 에러 ExternalException을 400으로 설정했다고 합니다. 만약 이 때 팀 차원에서 이것을 500으로 격상시킨다면 어떻게 될까요? 아 물론 예시가 적절치 않았을 수도 있지만, 기본적인 원칙은 그 메소드만 보고도 판단을 할 수 있게 한다. 를 기준으로 삼으면 좋을 거 같습니다.
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.
넵 감사합니다
=> 기능 단위로 branch를 만들고, 짧게 짧게 commit 을 남기는 형태로 할 수 있었던 것 같은데, 적절히 해내지 못해 아쉬운 것 같습니다