-
Notifications
You must be signed in to change notification settings - Fork 4
Fix/put project tag fix #299
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
Conversation
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.
Code review by ChatGPT
| projectTypeId = project.getProjectType().getId(); | ||
| projectStatusCode = project.getProjectStatusCode(); | ||
| bnsManager = project.getBnsManager(); | ||
| contractNumber = project.getContractNumber(); |
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.
코드 리뷰에 대해 말씀드리겠습니다.
-
변경 사항:
projectTypeName필드가String에서Long형식의projectTypeId로 변경되었습니다. 이는 프로젝트 유형의 ID를 저장하기 위한 변경으로 보입니다. 이 변경은 데이터 구조를 보다 명확하게 하고, 데이터베이스의 외래 키 관계를 더 잘 반영할 수 있습니다.
-
버그 리스크:
- 변경 전후 코드에서
project.getProjectType().getProjectTypeName()에서project.getProjectType().getId()로 변경되었는데, 이 부분에서 어떤 의도를 가지고 변경되었는지 명확히 확인할 필요가 있습니다. 만약projectTypeName을 사용하던 기존의 로직이 있다면, 이를 모두 수정해야 합니다. projectTypeId를 사용함에 있어 적절한 값이 항상 존재하는지를 체크하는 로직이 필요한지 고려해야 합니다. 예를 들어,projectType이 null인 경우를 처리하지 않으면 NullPointerException이 발생할 수 있습니다.
- 변경 전후 코드에서
-
개선 사항:
Response클래스의 필드들에 대해@Getter,@Setter와 같은 Lombok 어노테이션을 사용하여 코드의 양을 줄이는 것을 고려할 수 있습니다.- 필드에 대한 접근을 제한하여 불변성을 높이기 위해
final키워드를 사용하는 것도 좋은 방법입니다. developerCompanyName,bnsManager,contractNumber와 같은 필드는 nullable일 수 있는데, 이러한 경우에 대한 명시적 처리를 고려하는 것이 좋습니다. 예를 들어, 액세서 메서드를 통해 null 여부를 체크하는 기능을 추가할 수 있습니다.
-
코드 스타일:
- 코드의 주석은 한글로 잘 작성되어 있는 것 같습니다. 이는 해당 코드를 이해하는 데 도움이 됩니다. 그러나 일관성을 위해 클래스 및 메서드의 설명 주석 등을 추가해서 코드의 가독성을 높이는 것이 좋습니다.
이와 같은 점들을 고려하여 코드를 수정하거나 개선하는 것이 좋겠습니다.
| protected Authorization projectAuthorization; | ||
|
|
||
| public abstract ProjectAuthorization toEntity(Project project, Member member); | ||
|
|
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.
코드 리뷰에 대해 몇 가지 제안과 잠재적인 버그 위험을 말씀드리겠습니다.
-
타입 변경에 대한 확인:
protected String projectAuthorization;를protected Authorization projectAuthorization;으로 변경했습니다. 이 변경으로 인해projectAuthorization의 데이터 타입이 String에서 Authorization으로 변경되었습니다. 기존 코드에서 이 변수를 사용하는 다른 부분(예: Getter, Setter, 데이터베이스 맵핑 등)에서 문제가 발생할 수 있으므로, 이러한 변경을 적용하기 전에 해당 사용처들을 모두 확인해야 합니다.
-
Import 관리:
Authorization클래스를 추가로 임포트했습니다. 이 변경 후에 사용하지 않는 임포트 문이 없도록 관리하는 것이 좋습니다. 코드의 가독성을 높이고 불필요한 혼동을 피할 수 있습니다.
-
Null 안전성:
memberId,memberName,memberType,projectAuthorization필드에 대한 Null 체크가 필요할 수 있습니다. 이러한 필드들이 null일 경우 발생할 수 있는 오류를 예방하기 위해서 Null 처리 로직을 추가하는 것이 좋습니다.
-
주석 추가:
- 이 클래스가 어떠한 용도로 사용되는지에 대한 주석이 부족해 보입니다. 나중에 코드를 읽는 사람이나 본인에게 도움이 될 수 있도록 각 메소드와 필드에 대한 간단한 설명 주석을 추가하는 것이 좋습니다.
-
추상 메소드:
toEntity(Project project, Member member);메소드가 추상 메소드로 정의되어 있습니다. 이 메소드를 구현하는 클래스에서는 이 메소드를 반드시 구현해야 하며, 그 과정에서 매개변수로 넘어오는project와member의 null 체크 또한 필요합니다.
이와 같은 점들을 고려하여 코드의 안정성과 가독성을 높일 수 있을 것입니다. 코드를 개선하기 위해 위 사항들을 점검해 보시기 바랍니다.
| List<ProjectTag> findByProjectIdAndIsActive(Long projectId, YesNo isActive); | ||
|
|
||
|
|
||
| } |
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.
코드 패치에 대한 간단한 리뷰를 하겠습니다.
-
의미 있는 공백 제거: 두 개의 공백 행이 추가되었습니다. 이는 코드의 가독성을 저하시킬 수 있으며, 불필요한 공백은 지우는 것이 좋습니다.
-
메소드 명명 규칙: 메소드의 명칭에 대해 간단히 확인해 보아야 합니다.
findByProjectIdAndIsActive메소드 이름은 JPA 쿼리 메소드를 따라 잘 정의되어 있지만, 메소드의 반환 타입이List<ProjectTag>로 설정되어 있습니다. 만약 반환되는 데이터가 비어있을 경우에 대한 처리가 필요할 수 있습니다. -
Boolean 타입 사용 고려:
isActive매개변수는YesNo타입인데, 이는 일반적으로Boolean타입으로 충분히 대체할 수 있습니다. 필요에 따라YesNo를 사용하는 것이 괜찮은 경우도 있지만, 코드의 단순성을 위해Boolean을 사용하는 것이 더 직관적일 수 있습니다. -
예외 처리: 메소드가 예외를 던질 수 있는 경우 (예: DB 접근 문제) 해당 상황에 대한 문서화 및 예외 처리 로직을 고려해야 합니다.
-
의존성 주입의 활용:
@Repository애너테이션을 사용하고 있는 인터페이스이므로, 이 인터페이스를 사용하는 서비스 클래스에서 적절하게 의존성 주입이 이루어지는지 확인해보세요.
전반적으로 코드에 큰 문제가 보이지 않지만, 약간의 개선 여지가 있습니다. 위의 사항들을 참고하시어 코드를 수정하시면 좋을 것 같습니다.
| List<ProjectTag> projectTags = projectTagReader.readByProjectId(project.getId()); | ||
| return projectTagStore.store(project, tags, projectTags); | ||
| } | ||
| } |
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.
코드 패치에 대한 리뷰를 하겠습니다.
-
변수명 변경:
adminProjectTagStore를projectTagStore로 변경한 것은 코드의 가독성을 높이는 좋은 결정입니다. 변수 이름이 좀 더 일반적이고 이해하기 쉽게 되었습니다. -
메서드 로직 변경:
storeProjectTags메서드 내에서projectTagReader.readByProjectId(project.getId())을 호출하여 현재 프로젝트에 대한 태그를 읽은 후,projectTagStore.store(project, tags, projectTags)로 변경한 것은 기존 태그 리스트를 고려하여 저장하는 맞춤형 로직으로 개선된 것 같습니다. -
버그 리스크:
projectTagReader.readByProjectId(project.getId())에서project.getId()가null일 경우,NullPointerException이 발생할 수 있습니다. 입력 값이 항상 유효하다는 보장이 없다면, 유효성 검사 또는 예외 처리를 추가하는 것을 고려해야 합니다.projectTagStore.store메서드의 세 번째 인자인projectTags가 비어 있을 경우의 경우에 대한 처리가 필요할 수 있습니다. 이 경우, 적절한 로직이 없으면 예기치 않은 결과를 초래할 수 있습니다.
-
개선 제안:
storeProjectTags메서드에 대한 예외 처리 및 입력 값 검증을 추가하여 안정성을 높이면 좋겠습니다.- 또한, Javadoc 주석을 통해 메서드가 어떤 작업을 수행하는지 보다 명확히 설명하는 것도 가독성을 높이는 데 도움이 될 것입니다.
전반적으로 코드 개선이 잘 이루어졌으며, 위의 지적 사항들을 고려하여 수정해보면 더 나은 코드가 될 것 같습니다.
| public interface AdminProjectTagStore { | ||
| List<ProjectTag> store(Project project, List<String> tags); | ||
| List<ProjectTag> store(Project project, List<String> tags, List<ProjectTag> projectTags); | ||
| } |
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.
코드 리뷰 결과는 다음과 같습니다:
-
메서드 시그니처 변경:
store메서드에 새로운 파라미터List<ProjectTag> projectTags가 추가되었습니다. 이는 기존 코드와의 호환성에 영향을 미칠 수 있으며, 메서드 호출하는 곳에서 이 추가된 파라미터를 제공해야 합니다. 호출하는 모든 클래스에서 수정이 필요할 수 있으므로 이를 문서화하거나 주석으로 명시하는 것이 좋습니다. -
인수의 의미: 추가된
projectTags인수의 의미가 명확하지 않습니다. 이 인수가 어떤 역할을 수행하는지 설명하는 주석을 추가하는 것이 좋습니다. 이를 통해 다른 개발자들이 이 인터페이스의 사용 방법을 이해하기 쉬워질 것입니다. -
/code의 단위 테스트: 이 인터페이스를 구현하는 클래스에 대한 테스트가 제대로 되어 있는지 확인해야 합니다. 새로운 파라미터가 추가됨에 따라 테스트 케이스도 업데이트되어야 합니다.
-
예외 처리:
store메서드 내에서 예외가 발생할 수 있는 상황(예: null 값이 전달됨 등)에 대한 처리 로직이 필요할 수 있습니다. 메서드를 호출하는 측에서 유효성을 검사하고 필요한 경우 예외를 던지는 것도 고려해야 합니다. -
성능 고려: 만약
projectTags가 많은 요소를 포함할 수 있다면 성능 이슈를 고려해야 합니다. 리스트의 크기나 사용 방식에 따라 성능 저하가 발생할 수 있습니다.
이러한 부분들을 고려하여 코드의 안전성과 가독성을 높일 수 있을 것입니다. 추가적인 질문이나 코드에 대한 더 깊은 분석이 필요하다면 말씀해 주세요.
| this.authorizationCode = authorization.getAuthorization(); | ||
| this.isActive = authorization.getIsActive(); | ||
| } | ||
|
|
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.
코드 패치에 대해 간단히 검토해보겠습니다.
-
변수명 변경:
this.authorizationCode의 기존 값인authorization.getAuthorizationCode()를authorization.getAuthorization()으로 변경했습니다. 만약getAuthorization()이getAuthorizationCode()와 반환 타입이 다르다면, 예상치 못한 결과를 초래할 수 있습니다. 이 부분에서 반환 타입과 의미를 재확인해야 합니다. -
명확성: 메소드 이름이
getAuthorization()인 것으로 보아authorizationCode와 관련이 없을 수도 있습니다. 코드를 이해하는 데 혼란을 줄이기 위해 메소드명을 더 명확하게 하는 것이 좋습니다. 예를 들어,getAuthorizationDetails()같은 이름으로 메소드를 변환할 수 있습니다. -
Null 체크:
authorization객체가 null일 가능성도 고려되어야 합니다. null 체크를 추가한다면, NullPointerException을 방지할 수 있습니다. 예를 들어:if (authorization == null) { throw new IllegalArgumentException("Authorization must not be null"); }
-
주석 추가: 코드에 대한 주석이 없다면, 나중에 다른 개발자가 코드를 이해하기 어려울 수 있습니다. 트랜잭션의 흐름이나 직관적이지 않은 부분에 대해서는 주석을 추가하는 것이 좋습니다.
-
테스트: 변경 사항이 기존의 기능에 영향을 미치지 않는지 확인하기 위해 충분한 단위 테스트와 통합 테스트를 마련해야 합니다. 특히,
getAuthorization()의 반환값이 예상대로 작동하는지 검증하는 테스트가 필요합니다.
이러한 점들을 고려하면 코드의 안정성과 가독성을 높일 수 있을 것입니다.
| this.authorization = authorization.getAuthorization(); | ||
| this.memberType = authorization.getMemberType(); | ||
| } | ||
|
|
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.
코드 패치에 대한 리뷰를 하겠습니다.
-
변수 이름 변경:
getAuthorizationCode()에서getAuthorization()으로 변경된 점은 API의 의미를 더 명확하게 만드는 긍정적인 개선입니다. 이로 인해 코드 가독성이 향상될 것으로 보입니다. -
버그 리스크:
- 만약
getAuthorization()메서드가getAuthorizationCode()와 다른 반환 형식을 가지거나 예외를 던지는 경우가 있다면, 기존 코드와 호환되지 않게 되어 잠재적인 버그가 발생할 수 있습니다. 따라서, 메서드의 구현 내용을 확인하고 의도한 대로 동작하는지 테스트하는 것이 중요합니다.
- 만약
-
유닛 테스트: 변경된 메서드 호출에 대한 유닛 테스트가 적절히 구현되었는지 확인할 필요가 있습니다. 테스트가 없다면, 기존 코드의 기능이 새로운 코드에서도 잘 유지되는지 확인할 수 없습니다.
-
주석 추가: 코드의 가독성을 더 높이기 위해 변경된 부분이나 의도에 대한 주석을 추가하는 것이 좋습니다. 특히, 다른 개발자들이 이 부분을 이해하는 데 도움이 될 수 있습니다.
-
리팩토링 고려: 만약
Response클래스가 점점 더 커지고 복잡해진다면, 이 클래스를 더 작은 클래스로 나누거나 책임을 분리하는 것도 고려해볼 수 있습니다.
이와 같은 점들을 고려하여 코드를 개선하면 좋을 것 같습니다.
| this.projectAuthorization = projectAuthorization.getAuthorization(); | ||
| } | ||
|
|
||
| public static DeveloperMemberAuthorization toDto(ProjectAuthorization projectAuthorization) { |
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.
코드 패치에 대한 간단한 리뷰를 하겠습니다.
-
변경 사항의 의미:
projectAuthorization.getAuthorizationCode()에서projectAuthorization.getAuthorization()으로 변경되었습니다. 이 변경은 메서드의 이름을 더 명확하고 의미 있는 것으로 바꾸는 것으로 보입니다. 그러나, 이러한 변경이 기존에 의도된 동작과 다를 수 있으므로, 기존 로직에서 이 메서드가 반환하는 값이 무엇인지 확인이 필요합니다. -
버그 리스크:
- 만약
getAuthorization()메서드가 네이밍의 절대적인 변경이 아닌 다른 값을 반환한다면, 이로 인해 논리적 오류가 발생할 수 있습니다. 예를 들어, 권한 코드를 반환하는 경우 로직이 깨질 수 있습니다. - 변경된 메서드가 null을 반환할 가능성을 고려해야 합니다. 호출된 메서드가 null을 반환할 경우, 이후 코드에서 NullPointerException이 발생할 수 있습니다.
- 만약
-
개선 제안:
- 메서드의 의미나 반환 값의 변경 사항에 대한 주석을 추가하는 것이 좋습니다. 다른 개발자들이 이 변경을 이해하고 추후 유지보수를 하는 데 도움이 될 것입니다.
toString()메서드가 포함되어 있다고 언급되어 있으나, 해당 메서드의 실제 구현 코드가 보이질 않습니다. 이 메서드가 필요한 정보들을 적절히 포함하고 있는지 확인이 필요합니다.
-
테스트: 수정된 부분에 대한 유닛 테스트를 추가하거나 기존 테스트를 업데이트하여 변경 사항을 검증하는 것이 중요합니다. 이로 인해 새로 변경된 로직이 올바르게 작동하는지 확인할 수 있습니다.
위 사항들을 고려하여 추가적인 검토 및 테스트를 진행해 주시기를 권장합니다.
| this.projectAuthorization = projectAuthorization.getAuthorization(); | ||
| } | ||
|
|
||
| public static DeveloperMemberAuthorization toDto(ProjectAuthorization projectAuthorization) { |
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.
이 코드 패치에 대한 간단한 리뷰를 하겠습니다.
코드 변경 사항
projectAuthorization.getAuthorizationCode()메서드를projectAuthorization.getAuthorization()메서드로 변경했습니다.
잠재적 버그 리스크
- 메서드 반환 타입의 변경:
getAuthorizationCode()와getAuthorization()의 반환 타입이 다를 수 있습니다. 새로운 메서드getAuthorization()이 기대하는 데이터 형식이나 값이getAuthorizationCode()의 것과 다를 수 있으므로, 이 부분에 대한 확인이 필요합니다. - Null 체크:
projectAuthorization객체가 null인 경우에 대한 처리가 코드에 없기 때문에, 이 객체에 접근하려 할 때 NullPointerException이 발생할 수 있습니다.
개선 제안
- 명확한 메서드 이름:
getAuthorization()메서드가 어떤 값을 반환하는지(예: 코드인지, 객체인지 등) 명확히 이해할 수 있도록 주석을 추가하거나 이름을 좀 더 명확하게 변경하는 것이 좋습니다. - 예외 처리 추가:
projectAuthorization가 null일 경우 적절한 예외를 던지거나 디폴트 값을 설정하는 로직을 추가하여 안정성을 높일 수 있습니다. - 테스트 추가: 변경된 메서드의 반환 값을 확인하는 단위 테스트를 추가하여 코드가 올바르게 작동하는지 검증하는 것이 좋습니다.
이러한 점들을 고려하면 코드의 안정성과 가독성을 향상시킬 수 있을 것입니다.
| private Authorization projectAuthorization; | ||
| private HttpStatus status; | ||
| private String 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.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
변경 사항
- String에서 Authorization으로 변경:
projectAuthorization이라는 필드 타입이String에서Authorization타입으로 변경되었습니다. 이는 보다 명확하고 안전한 타입을 사용했다는 점에서 긍정적으로 평가됩니다.
잠재적 버그 리스크
-
Authorization 클래스 상태:
Authorization클래스의 정의가 코드 스니펫에 포함되어 있지 않아, 타입의 유효성 및 관련 로직이 올바르게 작동하는지 확인할 필요가 있습니다. 특히,Authorization타입이 null일 가능성을 고려해야 합니다. -
필드 초기화:
projectAuthorization필드가 자동으로 초기화되지 않는 경우, 이 회원의 권한 정보가 미설정된 상태로 남을 수 있습니다. 만약 이 필드가 필수라면 생성자에서 초기화해야 합니다. -
null 처리:
projectAuthorization의 null 체크 및 처리 로직이 필요할 수 있습니다. 특히, 이 값이 null인 상태에서 어떤 메소드를 호출할 때 NPE(Null Pointer Exception)가 발생할 수 있습니다.
개선 제안
-
유효성 검사 추가:
projectAuthorization값이 유효한지 확인하는 메소드를 추가하여, 유효하지 않은 상태로 인해 발생할 수 있는 문제를 방지할 수 있습니다. -
전반적인 타입 일관성 유지: 코드의 다른 부분에서도
String에서Authorization으로의 변경 사항이 제대로 반영되었는지 검토해야 합니다. 이로 인해 발생할 수 있는 불일치는 버그를 야기할 수 있습니다. -
문서화 및 주석: 코드의 변화를 문서화하고, 특히 변경된 필드의 목적과 사용 방법에 대한 주석을 추가하면 팀원들이 이해하기 쉬울 것입니다.
이와 같은 리뷰 사항을 반영하면 코드의 품질을 높이는 데 도움이 될 것입니다.
바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!
✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)
🏆 구현 목표
ex)
bucket post 및 bucket list view 초기 구현
📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)
ex)
🔍 테스트 방법 (변경 사항을 확인하기 위한 테스트 방법을 기술합니다.)
ex)
npm test실행🛠️ 쓰이는 모듈
ex)
💬 기타 질문 및 특이 사항
ex) 백프 배포 nginx 검토