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] 공통 에러 처리 및 API 응답 관련 추가 #56

Merged

Conversation

linglong67
Copy link
Collaborator

💡 Motivation and Context

1) 공통 에러 로직 및 Custom Exception 추가 2) API Response 관련 일관된 처리


🔨 Modified

BusinessException으로 내부에서 정의한 에러코드 리턴

  • 아래 사용예시 참고해주세요 (커밋 대상 파일 X)
package com.kernel360.commoncode.service;
...

import static com.kernel360.commoncode.code.CommonCodeErrorCode.*;

@Service
public class CommonCodeService {
    ...
    
    public List<CommonCodeDto> getCodes_1(String codeName) {
        List<CommonCode> list = commonCodeRepository.findAllByUpperNameAndIsUsed(codeName, true);

        if (list.size() == 0) {
            throw new BusinessException(INVALID_COMMON_CODE_NAME);
        }

        return list.stream().map(CommonCodeDto::from).toList();
    }
}

API Response를 사용한 리턴

  • 아래 사용예시 참고해주세요 (커밋 대상 파일 X)
package com.kernel360.commoncode.controller;
...

import static com.kernel360.commoncode.code.CommonCodeBusinessCode.*;

@RestController
@RequestMapping("/commoncode")
public class CommonCodeController {
    ...

    @GetMapping("/{codeName}/test")
    public ResponseEntity<ApiResponse> getCommonCode_1 (@PathVariable String codeName){
        List<CommonCodeDto> codes = commonCodeService.getCodes_1(codeName);
        ApiResponse<List<CommonCodeDto>> response = ApiResponse.of(GET_COMMON_CODE_SUCCESS, codes);

        return new ResponseEntity<>(response, HttpStatus.OK);
    }
}


🌟 More

  • 작성한 방식에 대해 설명 후 그대로 적용할지 보완할지 논의 필요
  • Enum에서 사용하는 HTTP 응답코드 관련 '200', 'HttpStatus.OK.value()' 둘 중 어떤 방식 사용할지 논의 필요


📋 커밋 전 체크리스트

  • 추가/변경에 대한 단위 테스트를 완료하였습니다.
  • 컨벤션에 맞게 작성하였습니다.

🤟🏻 PR로 완료된 이슈

closes #54 #55

- GlobalExceptionHandler, BusinessException 추가
- ErrorCode 인터페이스로 추가
- BusinessCode 인터페이스로 추가
@linglong67 linglong67 added 🖥️ BackEnd 서버 관련 💡 Feature 새로운 기능 추가, 혹은 구현 labels Jan 15, 2024
@linglong67 linglong67 self-assigned this Jan 15, 2024
public String getMessage() {
return message;
}
}
Copy link

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:

  1. 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 to CommonCodeBusinessCode).

  2. It would be helpful to include comments or documentation explaining the purpose of the enum and its constants.

  3. 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.

  4. 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.

  5. 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;
}
}
Copy link

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:

  1. Make sure the package (com.kernel360.commoncode.code) matches your project's package structure.
  2. Consider providing a more descriptive message for the error code, such as "Invalid common code name: {code}" instead of just "존재하지 않는 공통코드".
  3. 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.

Copy link
Collaborator

@chan99k chan99k left a comment

Choose a reason for hiding this comment

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

LGTM

@chan99k chan99k self-requested a review January 15, 2024 09:24
- ex> 200 -> HttpStatus.OK.value()
public String getMessage() {
return message;
}
}
Copy link

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;
}
}
Copy link

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:

  1. 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 to CommonBusinessErrorCode or something similar to conform with established conventions.

  2. 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.

  3. 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", "저장이 왼료되었습니다.");
Copy link
Collaborator

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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

어제 이야기를 복기하다가 어디서부터 어디까지가 비즈니스 코드인지 잘 모르겠다는 생각이 들었습니다. 단순 CRUD는 비즈니스 코드로 치지 않는 것처럼 이야기하시는 분들도 있고...
저희는 Service 빈 안에 있는 코드를 모두 비즈니스 코드라고 생각하면 될까요?

Copy link
Collaborator Author

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가 필요하다고 생각해서 에러 처리 로직을 만들면서 같이 추가하게 되었습니다!
이름에서 오는 모호함이 있다면 클래스명을 수정하는 것도 고려할 수 있다고 생각해요

Copy link
Collaborator

@chan99k chan99k left a comment

Choose a reason for hiding this comment

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

LGTM2

Copy link
Collaborator

@HyunJunSon HyunJunSon left a comment

Choose a reason for hiding this comment

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

LGTM
회의때 봐서 빨리 리뷰 했습니다~

@linglong67 linglong67 merged commit 092ed9c into Kernel360:develop Jan 16, 2024
1 check passed
@gunsight1 gunsight1 mentioned this pull request Jan 17, 2024
2 tasks
@linglong67 linglong67 deleted the feature/response-common-process branch January 19, 2024 08:05
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🖥️ BackEnd 서버 관련 💡 Feature 새로운 기능 추가, 혹은 구현
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature :: API Response 처리 및 BusinessCode 추가
4 participants