-
Notifications
You must be signed in to change notification settings - Fork 4
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] 공통 에러 처리 및 API 응답 관련 추가 #56
[feat] 공통 에러 처리 및 API 응답 관련 추가 #56
Conversation
- GlobalExceptionHandler, BusinessException 추가 - ErrorCode 인터페이스로 추가
- BusinessCode 인터페이스로 추가
public String getMessage() { | ||
return message; | ||
} | ||
} |
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.
The provided code appears to be an enum class named CommonCodeBusinessCode
that implements the interface BusinessCode
. It defines a single enum constant GET_COMMON_CODE_SUCCESS
, which has three properties: status
, code
, and message
.
Overall, the code looks fine, but there are a few suggestions for improvement:
-
Consider following Java naming conventions. Class names and enum constants should typically use camel case with the first letter in lowercase (e.g.,
CommonCodeBusinessCode
can be renamed toCommonCodeBusinessCode
). -
It would be helpful to include comments or documentation explaining the purpose of the enum and its constants.
-
If this code is part of a larger system, it's important to ensure that error codes (
code
property) are unique and well-defined across the entire system. -
Ensure that the status values used in the enum (
status
property) align with the intended HTTP status codes, as 200 usually represents a successful response. -
Depending on your specific requirements, you might want to consider adding additional enum constants to cover different scenarios or error cases.
Remember that a thorough code review involves examining other parts of the codebase beyond just these few lines.
public String getMessage() { | ||
return message; | ||
} | ||
} |
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.
The code patch you provided appears to be defining an enum called CommonCodeErrorCode
. It implements the ErrorCode
interface and provides an error code for an invalid common code name.
Upon a brief review, I don't see any bugs or immediate issues with this code. Here are a few points to consider for improvement:
- Make sure the package (
com.kernel360.commoncode.code
) matches your project's package structure. - Consider providing a more descriptive message for the error code, such as "Invalid common code name: {code}" instead of just "존재하지 않는 공통코드".
- Ensure that the
ErrorCode
interface is correctly defined and that it aligns with the usage in other parts of your codebase.
Apart from these general considerations, the code you provided seems fine. Remember to thoroughly test and integrate it into your project to confirm its correctness and adequacy for your specific needs.
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.
LGTM
- ex> 200 -> HttpStatus.OK.value()
public String getMessage() { | ||
return message; | ||
} | ||
} |
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.
The code you provided looks fine and does not seem to have any bugs. It defines an enumeration called CommonCodeBusinessCode
that implements the BusinessCode
interface. It has a single constant GET_COMMON_CODE_SUCCESS
that represents a successful common code retrieval with a status of 200, a code of "BC001", and a message of "공통코드 조회 성공" (which translates to "Successful common code retrieval" in Korean).
Since the code snippet is small and straightforward, there are no specific improvements or bug risks to address. However, it's important to ensure that the dependencies (com.kernel360.code.BusinessCode
and org.springframework.http.HttpStatus
) are properly imported and available in your project.
If you have further questions or if you provide additional code snippets, I'll be happy to assist you!
public String getMessage() { | ||
return message; | ||
} | ||
} |
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.
The code patch you provided appears to define an enum called CommonBusinessCode
that implements the ErrorCode
interface. It defines a single enum value SAVED
, which represents a successful save operation with an HTTP status of 200 (OK), a code of "B001," and a message of "저장이 완료되었습니다." (which translates to "Save completed successfully").
Here are a few suggestions for improvement:
-
Consider naming conventions: It is common practice in Java to use CamelCase for class and enum names, so you may want to consider renaming
CommonBusinessCode
toCommonBusinessErrorCode
or something similar to conform with established conventions. -
Error code convention: It's generally a good idea to establish a consistent convention for error codes, such as prefixing them with a specific string or using a specific format. For example, you could use "CB001" instead of just "B001" for the error code in your
SAVED
enum value. -
Expand error handling: Currently, the code only includes a single success case. Depending on your requirements, you may need to define additional enum values to represent different error scenarios or responses.
Overall, without knowing the context or purpose of this code, it's challenging to identify any specific bug risks. However, as long as the code is used correctly and the ErrorCode
interface is implemented properly, this code snippet seems fine. Remember to consider these suggestions based on your project's requirements and coding standards.
import org.springframework.http.HttpStatus; | ||
|
||
public enum CommonBusinessCode implements ErrorCode { | ||
SAVED(HttpStatus.OK.value(), "B001", "저장이 왼료되었습니다."); |
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.
회의 통해서 내용 확인했으며 business 쪽 코드는 관련하여서 이용하다가
추후 실제 로그만 보고 문제 파악이 힘들다던지 하는 이유 발생한다면
디테일하게 작성하는 방향으로 가는 것이 좋겠습니다. (회의에서 이야기 했지만, 까먹지 않기 위한 코맨트 작성)
import com.kernel360.code.ErrorCode; | ||
import org.springframework.http.HttpStatus; | ||
|
||
public enum CommonBusinessCode implements ErrorCode { |
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.
어제 이야기를 복기하다가 어디서부터 어디까지가 비즈니스 코드인지 잘 모르겠다는 생각이 들었습니다. 단순 CRUD는 비즈니스 코드로 치지 않는 것처럼 이야기하시는 분들도 있고...
저희는 Service 빈 안에 있는 코드를 모두 비즈니스 코드라고 생각하면 될까요?
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.
일단 제가 생각한 기준은 ErrorCode을 구현한 코드(Enum)들은 Exception을 던질 때 사용하게 되고, 그 외 코드들은 BusinessCode를 구현해서 업무 로직에서 로그를 남길 때 사용하는 거였어요~!
커스텀 메시지를 관리하고자 할 때, BusinessCode가 필요하다고 생각해서 에러 처리 로직을 만들면서 같이 추가하게 되었습니다!
이름에서 오는 모호함이 있다면 클래스명을 수정하는 것도 고려할 수 있다고 생각해요
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.
LGTM2
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.
LGTM
회의때 봐서 빨리 리뷰 했습니다~
💡 Motivation and Context
1) 공통 에러 로직 및 Custom Exception 추가 2) API Response 관련 일관된 처리
🔨 Modified
🌟 More
📋 커밋 전 체크리스트
🤟🏻 PR로 완료된 이슈
closes #54 #55