refactor : project steps return index #295
Conversation
Eseas
requested review from
HiSeungmin,
K-KY,
jeongns2611,
kdh10806 and
taekminKwon
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ADMIN("ADMIN"), | ||
| CLIENT("client"), | ||
| DEVELOPER("developer"); | ||
|
|
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰입니다.
-
상수 표기법: 패치에서
ADMIN의 값을 "admin"에서 "ADMIN"으로 변경한 것은 좋습니다. 열거형 상수는 일반적으로 대문자로 표현하는 것이 관례이므로, 일관성을 유지하는 데 도움이 됩니다. -
버그 리스크: 현재 코드에는 즉각적인 버그 위험은 없어 보입니다. 그러나 이 변경이 기존 코드와 호환성이 있는지 확인해보아야 합니다. 만약 이 enum 값을 사용하는 다른 코드가 "admin"을 기대하고 있다면, 잠재적인 오류를 초래할 수 있습니다. 따라서 전체 프로젝트에서 이 값을 참조하는 부분을 점검하는 것이 필요합니다.
-
문서화 및 주석: 열거형 타입이 어떤 목적으로 사용되는지에 대한 주석이 없다면, 코드를 사용하는 다른 개발자들이 이해하는 데 어려움을 겪을 수 있습니다. 각 타입의 용도와 그에 대한 간단한 설명을 주석으로 추가하는 것이 좋습니다.
-
일관성 유지: 다른 enum 값들도 대문자 형태로 통일해 주면 가독성이 높아질 수 있습니다. 예를 들어
CLIENT와DEVELOPER도 대문자로 변경하는 것이 좋습니다. -
추가적인 타입: 만약 나중에 더 많은 사용자 유형이 추가될 가능성이 있다면, 현재 지정된
MemberType의 구조를 점검하고, 확장성 있게 설계해 둘 필요가 있습니다.
종합적으로 보았을 때 이 변경은 긍정적이지만, 기존 코드와의 호환성 체크 및 코드 문서화에 유의해야 할 필요가 있습니다.
| return new ProjectStepInfo(projectStep, order, projectChecklist); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
코드 리뷰에 대해 몇 가지 피드백을 드리겠습니다.
-
변경 사항 명확성:
ProjectStepInfo클래스의 생성자에Integer order매개변수를 추가한 것은 명확하게 보입니다. 그러나 코드의 다른 부분에서order값이 어떻게 사용되는지, 혹은 잘못된 값이 전달될 경우 발생할 수 있는 문제를 고려할 필요가 있습니다. 예를 들어,order가 null일 때의 처리를 추가하는 것이 좋습니다. -
정확한 데이터 타입 사용: 현재
order가Integer타입인데, 만약 순서를 반드시 0 이상의 정수로 제한해야 한다면Integer대신int를 사용하거나 유효성을 검증하는 로직을 구현하는 것이 필요합니다. -
가독성: 코드의 가독성을 높이기 위해, 가능하다면 매개변수의 의미를 명확하게 색깔을 입혀주는 것도 좋습니다. 예를 들어,
order대신 더 구체적인 이름을 사용하는 것이 코드를 읽는 사람에게 도움이 될 수 있습니다. -
예외 처리:
order값이 유효하지 않은 경우 (예: 음수인 경우 등) 어떻게 대처할 것인지에 대한 로직이 필요합니다. 이 부분에 대한 예외 처리를 추가하는 것이 좋습니다. -
불변성:
projectChecklist가 Immutable 컬렉션으로 전달되고 처리되는지 확인하십시오. mutable한 리스트가 전달될 경우 원본 데이터가 변경될 수 있으므로, 이를 방지하기 위한 방법을 고려해야 합니다.
결론적으로, 이 코드 패치는 기본적인 의도는 명확하지만, 입력 값에 대한 유효성 검사 및 예외 처리를 보완하면 더욱 안전하고 견고한 코드가 될 것입니다.
| index.getAndIncrement(), | ||
| checklists | ||
| ); | ||
| }).toList(); |
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다. 다음은 주목할 점과 개선 제안입니다.
-
AtomicInteger 사용의 적절성:
AtomicInteger를 사용하여 인덱스를 관리하고 있는데, 이것이 꼭 필요한지 검토할 필요가 있습니다. 만약 이 메소드가 멀티스레드 환경에서 호출되지 않는다면, 일반적인int변수를 사용하는 것이 더 단순하고 효율적일 수 있습니다.
-
스트림과 성능:
projectSteps.stream().map(...)사용은 간편하지만, 메소드 호출 및 리스트 생성을 포함한 반복적인 작업이 필요하므로, 대규모 데이터셋에서 성능 문제가 발생할 수 있습니다. 이 경우, 필요한 데이터만 미리 필터링하거나 외부 라이브러리를 사용하여 성능을 높일 수 있는지 검토하세요.
-
에러 처리:
checklistReader.read(stepInfo.getId())호출 결과에 대해 에러 처리나 예외 처리가 필요한지 확인해보세요. 만약 특정 ID에 대한 체크리스트가 없을 수 있다면, 이에 대한 처리가 추가적인 요구사항이 될 수 있습니다.
-
리턴 타입의 null 처리:
- 메소드가 null을 반환할 가능성이 있는지 체크하고, 명시적으로 null을 처리하는 로직을 고려하여 NullPointerException을 방지하는 것이 좋습니다.
-
주석 및 문서화:
- 코드의 동작을 명확히 하기 위해 주석을 추가하는 것이 좋습니다. 특히, 이 메소드가 다루는 비즈니스 로직이나 특정 흐름을 명확히 설명하는 주석이 있으면 후속 개발자에게 도움이 됩니다.
-
코드 정렬 및 가독성:
- 코드의 가독성을 높이기 위해, 들여쓰기를 정리하고 일관된 코드 스타일을 유지하는 것이 좋습니다. 예를 들어, 변수 선언과 초기화를 좀 더 뚜렷하게 할 수 있습니다.
종합적으로, 핵심 비즈니스 로직에 대한 이해가 필요하며, 성능, 에러 처리, 및 가독성을 우선 고려하여 코드를 개선할 수 있습니다.
| GetProjectStep.Response response = GetProjectStep.Response.create(projectId); | ||
|
|
||
| projectSteps.forEach(stepInfo -> { | ||
| response.add(stepInfo, checklistReader.read(stepInfo.getId())); | ||
| }); | ||
|
|
||
| return response; | ||
| } |
| developer, | ||
| projectType, | ||
| bnsManager | ||
| ); |
There was a problem hiding this comment.
코드 패치에 대한 리뷰를 진행하겠습니다. 전반적으로 코드를 간결하게 작성하려고 한 점은 긍정적입니다. 그러나 몇 가지 주의해야 할 사항과 개선할 수 있는 점들이 있습니다.
-
삭제된 매개변수 확인:
Company customer와Company developer매개변수가 삭제되었습니다. 이 두 개체가update메서드에서 필요하지 않다면, 메서드에 대한 호출에서도 이 매개변수들이 제거되었는지 확인이 필요합니다. 그렇지 않으면 호출부에서 오류가 발생할 수 있습니다. -
Null 체크:
update메서드 내에서startDate,endDate,finalApprovalDate,bnsManager등의 값이 null인지 여부를 체크하는 로직이 필요할 수 있습니다. 특히 이러한 값들이 데이터베이스에 저장된 경우, null 값을 허용하지 않는 경우도 있으므로 오류가 발생할 수 있습니다. -
테스트 커버리지: 이 변경 사항과 관련된 테스트 케이스가 충분한지 확인해야 합니다. 특히 매개변수가 삭제된 부분과 관련하여 기존의 기능이 정상적으로 작동하는지를 검증할 필요가 있습니다.
-
주석 추가: 코드의 변경 이유에 대한 주석을 추가하는 것도 좋습니다. 이는 다른 개발자들이 변경 사항을 이해하는 데 큰 도움이 됩니다.
-
코드 스타일: 코드 포매팅을 일관되게 유지하는 것이 좋습니다. 예를 들어, 각 메서드와 매개변수 간의 공백이 일관되게 유지된다면 가독성이 향상됩니다.
이러한 점들을 고려하여 코드를 개선해 나가면 더욱 안정적인 결과를 얻을 수 있을 것입니다.
| Project updatedProject = request.updateProject(project, projectType); | ||
| return adminProjectStore.store(updatedProject); | ||
| } | ||
|
|
There was a problem hiding this comment.
코드 패치에 대한 간단한 코드 리뷰를 진행하겠습니다.
주요 변경 사항:
customer와developer변수를 생성하는 코드를 주석 처리했습니다.updateProject메서드 호출에서customer와developer를 제거하고projectType만 전달하도록 수정했습니다.
잠재적인 버그 위험:
-
고객 및 개발자 정보 없음:
customer와developer관련 정보가 주석 처리됨에 따라, 이후 로직에서 이 정보가 필요한 경우NullPointerException이 발생할 수 있습니다. 특히updateProject메서드가 고객과 개발자 정보를 필요로 하는 경우 문제가 될 수 있습니다. 이 부분에서 예외 처리가 필요할 수 있습니다. -
기존 로직에 대한 이해 부족: 주석 처리된 코드가 왜 필요하지 않은지, 혹은 어떤 이유로 제거했는지에 대한 설명이 부족합니다. 이는 나중에 코드를 유지보수하는 다른 개발자에게 혼란을 줄 수 있습니다.
개선 제안:
-
주석에 이유 추가: 고객 및 개발자 정보를 주석 처리한 이유를 코드에 명확하게 주석으로 남기면 향후 유지보수 시 이해하는 데 도움이 될 것입니다.
-
유효성 검사 추가:
projectType외에도 필요한 정보가 있는지(예: 고객 및 개발자) 확인하는 유효성 검사 로직을 추가하는 것이 좋습니다. 이는 데이터 무결성을 보장하는 데 도움이 됩니다. -
명확한 메서드 시그니처:
request.updateProject메서드의 시그니처도 적절히 수정되었는지 확인해야 합니다. 인수와 반환 값이 변경된다면 해당 메서드를 호출하는 모든 코드도 업데이트해야 합니다. -
리팩토링 고려:
request.updateProject가 필요한 모든 정보를 통합해서 수정하는 것이 더 나을 경우, 요청 객체를 조정하여 필요한 정보를 모두 포함하게 할 수도 있습니다. 이를 통해 메서드 호출 시 적절한 정보를 전달할 수 있게 할 수 있습니다.
이러한 점들을 반영하면 코드의 안정성과 가독성을 높일 수 있습니다.
| this.developer = developer; | ||
| this.projectDescription = description; | ||
| this.projectType = ProjectType; | ||
| this.projectStatusCode = statusCode; |
There was a problem hiding this comment.
코드 패치에 대해 간단한 리뷰를 하겠습니다.
-
변수 삭제:
Company customer와Company developer절이 제거되었습니다. 이 부분이 실제 비즈니스 로직에 중요한 변수라면 이들이 필요 없어졌는지 확인해야 합니다. 만약 불필요하다면 괜찮지만, 기존 코드에서 의존하고 있던 부분은 없는지 확인하는 것이 필요합니다. -
일관성: 변수 이름이
ProjectType과 같이 대문자로 시작하는데, 이는 일반적으로 클래스나 인터페이스의 이름에 사용하는 방식입니다. Java에서 변수를 정의할 때는 소문자로 시작하는 것이 일반적이므로projectType으로 변경하는 것이 좋습니다. -
매개변수 이름: 코드의 가독성을 높이기 위해 매개변수의 이름을 더 명확하게 설정할 수 있습니다. 예를 들어,
bnsManager대신businessManager와 같이 풀어서 의미를 명확히 하는 것도 고려해볼 수 있습니다. -
Null 검사: 만약
name,description,statusCode와 같은 매개변수들이 null이 될 경우 코드에서 오류가 발생할 수 있습니다. 입력 값에 대한 검증이 필요합니다. -
주석 추가: 코드의 변경 사항이나 의도를 명확히 하기 위해 간단한 주석을 추가하는 것도 좋은 방법입니다. 예를 들어 왜 고객과 개발자 정보가 필요 없어졌는지 설명할 수 있습니다.
위의 사항들을 고려하여 수정하면 코드의 품질과 안정성을 높일 수 있을 것입니다.
바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!
✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)
🏆 구현 목표
ex)
bucket post 및 bucket list view 초기 구현
📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)
ex)
🔍 테스트 방법 (변경 사항을 확인하기 위한 테스트 방법을 기술합니다.)
ex)
npm test실행🛠️ 쓰이는 모듈
ex)
💬 기타 질문 및 특이 사항
ex) 백프 배포 nginx 검토