Feature/checklist application get api #225 #229
Conversation
Eseas
requested review from
taekminKwon,
K-KY,
kdh10806,
HiSeungmin and
jeongns2611
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| APPROVED("승인"), | ||
| REJECTED("반려"); | ||
|
|
||
| private final String description; | ||
| } No newline at end of file |
There was a problem hiding this comment.
코드 패치에 대한 리뷰를 진행하겠습니다.
-
코드의 의도:
ApprovalStatus열거형에 한국어 설명을 추가한 것으로 보입니다. 이는 가독성을 높이고, 사용자가 상태의 의미를 쉽게 이해할 수 있게 합니다. -
버그 위험 요소:
- 현재 코드는 적절하게 작성된 것처럼 보이며 기능적으로 문제는 없어 보입니다. 하지만
description필드의 접근 메소드는@Getter어노테이션 덕분에 잘 작동할 것입니다. 이로 인해description에 안전하게 접근할 수 있습니다.
- 현재 코드는 적절하게 작성된 것처럼 보이며 기능적으로 문제는 없어 보입니다. 하지만
-
개선 제안:
- 열거형의 각 상태에 대한 설명을 추가하는 것은 좋은 접근이지만, 만약 후에 추가적인 상태가 필요해진다면 이 코드가 어떻게 확장될지 고려해야 합니다. 필요에 따라
toString()메소드를 오버라이드하여 각 상태의 설명을 반환하도록 할 수 있습니다. - 각 상태에 대해 명확한 aliase를 정의하면, 이후의 코드에서
ApprovalStatus를 사용할 때 직관성을 더욱 높일 수 있을 것입니다. - 열거형의 명확성을 높이기 위해 JavaDoc 주석을 활용하여 각 상태가 무엇을 의미하는지 설명하면 좋겠습니다.
- 열거형의 각 상태에 대한 설명을 추가하는 것은 좋은 접근이지만, 만약 후에 추가적인 상태가 필요해진다면 이 코드가 어떻게 확장될지 고려해야 합니다. 필요에 따라
-
코드 스타일:
No newline at end of file경고가 있습니다. 파일의 끝에 공백 줄을 추가하여 이러한 경고를 없앨 수 있습니다. 이는 코드 유지보수 측면에서 권장되는 좋은 관행입니다.
전반적으로, 이 패치는 잘 작성되었으며 기능적으로 문제는 없지만, 코드 확장성 및 주석 추가와 같은 몇 가지 개선 요소를 고려하면 더욱 좋을 것입니다.
| projectChecklistFacade.getChecklistAllApplications(checklistId)) | ||
| ); | ||
| } | ||
|
|
||
| } No newline at end of file |
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다.
-
메서드 이름 변경: 기존의
getProjectChecklistApplication메서드를 수정하여 반환 타입이GetApplication.Response로 변경되었습니다. 이 변경이 데이터 흐름 및 처리 방법에 적합한지 확인해야 합니다. 만약GetApplication이 적절한 타입이라면 문제가 없지만, 문맥상 혼란을 줄 수 있으므로 충분한 주석이나 문서화가 필요합니다. -
주석: 추가된 주석은 함수의 목적을 명확히 설명하고 있습니다. 하지만 주석에서 "모든 승인 요청 목록"을 언급하고 있으므로, 이 메서드가 반환할 수 있는 데이터의 구조나 반환 형식에 대한 구체적인 설명을 추가하는 것이 좋습니다. 향후 유지보수를 고려할 때 명확한 주석은 도움이 됩니다.
-
HTTP 응답 상태 코드: 두 메서드에서
SuccessCode.OK.getStatusCode()를 사용하여 응답 상태 코드를 설정하고 있습니다. 이 방법은 일관성 있지만, 특정 오류 상황을 처리할 전략도 포함되어야 합니다. 실패 상황에 대한 핸들링도 고려해야 합니다. -
예외 처리: 새로운
getChecklistApplication메서드는 예외 처리가 없습니다. 예를 들어,checklistId에 대한 유효성 검사 및 해당 체크리스트가 존재하지 않을 때의 처리가 필요합니다. 이를 통해 안정성을 높이고, 클라이언트에게 좀 더 유용한 오류 메시지를 제공할 수 있습니다. -
코드 포매팅: 코드 블록의 가독성을 높이기 위해 적절한 줄 바꿈과 페어 인덴트를 사용하는 것이 좋습니다. 특히, 코드 변경 부분의 마지막에 newline이 없는 것은 일반적으로 좋지 않은 관행입니다. 새로운 파일의 끝에는 항상 newline을 추가하는 것이 권장됩니다.
이러한 점들을 고려하면 코드의 안정성 및 가독성을 높일 수 있을 것입니다. 추가적인 테스트 케이스를 작성하여 새로운 메서드가 의도한 대로 작동하는지 확인하는 것도 좋은 방법입니다.
| projectCheckRequestService.getChecklistApplications(applications); | ||
|
|
||
| return GetChecklistApplication.Response.toDto(result); | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
-
버그 위험성:
getChecklistAllApplications메서드에서checklistId에 대한 유효성 검사가 없는 것 같습니다. 이 ID가 유효하지 않거나 존재하지 않을 경우,projectChecklistService.getChecklist(checklistId)호출이 NullPointerException을 발생시킬 수 있습니다. ID가 유효한지 확인하는 로직을 추가하는 것이 좋습니다.getAllCheckRequestByChecklistId메서드를 호출하기 전에checklist객체가 null인지 검사해야 합니다. nullptr에 대한 검사가 없다면 예외가 발생할 수 있습니다.
-
개선 제안:
getChecklistAllApplications메서드의 로직을 간결하게 유지하기 위해 에러 처리 및 유효성 검사 로직을 별도의 헬퍼 메서드로 분리하는 것을 고려해보세요. 이렇게 하면 메서드의 가독성이 높아지고 관리가 용이해집니다.- 주석으로 "Service로 이동시켜야 할까?"라 언급하셨는데, 이 부분이 어떤 서비스의 책임인지 명확히 하고 이를 기반으로 적절한 서비스 클래스로 로직을 이동 시킬 수 있는지 고민해보는 것이 좋습니다. 이렇게 함으로써 SRP(Single Responsibility Principle)를 더 잘 준수할 수 있습니다.
- 가능하다면,
GetChecklistApplication.Response.toDto(result)메서드의 내용에 대해서도 검토하여, DTO 변환 시 어떤 Validation이나 Exception Handling이 필요한지 고려해야 합니다.
-
코드 컨벤션:
- 패키지와 클래스 임포트의 순서를 일관성 있게 정리하는 것이 바람직합니다. 예를 들어, 빈(BEAN) 관련 임포트, 서비스 관련 임포트 등을 그룹화하여 읽기 쉽게 만들어 보세요.
이 외에도 코드 테스트 및 성능 최적화를 위해 좀 더 구체적인 목적으로 단위 테스트를 추가하는 것도 좋은 방법입니다.
| return new ApplicationResult(checkResult); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 패치를 검토한 결과, 다음과 같은 사항을 고려해 볼 수 있습니다.
-
불필요한 NoArgsConstructor:
@NoArgsConstructor가 여러 클래스에 정의되어 있으나, 해당 클래스들이 모든 필드를 초기화하는 생성자를 가지고 있습니다. 이러한 경우, 관례적으로 기본 생성자가 필요할 가능성이 낮습니다. 기본 생성자가 필요한 경우에도, 필드를 초기화하지 않는 상태로 남기면 의도하지 않은 오류를 초래할 수 있습니다. -
생성자 접근 제어자: 생성자들이
private로 정의되어 있어 외부에서 인스턴스를 생성할 수 없습니다.toDto메서드를 통해 인스턴스를 생성할 수 있게 한 것은 좋지만, 장기적으로는 생성자 접근 제어자를protected로 변경하는 방법도 고려해볼 수 있습니다. 이런 방식은 상속을 통한 확장성을 고려할 때 유용할 수 있습니다. -
중복 코드 제거:
ChecklistApplication클래스 내 두 개의 생성자가 비슷한 내용을 가지고 있습니다. 중복된 코드를 피하기 위해 공통 로직을 따로 메서드로 분리하면 가독성도 높이고 유지보수도 용이합니다. -
Null 체크:
CheckRequest및CheckResult에서 값을 가져오도록 하는 메서드들이 있어, 이 객체들이 null일 경우 NullPointerException이 발생할 수 있습니다. 이에 대한 검증 로직을 추가하는 것이 좋습니다. -
DTO의 불변성: DTO는 불변 객체로 만드는 것이 일반적입니다. 이에 따라 필드를
final로 선언하고, 외부에서 세터를 통해 상태가 변경되지 않도록 하는 것이 좋습니다. -
지정된 패키지 구조: 클래스 구조가 명확하지만,
dto패키지 내에 DTO 외의 다른 엔티티와 관련된 코드가 포함될 시 의도를 명확히 하기 어렵습니다. DTO와 관련된 다른 클래스를 생성하여 관리하는 것이 좋습니다. -
적절한 주석 추가: 각 클래스와 메서드에 대한 주석이 없는데, 이로 인해 코드의 의도가 불분명해질 수 있습니다. 각 필드와 메서드의 목적을 설명하는 주석을 추가하면 코드 가독성이 향상될 수 있습니다.
-
테스트 케이스: 새로 추가된 기능에 대해 단위 테스트를 작성하여 의도한 대로 작동하는지 검증하는 것이 중요합니다. 특히 NullPointerException이 발생할 수 있는 부분에 대한 테스트를 추가하는 것이 좋습니다.
이와 같은 사항을 고려하여 코드의 가독성, 안정성, 확장성을 높이는 방향으로 보완하시면 좋겠습니다.
| @@ -31,4 +31,6 @@ public interface CheckRequestRepository extends JpaRepository<CheckRequest, Long | |||
| List<GetProjectDetail.ChecklistApplicationList> findAllApplicationLists(Long projectId); | |||
|
|
|||
| Optional<CheckRequest> findByIdAndIsActive(Long id, YesNo isActive); | |||
|
|
|||
| List<CheckRequest> findByChecklistIdAndIsActive(Long checklistId, YesNo isActive); | |||
| } | |||
There was a problem hiding this comment.
이 코드 패치에 대한 간단한 코드 리뷰를 하겠습니다.
-
버그 리스크:
findByChecklistIdAndIsActive메서드에서 체크리스트 ID와 활성 상태를 기반으로CheckRequest목록을 검색하고 있는데, 이 메서드의 이름은 명확하여 의도한 기능이 잘 드러나고 있습니다. 하지만, 만약checklistId나isActive값이 null일 경우 어떻게 처리하는지에 대한 고려가 필요합니다. 예를 들어, null 값을 전달했을 때의 반응이 명확하지 않으면 예기치 않은 결과를 초래할 수 있습니다.
-
개선 사항:
YesNo타입이 어떤 값을 허용하는지에 대한 문서화가 필요합니다. 특히, 이 코드를 사용하는 개발자들이 해당 타입의 용도를 쉽게 이해할 수 있도록 주석이나 설명을 추가하는 것이 좋습니다.- 만약
List<CheckRequest>가 null을 반환할 수 있다면 이를 명시적으로 처리하는 것이 좋습니다. 예를 들어, 반환값을Optional<List<CheckRequest>>로 감싸서 호출자가 null 체크를 쉽게 할 수 있도록 할 수 있습니다. - 메서드에 대한 단위 테스트를 추가하여 다양한 입력에 대한 처리가 올바르게 이루어지는지 검증하는 것이 좋습니다.
-
타입 안전성:
- 현재
findByChecklistIdAndIsActive메서드가 어떤 종류의 체크리스트를 처리할 것인지에 대한 정보가 부족합니다. 체크리스트 ID가 특정 범위의 값이 아닐 경우 예외를 던지거나, 비즈니스 로직을 통해 검증하는 처리가 필요할 수 있습니다.
- 현재
이러한 점들을 고려하면 코드의 안정성과 가독성을 높일 수 있습니다. 추가적인 질문이나 도움이 필요하시면 말씀해 주세요.
| return GetChecklistApplication.ChecklistApplication.toDto(application); | ||
| } | ||
| }).toList(); | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 패치에 대한 리뷰를 진행하겠습니다.
코드 리뷰
-
의존성 주입:
CheckResultReader를 추가했는데, 이 클래스의 역할과 책임에 대한 주석이나 문서가 있으면 좋겠습니다. 모든 의존에 대한 설명이 필요할 수 있습니다.
-
예외 처리:
getChecklistApplications메서드에서BusinessException을 잡고 기본 DTO를 반환하는 방식은 일반적으로 좋은 접근이지만, 예외를 로깅하지 않고 있습니다. 로그를 추가하여 문제 발생 시 추적할 수 있도록 하는 것이 좋습니다.
log.error("Error reading check result for application ID: {}", application.getId(), e);
-
리스트 반환:
getChecklistApplications메서드는List<CheckRequest>를 매개변수로 받고,GetChecklistApplication.ChecklistApplication타입의 리스트를 반환합니다. 이 메서드의 이름에get이 포함되어 있는데, 반환되는 데이터는 리스트의 변환 결과이므로 메서드 이름을convertToChecklistApplications와 같이 변경하는 것이 더 적절할 수 있습니다.
-
매개변수 검증:
getAllCheckRequestByChecklistId와getChecklistApplications메서드의 경우,checklist와applications매개변수가 null일 경우 예외를 던지도록 검증하는 로직을 추가하는 것이 좋습니다. 이는 NullPointerException을 예방하는 데 도움이 됩니다.
-
Stream API 사용:
getChecklistApplications에서 스트림 API를 사용하는 것은 가독성에 도움이 되며, 잘 사용되었습니다. 그러나, 예외 발생 시 기본 DTO로 대체하는 로직은 명확성과 유지보수 측면에서 고려할 필요가 있습니다. 예외 처리 로직을 명확히 하기 위해 별도의 메서드를 만드는 것도 고려해볼 수 있습니다.
-
주석 추가:
- 각 메서드에 주석을 추가하여 해당 메서드의 목적과 사용법을 설명하는 것이 좋습니다. 이는 코드 유지보수 및 팀원 간의 커뮤니케이션에 도움을 줄 수 있습니다.
결론
위의 피드백을 반영하면 코드의 안전성 및 가독성, 유지보수성이 향상될 것입니다. 각 객체와 의존성이 어떻게 동작하는지에 대한 명확한 설명을 추가하는 것도 장기적으로 큰 도움이 될 것입니다.
| @@ -14,7 +14,7 @@ | |||
| import java.util.List; | |||
|
|
|||
| public interface ProjectChecklistService { | |||
| GetProjectChecklistApplication.Response getApplicationDetail(Long applicationId); | |||
| GetApplication.Response getApplicationDetail(Long applicationId); | |||
|
|
|||
| List<FileMetadataDto> getApplicationFiles(Long applicationId); | |||
|
|
|||
There was a problem hiding this comment.
코드 패치에 대한 간단한 검토를 아래와 같이 진행했습니다.
-
변경된 메서드 반환 타입:
GetProjectChecklistApplication.Response에서GetApplication.Response로 변경되었습니다. 이 변경이 의도한 바가 명확한지 확인이 필요합니다. 만약 두 클래스 간에 관계가 없거나 차이가 있다면, 호출하는 쪽에서 문제가 발생할 수 있습니다. 변경의 이유와 문서화를 통해 파악할 수 있도록 하는 것이 좋습니다. -
메서드 이름의 일관성:
getApplicationDetail메서드의 이름은Application에 대한 세부 정보를 가져오는 것으로 보입니다. 하지만, 반환 타입이 변경되면서 메서드의 목적과 반환 값이 일관성을 유지하는지 확인해야 합니다. 일관성 있는 명명 규칙을 통해 코드의 가독성을 높일 수 있습니다. -
예외 처리:
applicationId가 null이거나 존재하지 않는 ID일 경우의 예외 처리가 필요합니다. 메서드를 호출하는 쪽에서 이러한 상황을 처리할 수 있도록 명시적인 예외를 정의하는 것이 좋습니다. -
주석 추가: 변경 사항이나 메서드의 목적을 명확히 설명하는 주석을 추가하는 것이 좋습니다. 특히 인터페이스는 다양한 구현체에서 사용될 수 있으므로, 문서화가 특히 중요합니다.
-
의존성 확인:
GetApplication.Response와 관련된 클래스나 메서드의 의존성을 확인해야 합니다. 만약 해당 타입에 변화가 생긴다면, 이 인터페이스를 사용하는 모든 구현체에서도 적절히 수정해야 할 것입니다.
이 정도로 개선 사항과 잠재적 버그 리스크를 정리할 수 있을 것 같습니다. 코드를 유지보수하는 데 도움이 되시길 바랍니다.
| CheckRequest checkRequest = checkRequestReader.read(applicationId); | ||
| return GetProjectChecklistApplication.Response.toDto(checkRequest); | ||
| return GetApplication.Response.toDto(checkRequest); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
코드 리뷰 결과를 아래와 같이 정리했습니다.
-
메서드 이름 변경:
getApplicationDetail메서드는 반환 타입이GetProjectChecklistApplication.Response에서GetApplication.Response로 변경되었습니다. 이 변경 사항이 기존 코드에서 잘 반영되고, 원하는 결과를 반환하는지 확인이 필요합니다. 사용되는 다른 부분에서의 의존성이 있는지 확인하는 것도 중요합니다. -
객체 변환:
CheckRequest객체를GetApplication.Response.toDto메서드를 통해 변환하고 있습니다. 이 메서드가CheckRequest를 처리하는 데 적절한지 확인해야 하며, 변환 로직이 필요한 모든 데이터를 포함하고 있는지 확인해야 합니다. 만약 일부 필드가 누락되면, 애플리케이션의 동작에 문제가 생길 수 있습니다. -
예외 처리: 현재 코드에서
applicationId가 유효하지 않거나checkRequestReader.read()가 실패할 경우에 대한 예외 처리가 없습니다. 이를 보완하기 위해 예외 처리 로직을 추가하는 것이 좋습니다. -
주석 및 문서화: 코드 변경에 대한 설명이나 매서드의 기능을 설명하는 주석이 부족합니다. 코드 변경이 발생했을 때, 기존 코드 및 새로운 코드의 의도를 명확하게 설명할 수 있는 주석을 추가하는 것이 좋습니다.
-
테스트 케이스: 변경된 코드에 대한 유닛 테스트 또는 통합 테스트가 적절하게 작성되었는지 확인해야 합니다. 특히, 반환 값이 예상과 일치하는지 확인하는 테스트가 필요합니다.
위의 사항들을 고려하여 코드를 점검하고, 필요시 수정하시는 것을 권장합니다.
| public interface CheckRequestReader { | ||
| CheckRequest read(Long applicationId); | ||
| List<CheckRequest> readByChecklistId(Long checklistId); | ||
| } |
There was a problem hiding this comment.
코드 패치를 확인해보니 몇 가지 개선점과 버그 리스크가 보입니다.
-
주석 추가: 인터페이스에 새로운 메서드를 추가했지만, 해당 메서드의 용도나 사용 방법에 대한 주석이 없습니다. 코드의 가독성을 높이기 위해 메서드에 대한 간단한 설명을 추가하는 것이 좋습니다.
-
예외 처리:
readByChecklistId메서드에서checklistId가 Null일 경우 또는 해당 checklistId에 대한 데이터가 없을 경우에 대한 처리가 없습니다. 이러한 경우에 대한 예외 처리를 구현하는 것이 좋습니다. -
네이밍 컨벤션: 메서드 이름이 명확하게 기능을 설명하고 있지만,
readByChecklistId보다findByChecklistId와 같은 네이밍을 사용하는 것이 더 직관적일 수 있습니다. -
List 반환 타입:
List<CheckRequest>를 반환할 때, 빈 리스트를 반환하는 것이 좋습니다. 만약 데이터가 없을 경우 Null을 반환하면, 호출하는 쪽에서 NullPointerException이 발생할 수 있습니다. 따라서 빈 리스트를 반환하도록 구현하는 것을 추천합니다. -
테스트 코드: 추가된 메서드에 대한 유닛 테스트가 있어야 합니다. 새로운 기능이 올바르게 작동하는지 확인하는 테스트 케이스를 작성하는 것이 중요합니다.
이와 같은 점들을 고려하여 코드를 개선하면 더 안정적이고 가독성이 좋은 코드를 만들 수 있을 것입니다.
| @Override | ||
| public List<CheckRequest> readByChecklistId(Long checklistId) { | ||
| return checkRequestRepository.findByChecklistIdAndIsActive(checklistId, YesNo.YES); | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다.
코드 변경 사항 요약
java.util.List를 import 함.readByChecklistId메소드를 추가하여 특정 checklistId에 대한 활성 CheckRequest를 조회하는 기능을 추가함.
개선 사항 및 버그 위험
-
Null 체크:
readByChecklistId메소드에서checklistId가 null일 경우 어떤 처리가 이루어지는지 명시되어 있지 않습니다. 이를 방지하기 위해, null 체크 후 적절한 예외를 던지도록 하는 것이 좋습니다.
if (checklistId == null) { throw new IllegalArgumentException("checklistId must not be null"); }
-
반환값 문서화:
readByChecklistId메소드의 반환값에 대한 설명이 부족합니다. 문서화(자바 독 주석)를 추가하여 메소드의 기능과 반환값에 대한 정보를 명확히 하는 것이 좋습니다.
-
오류코드 상수화:
readByChecklistId메소드에서 발생할 수 있는 오류에 대해 적절한 오류 코드와 예외 처리를 고려해야 합니다. 예를 들어, 주어진 checklistId로 활성화된 요청이 없을 경우 적절한 예외를 던지는 것이 좋습니다.
-
테스트 케이스:
- 새로운 메소드에 대한 유닛 테스트를 반드시 작성해야 합니다. 다양한 checklistId에 대해 올바른 결과가 반환되는지, null 또는 잘못된 값이 들어왔을 때 적절히 오류가 발생하는지를 체크하는 테스트 케이스가 필요합니다.
-
남은 틈새:
- 특정 repository 메소드가 데이터베이스에서 올바르게 조회되는지 확신할 수 없습니다. 데이터베이스 쿼리의 성능과 정확성을 확인할 필요가 있으며, 만약 대량의 데이터를 다루는 경우 페이징 처리나 최적화도 고려해야 합니다.
전체적으로, 코드는 새로운 기능을 잘 추가하고 있습니다. 그러나 위와 같은 세세한 부분을 보강하여 코드의 안정성을 높이는 것이 좋습니다.
바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!
✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)
🏆 구현 목표
ex)
bucket post 및 bucket list view 초기 구현
📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)
🔍 테스트 방법 (변경 사항을 확인하기 위한 테스트 방법을 기술합니다.)
ex)
npm test실행🛠️ 쓰이는 모듈
ex)
💬 기타 질문 및 특이 사항
ex) 백프 배포 nginx 검토