fix : project detail fix #120
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}
|
|
||
| // TODO - 검색 api 추가 필요 | ||
| } No newline at end of file |
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰입니다.
-
주석 처리: 이전 코드에서 모든 내용을 주석 처리한 것은 비효율적입니다. 이후 다시 사용할 필요가 없으면 완전히 제거하고, 필요할 경우 커밋 메시지에 주석을 남겨두는 것이 좋습니다.
-
에러 처리:
getProjectDetail메소드에서는 프로젝트 ID에 해당하는 프로젝트가 존재하지 않을 경우에 대한 처리가 없습니다. 유효성 검사를 추가하거나, 해당 프로젝트가 없을 때의 예외 처리를 구현하는 것이 좋습니다. 예를 들어,projectService.getProjectDetail(projectId)가 null을 반환할 경우에는 404 Not Found 응답을 반환하도록 수정할 수 있습니다. -
정렬 조건: TODO로 남겨진 정렬 조건이 필요하다면, 작업의 우선순위를 정하고 관련 기능을 확실히 구현하는 것이 좋습니다. 사용자 경험을 위해 적절한 정렬 기능은 매우 중요할 수 있습니다.
-
API 문서화:
ProjectDocs를 통해 API 문서화를 진행하는 것은 좋지만, 구현되지 않은 메소드나 API에 대한 설명이 필요합니다. API의 사용법이나 응답 형식 등을 문서화할 수 있다면, 추후 유지보수와 팀 내 협업에 도움이 될 것입니다. -
비즈니스 로직 분리: 현재
getProjectDetail메소드에 비즈니스 로직이 포함되어 있습니다. 가능하다면 서비스 계층에서 로직을 처리하고, 컨트롤러에서는 요청과 응답의 변환에 집중하도록 하는 것이 좋습니다. 이는 코드의 가독성과 유지보수를 향상시킬 수 있습니다. -
종속성 주입:
@RequiredArgsConstructor를 이용한 생성자 주입은 좋은 방법이지만, 주입된 객체가 null이 아닌지 확인하는 로깅 또는 검사기능을 추가하는 것도 고려해볼 만합니다.
이 외에도 추가적인 피드백이나 개선이 필요한 부분이 있을 수 있으므로 팀 내에서 함께 논의해보는 것이 좋겠습니다.
| this.projectContact = name; | ||
| this.projectContactPhone = phoneNumber; | ||
| // this.projectContact = name; | ||
| // this.projectContactPhone = phoneNumber; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 제공하겠습니다.
-
주석 처리된 코드:
name과phoneNumber변수가 주석 처리되어 있습니다. 만약 이 변수들이 필요하지 않다면, 관련 코드를 완전히 제거하는 것이 더 깔끔합니다. 주석으로 남겨두면 코드의 가독성이 떨어질 수 있습니다. -
불필요한 주석: 주석으로 남겨두었더라도, 미래에 이 코드가 다시 사용될 가능성이 낮다면, 주석을 사용하기 보다는 과거의 커밋 기록에서 변경 이력을 확인하는 것이 나을 수 있습니다.
-
유지보수성:
ProjectDetail클래스의 인자 수가 줄어들면서, 필요한 정보를 추가하기 위해 다른 구조를 고려해야 할 수도 있습니다. 클래스의 설계를 확인하고 이 정보가 나중에 필요할 가능성에 대해 고민해 보는 것이 좋습니다. -
방어적 프로그래밍:
projectId,projectTypeName,projectName,projectDescription등과 같은 필수 인자가 null일 경우에 대한 처리 로직이 필요할 수 있습니다. 생성자에서 인자로 받은 값들을 검증하고, null인 경우 예외를 발생시키는 것이 좋습니다.
요약하자면, 코드 리뷰를 통해 주석 처리된 부분을 정리하고, 클래스 설계와 입력 값 검증에 대한 고민이 필요합니다.
| WHERE | ||
| p.id = :projectId | ||
| """) | ||
| Optional<GetProjectDetail.ProjectDetail> getProjectDetail(@Param("projectId") Long projectId); | ||
| } |
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
-
Optional 사용:
getProjectDetail메서드의 반환 타입을Optional<GetProjectDetail.ProjectDetail>로 변경한 것은 좋은 개선입니다. 이로 인해, 메서드 호출자가 반환 값이 없을 경우에 대해 보다 안전하게 처리할 수 있습니다. 이는 NullPointerException을 방지하는 데 도움이 됩니다.
-
주석 처리된 코드:
- 기존의 주석 처리된 코드가 남아 있는 것은 읽는 데 혼란을 줄 수 있습니다. 코드가 유지되지 않는다면 주석을 제거하고, 필요하다면 별도의 이슈 트래커에 기록하는 것이 좋습니다. 코드를 깔끔하게 유지하는 것이 중요합니다.
-
쿼리 최적화:
- 현재 쿼리에서는
Member엔티티에 대한 조인이 없습니다. 프로젝트 세부사항을 가져오는 데 멤버의 정보가 필요하다면, 해당 조인을 포함해야 할 것입니다. 만약 필요 없다면 삭제하여 쿼리를 간소화할 수 있습니다.
- 현재 쿼리에서는
-
메서드의 명확한 네이밍:
getProjectDetail이라는 메서드 이름은 세부사항을 가져온다는 의미를 명확하게 전달합니다. 그러나, 반환 타입의 이름에 대해 좀 더 구체적인 설명을 포함하면 좋을 것 같습니다. 예를 들어, 반환하는 DTO가 특정한 정보를 담고 있다면, 이를 메서드 이름에 반영할 수 있습니다.
-
예외 처리 고려:
- Optional을 사용함으로써 호출자가 null 대신에 값을 안전하게 처리할 수 있게 되지만, 호출 측에서 적절한 예외 처리를 추가하는 것이 좋습니다. 예를 들어, 값이 없을 때 사용자에게 친절한 메시지를 노출하는 등의 처리가 필요합니다.
-
쿼리 성능:
- 쿼리의 성능을 항상 고려해야 합니다. 필요하다면
@Query에서 주어진 쿼리의 실행 계획을 확인하여 인덱스 등의 최적화를 위한 조치를 고려하는 것이 좋습니다.
- 쿼리의 성능을 항상 고려해야 합니다. 필요하다면
종합적으로, 전반적으로 좋은 방향으로 개선되었으며, 위의 몇 가지 사항을 고려하여 더 나은 결과를 도출할 수 있을 것입니다.
| @@ -5,5 +5,5 @@ | |||
|
|
|||
| public interface ProjectService { | |||
| GetProjectList.Response getProjectList(Long memberId); | |||
| // GetProjectDetail.Response getProjectDetail(Long projectId); | |||
| GetProjectDetail.Response getProjectDetail(Long projectId); | |||
| } | |||
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
-
주석 제거: 이전에는
getProjectDetail메서드가 주석 처리되어 있었고, 현재는 주석이 제거되어 활성화되었습니다. 이 메서드가 사용될 수 있는 상황인지, 또는 실제로 필요한지 확인해야 합니다. 만약 아직 사용되지 않는다면, 주석을 계속 유지하는 것이 좋을 수 있습니다. -
Null 처리:
getProjectDetail메서드가Long projectId를 매개변수로 받기 때문에, 호출 시 null을 전달할 경우 문제가 발생할 수 있습니다.projectId에 대한 null 체크를 추가하는 것을 고려해 보십시오. -
명세서: 다른 메서드와 마찬가지로
getProjectDetail메서드의 반환 값에 대한 문서화가 필요합니다. 이를 통해 이해관계자가 메서드의 의도를 더 잘 알 수 있도록 하는 것이 좋습니다. -
예외 처리: 세부 프로젝트를 가져오는 과정에서 발생할 수 있는 예외 상황을 어떻게 처리할지에 대한 방침을 세워야 합니다. 예를 들어, 유효하지 않은
projectId에 대한 예외 처리를 추가할 수 있습니다. -
인터페이스 설계: 전체적인 인터페이스 설계가 잘 되어 있는지 검토해 보십시오. 필요하다면 메서드 간의 관계 또는 추가적인 메서드를 고려할 수 있습니다.
이런 점들을 확인하고 개선하면 코드 품질과 안정성을 높일 수 있을 것입니다.
| @Override | ||
| public GetProjectDetail.Response getProjectDetail(Long projectId) { | ||
| return projectReader.getProjectDetail(projectId); | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다.
-
주석 처리된 코드 제거: 이전에 주석 처리된
getProjectDetail메서드를 활성화하는 것으로 보입니다. 만약 해당 메서드가 더 이상 필요하지 않다면, 주석을 제거하는 것보다는 아예 삭제하는 것이 코드 유지보수성에 더 좋습니다. -
null 체크 및 예외 처리:
getProjectDetail메서드에서projectId가 null일 경우 또는projectReader.getProjectDetail(projectId)호출이 실패할 가능성에 대한 예외 처리가 없습니다. 이 부분에 대한 검증 로직을 추가하는 것이 좋습니다. 예를 들어, 다음과 같이 수정할 수 있습니다:if (projectId == null) { throw new IllegalArgumentException("projectId must not be null"); }
-
단위 테스트: 새로운 메서드를 추가하거나 기존 메서드를 활성화할 때는 반드시 관련 테스트가 있는지 확인해야 합니다. 특히
getProjectDetail의 경우, 다양한 케이스에 대한 테스트가 필요할 수 있습니다. -
API 문서화: 만약 이 메서드가 외부 API와 연결되어 있다면, 메서드에 대한 문서화를 추가하여 사용자가 어떻게 사용할 수 있는지에 대한 정보를 제공하는 것이 좋습니다.
제안된 변경 사항들을 통해 코드의 안정성과 가독성을 높일 수 있을 것입니다. 전체적으로 코드는 명확해 보이지만, 위의 개선 사항을 고려해 보시기 바랍니다.
| @@ -7,5 +7,5 @@ | |||
| public interface ProjectReader { | |||
| Project read(Long projectId); | |||
| GetProjectList.Response getProjectList(Long memberId, Long companyId); | |||
| // GetProjectDetail.Response getProjectDetail(Long projectId); | |||
| GetProjectDetail.Response getProjectDetail(Long projectId); | |||
| } No newline at end of file | |||
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 제공하겠습니다.
-
주석 처리 해제: 주석 처리된
getProjectDetail메소드를 활성화 한 것으로 보입니다. 이 메소드는ProjectReader인터페이스에서 프로젝트 세부 정보를 가져오는 데 필요한 중요 기능일 수 있으며, 이를 통해 코드의 가독성이 향상되었습니다. -
문서화 부족:
getProjectDetail메소드에 대한 설명이 부족합니다. 각 메소드에 어떤 역할이 있는지 주석으로 설명해 주는 것이 좋습니다. 그렇지 않으면 다른 개발자가 이 메소드를 사용하거나 유지보수하는 데 어려움을 겪을 수 있습니다. -
매개변수 검증:
getProjectDetail메소드에 대한 매개변수 검증 로직이 추후 구현될 경우,projectId가 null이거나 유효하지 않은 값일 경우에 대한 처리도 고려할 필요가 있습니다. -
리턴 값 검증: 메소드를 구현할 때, 반환값이 null일 경우의 처리도 고려해야 합니다. 예를 들어, 해당 프로젝트가 존재하지 않을 경우 적절한 예외를 던지거나 기본값을 리턴할 수 있는 방법을 생각해 볼 필요가 있습니다.
-
파일 끝의 새 줄: 마지막 줄에 개행이 없습니다. 일부 스타일 가이드에서는 파일의 끝에 빈줄을 두는 것을 권장하므로, 추가하는 것이 좋습니다.
총체적으로, 패치가 간단하지만 필요한 기능을 추가하는 데 있어 긍정적인 방향으로 보입니다. 각 메소드의 역할을 명확히 하고, 예외 처리를 고려한다면 코드의 안정성도 높일 수 있을 것입니다.
| projectStepRepository.findStepProcessRate(projectId), | ||
| checkRequestRepository.findAllApplicationLists(projectId) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 제공하겠습니다.
개선 사항 및 버그 위험
-
Optional 처리:
projectRepository.getProjectDetail(projectId)에서 반환되는 결과가Optional인 경우,orElseThrow를 사용하여 예외를 처리하는 것은 좋은 접근입니다. 그러나 이 방식이 항상 적절한지는 확인이 필요합니다. 예외 발생 시의 처리 로직이 해당 서비스 전반에 걸쳐 일관되게 이루어지는지 검토해야 합니다.
-
트랜잭션 설정:
@Transactional(readOnly = true)어노테이션을 붙인 것은 적절하며, 데이터베이스의 성능 최적화를 위해 좋은 선택입니다. 데이터가 읽기 전용이면 해당 어노테이션을 사용하는 것이 좋습니다.
-
예외 처리:
BusinessException을 사용할 때, 어떤 상황에서 이 예외가 발생하는지 문서화하는 것이 좋습니다. 또한, 예외 처리 로직이 애플리케이션의 다른 부분과 일관성을 유지하고 있는지도 확인해야 합니다.
-
메서드 이름:
getProjectDetail이라는 메서드 이름이 기능을 잘 설명하고 있습니다. 하지만, 메서드가 어떤 종류의 상세 정보를 반환하는지 더 구체적으로 나타내는 이름으로 개선할 수 있습니다. 예를 들어,getProjectDetailWithSteps처럼 구체화할 수 있습니다.
-
주석 처리된 코드 제거:
- 기존의 주석 처리된 코드(가령,
getProjectDetail앞 부분)는 불필요한 코드로 보입니다. 코드 관리 관점에서 불필요한 코드(chunk)는 제거하는 것이 좋습니다.
- 기존의 주석 처리된 코드(가령,
-
에러 코드 관리:
ErrorCode.PROJECT_NOT_FOUND와 같은 에러 코드는 중앙집중적으로 관리될 필요가 있습니다. 에러 코드에 대한 문서화도 검토해보세요.
결론
전체적으로 이 패치는 긍정적인 변화를 포함하고 있으며, 향후 유지보수 과정에서도 예외와 상태 관리를 잘 수행할 수 있도록 지원합니다. 위에서 언급한 몇 가지 점을 고려하여 코드를 개선하면 더욱 견고하고 효율적인 코드가 될 것입니다.
바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!
✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)
🏆 구현 목표
📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)
🔍 테스트 방법 (변경 사항을 확인하기 위한 테스트 방법을 기술합니다.)
🛠️ 쓰이는 모듈
ex)
💬 기타 질문 및 특이 사항
ex) 백프 배포 nginx 검토