-
Notifications
You must be signed in to change notification settings - Fork 4
Refactor/put project #297
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
Refactor/put project #297
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
| 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.
코드 패치에 대한 간단한 리뷰를 아래에 제시합니다.
코드 리뷰
-
타입 변경:
projectAuthorization의 타입을String에서Authorization으로 변경했습니다.- 이 변경은 명확성과 타입 안전성을 높이는 긍정적인 개선입니다.
String으로 표현되는 권한보다Authorization객체를 사용하면 코드의 가독성과 유지보수성이 향상됩니다.
-
의존성:
Authorization클래스를 추가함으로써, 해당 클래스에 대한 의존성이 생깁니다.- 클래스가 잘 정의되어 있고 필요한 모든 메서드와 속성을 포함하고 있는지 확인해야 합니다. 아니면 코드에 오류가 발생할 가능성이 있습니다.
-
추상 메서드:
toEntity메서드가 잘 정의되어 있습니다. 하지만 이 메서드의 구현체에서Authorization타입을 어떻게 처리할 것인지에 대한 고려가 필요합니다.- 이를 위해 추가적인 검증 로직이 필요할 수 있습니다.
-
Null 안전성:
projectAuthorization을Authorization타입으로 변경하면서, null 값이 될 수 있는 가능성에 대해 확인이 필요합니다.- null 체크 또는 기본값 설정을 고려해보세요.
-
주석 및 문서화:
- 이 변경 사항에 대한 주석이나 문서화가 없으므로, 코드의 가독성을 높이기 위해 해당 필드의 목적과 사용 방법에 대한 설명을 추가하는 것이 좋습니다.
결론
변경된 코드는 명확성과 타입 안전성을 높이는 긍정적인 방향으로 개선되었습니다. 하지만 의존성 관리와 null 안전성, 및 문서화에 추가적인 주의가 필요합니다. 코드베이스에 통합하기 전에 충분한 테스트를 수행하는 것이 좋습니다.
| contractNumber = project.getContractNumber(); | ||
| plannedStartDate = project.getPlannedStartDate(); | ||
| plannedEndDate = project.getPlannedEndDate(); | ||
| startDate = project.getStartDate(); // 시작일 |
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.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
-
변수 제거:
contractNumber변수가 몇 군데에서 제거되었습니다. 이 부분이 의도적인 것인지 확인이 필요합니다. 만약contractNumber가 더 이상 필요 없다면, 관련 로직에서 이 값을 참조하고 있는 부분이 없도록 전체적으로 점검해야 합니다. -
유지보수성: 만약
contractNumber관련 기능이 실제로 제거되었다면, 이와 관련된 주석이나 문서화도 함께 업데이트 하는 것이 좋습니다. 코드 내에서 제거된 사항에 대한 설명이 없으면 나중에 코드를 읽는 사람이 혼란스러워 할 수 있습니다. -
Nullable 처리:
contractNumber가 제거되면, 경우에 따라서 관련된 데이터베이스 필드나 API 응답에서도 이 필드의 존재 여부를 확인할 필요가 있습니다. 데이터베이스 스키마나 기존의 API 문서도 점검하여 일관성을 유지하는 것이 중요합니다. -
테스트 케이스: 이 변경 사항으로 인해 기존의 테스트 케이스가 실패할 수 있습니다.
contractNumber와 관련된 모든 테스트가 포함되어 있는지 확인하고, 필요 없어진 테스트는 삭제하고, 새로운 로직에 대한 테스트를 추가해야 합니다. -
의존성 문제: 만약
contractNumber가 다른 클래스나 메서드에서 사용되었다면, 이들을 찾아서 어떤 영향을 미칠지 분석해야 합니다. 원치 않는 런타임 에러가 발생할 수 있습니다. -
데이터 무결성: 계약서 번호는 비즈니스 로직에서 중요한 역할을 할 수 있습니다. 제거한 이유가 비즈니스 요구사항 변화 때문이라면, 그러한 변화를 문서화하거나 관련 이해관계자와 논의하는 것이 좋습니다.
이러한 점들을 고려하여 코드를 점검하고, 필요시 수정 및 추가 작업을 수행하는 것이 좋습니다. 전체적인 의도와 비즈니스 요구사항에 부합하는 방향으로 코드를 유지보수하는 것이 중요합니다.
| 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는 괜찮지만,YesNo열거형으로 사용되고 있는 인자에 대해 주석을 추가하면 좋습니다. 이 열거형이 '예/아니오' 외 다른 값을 가질 경우, 호출하는 쪽에서 혼란이 발생할 수 있습니다. -
예외 처리: 이 리포지토리는 데이터베이스와의 연결에 문제가 생길 경우 적절한 예외 처리가 필요할 수 있습니다. 현재 상태로는 예외 처리 로직이 보이지 않으므로, 서비스 레이어에서 적절한 예외 처리를 추가해야 할 필요가 있습니다.
-
Query Method의 성능: 데이터베이스에서 데이터를 조회할 때,
find메서드가 많은 데이터를 반환할 가능성이 있는 경우 페이징과 정렬을 고려해야 할 수 있습니다. 필요하다면Pageable객체를 도입해 보세요. -
테스트 추가: 새로운 메서드가 추가되기 때문에, 유닛 테스트도 함께 작성하여 해당 메서드의 동작을 검증하는 것이 좋습니다.
이러한 사항들을 고려하여 코드를 개선하면, 더욱 안정적이고 유지보수하기 쉬운 코드가 될 것입니다.
| 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())를 호출하여 프로젝트 태그를 가져오고 있습니다. 이 메소드가 반환하는 결과가 항상 null이 아닌지 확인하는 것이 중요합니다. 예를 들어, 비어 있는 결과가 반환될 경우projectTagStore.store메소드는 제대로 작동할지 의문입니다.
-
오류 처리:
- 만약
projectTagReader.readByProjectId에서 예외가 발생한다면 이를 적절히 처리하지 않으면 어플리케이션이 크래시할 수 있습니다. 예외 처리를 추가하여 코드의 안정성을 높이는 것이 좋을 것 같습니다.
- 만약
-
입력 검증:
storeProjectTags메소드의 매개변수인project와tags에 대한 검증이 필요합니다. null 또는 유효하지 않은 값이 전달될 경우 이를 처리하는 로직을 추가하는 것이 좋습니다.
-
주석 추가:
- 메소드와 클래스에 대한 주석이 없으므로, 코드의 의도를 이해하기 어렵습니다. 각 메소드가 수행하는 일이 무엇인지 간단한 주석을 추가하는 것이 유지 보수성을 높이는 데 도움이 됩니다.
-
테스트 추가:
- 변경된 코드에 대한 단위 테스트가 있다면, 이 테스트가 모든 시나리오를 포괄하고 있는지 확인해야 합니다. 새로운 로직이 추가되었으므로 관련된 테스트 케이스를 작성하는 것이 중요합니다.
이 외에도 코드 스타일이나 일관성 같은 점을 검토할 수 있지만, 주된 리스크와 개선 사항은 위와 같습니다.
| 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인자에 대한 유효성 검사가 필요합니다. 이 리스트가 null일 경우, 또는 예상치 못한 데이터가 전달될 경우 예외가 발생할 수 있습니다. 적절한 예외 처리 로직을 추가하는 것이 좋습니다.
-
코드 개선 제안:
- 메서드의 의미를 더욱 명확하게 하기 위해 Javadoc 주석을 추가하는 것을 추천합니다. 파라미터에 대한 설명과 메서드의 기능을 서술하면 코드의 가독성이 향상됩니다.
- 또한,
List<String> tags와List<ProjectTag> projectTags가 서로 어떻게 관계되는지에 대한 설명이 필요할 수 있습니다. 가능하다면 이를 명확히 문서화해야 합니다.
-
테스트 검증:
- 변경된
store메서드에 대한 테스트 케이스를 추가하여 새로운 파라미터에 대한 시나리오를 검증해야 합니다. 특히, 빈 리스트, null 값, 그리고 예상치 못한 입력값에 대한 테스트가 중요합니다.
- 변경된
이 부분들을 고려하여 코드를 개선하면 더욱 안정적이고 유지보수하기 쉬운 코드를 만들 수 있을 것입니다.
| 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.
코드 패치에 대한 간단한 리뷰를 진행하겠습니다.
-
변경 사항 검사:
authorization.getAuthorizationCode()를authorization.getAuthorization()으로 변경했습니다. 이 변경은 코드의 기능적 동작에 영향을 미칠 수 있으므로,getAuthorization()메소드가getAuthorizationCode()와 동일한 데이터 타입과 의미를 가지는지 확인해야 합니다. 그렇지 않다면, 이로 인해 버그가 발생할 수 있습니다.
-
코드 가독성:
this.authorizationCode라는 변수의 이름이 의미하는 바가 명확해야 하므로,getAuthorization()메소드의 본문을 확인하여 이 메소드가 어떤 값을 반환하는지 명확히 이해할 필요가 있습니다.
-
에러 핸들링:
authorization객체가null일 경우NullPointerException이 발생할 수 있습니다. 이 경우를 처리하기 위한 정부 처리가 필요합니다.
-
주석 추가:
- 코드 변경의 이유를 설명하는 주석을 추가하는 것이 좋습니다. 특히,
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.
코드 패치에 대한 간단한 리뷰를 제공하겠습니다.
-
변경 내용:
authorization.getAuthorizationCode()에서authorization.getAuthorization()으로 변경되었습니다. 이 변경이 의도된 대로 작동하는지,getAuthorization()메서드가 올바른 형식의 값을 반환하는지 확인해야 합니다. 기존 코드에서 깃발이 될 수 있는 점은,getAuthorizationCode()와getAuthorization()의 반환 값이 동일한 형식이 아닐 수 있다는 것입니다. -
명확성:
authorization이라는 이름은 다소 애매할 수 있습니다.getAuthorization()이 반환하는 값이 어떤 의미인지 명확하지 않다면, 더 구체적인 이름으로 변경하는 것이 코드 가독성을 높일 수 있습니다. 예를 들어,getAuthorizationDetails()와 같은 이름으로 변경하는 것이 좋을 수 있습니다. -
예외 처리:
authorization.getAuthorization()호출 시 NullPointerException이 발생할 수 있습니다.authorization객체가 null이 아닌지, 혹은getAuthorization()메서드가 null을 반환할 가능성이 있는지 확인하고, 이에 대한 예외 처리를 추가하는 것이 좋습니다. -
주석 추가: 변경된 코드의 의도를 설명하는 주석을 추가하는 것도 가독성을 높일 수 있습니다. 이는 다른 개발자가 코드를 이해하는 데 도움이 될 것입니다.
-
테스트 케이스: 변경 사항이 기존 기능에 영향을 미칠 수 있으므로, 관련된 테스트 케이스를 작성하거나 기존 테스트가 통과하는지 확인해야 합니다. 특히 이 부분이 중요한 이유는
getAuthorization()의 반환 값이 기존과는 다를 수 있기 때문입니다.
이러한 점들을 고려하여 코드를 개선할 수 있습니다. 전반적으로, 변경 내용이 성공적으로 의도한 바를 달성하는지 검토하고, 가능한 예외 상황에 대한 처리가 필요할 것 같습니다.
| 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을 반환할 수 있다면,projectAuthorization필드가 null이 되는 경우를 처리해야 합니다. 이렇게 되면 후속 코드에서 NullPointerException이 발생할 수 있습니다. - 또한,
getAuthorizationCode()에서getAuthorization()으로의 변경으로 인해 의도하지 않은 기능상의 변화가 있을 수 있습니다. 이 메서드들 간의 차이점을 잘 이해하고 있는지 확인할 필요가 있습니다.
- 만약
-
개선 제안:
- 메서드 변경에 대한 테스트 케이스를 추가하는 것이 좋습니다. 특히, 변경된 메서드가 예상대로 작동하는지 확인하기 위한 단위 테스트를 포함해야 합니다.
- 문서화: 해당 변화에 대한 문서나 주석을 추가하여 이후 코드 유지보수 시에 혼선을 방지할 수 있습니다.
- 변수 검사:
getAuthorization()의 반환값을 확인 후 초기화하는 방법을 사용하는 것이 좋습니다. 예를 들어,this.projectAuthorization = projectAuthorization.getAuthorization() != null ? projectAuthorization.getAuthorization() : DEFAULT_VALUE;와 같은 방식입니다.
-
전반적인 평가: 전반적으로 메서드 이름이 더 명확해지는 긍정적인 변화이지만, 변경으로 인한 잠재적인 문제를 고려하고 철저한 검증이 필요합니다. 변경 사항을 빠르고 안전하게 반영하기 위해 추가적인 테스트를 권장합니다.
| 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.
코드 패치를 검토한 결과, 몇 가지 주의할 점과 개선 사항을 제안할 수 있습니다.
-
메서드 이름 변화:
getAuthorizationCode()에서getAuthorization()으로 변경되었습니다. 이 변경이 의도하는 바는 명확해 보이지만, 호출하는 다른 코드에서 메서드 이름 변경으로 인한 오류가 발생할 수 있습니다. 이 메서드를 호출하는 모든 곳에서 변경 사항이 반영되었는지 확인해야 합니다. -
주석 추가: 변경된 메서드의 목적이나 의도를 명확히 하기 위해 주석을 추가하는 것이 좋습니다. 이를 통해 코드의 가독성이 향상되고, 다른 개발자들이 코드를 이해하는 데 도움이 됩니다.
-
예외 처리:
projectAuthorization이null일 경우를 고려해야 합니다. 현재 코드는null체크가 없는 상태입니다.null값이 넘어올 경우NullPointerException이 발생할 수 있으므로, 이를 처리하는 로직을 추가하는 것이 좋습니다. -
일관성 유지:
CustomerMemberAuthorization과DeveloperMemberAuthorization에서 동일한 변경이 적용되었으니, 다른 관련 클래스에서도 같은 메서드 이름 변경이 필요한지 확인해야 합니다. -
코드 포맷팅: 코드의 일관된 포맷팅을 유지하여 가독성을 높이는 것이 좋습니다. 예를 들어, 클래스/메서드 사이에 적절한 빈 줄을 추가하면 더 깔끔하게 보일 수 있습니다.
요약하면, 메서드 이름 변경으로 인한 링크 끊김, 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 projectAuthorization에서Authorization projectAuthorization으로 변경되었습니다. 이로 인해 코드의 다른 부분에서projectAuthorization을 사용하는 부분에서 타입 불일치가 발생할 수 있습니다. 이를 확인해야 합니다. -
Null 안전성:
Authorization타입이 nullable할 수 있는 상황에서, 이를 사용하는 부분에서 null 체크를 하지 않으면 NullPointerException이 발생할 수 있습니다.projectAuthorization에 대한 null 체크를 추가하는 것이 좋습니다. -
클래스 및 인터페이스 문서화:
MemberAuthorization인터페이스와BaseMemberAuthorization클래스에 대한 JavaDoc을 추가하는 것이 좋습니다. 각 메서드와 클래스의 역할을 명확히 문서화하여 코드의 가독성과 유지보수성을 향상시킬 수 있습니다. -
상수 사용:
Authorization클래스의 사용이 늘어날 경우, 이와 관련된 상수를 잘 정의하여 사용하는 것이 좋습니다. 공간을 절약하고 코드의 가독성을 높일 수 있습니다. -
Error Handling:
create메서드의 책임에 대한 예외 처리 로직이 부족한 것 같습니다. 메서드 사용시 발생할 수 있는 오류에 대한 처리(예: 프로젝트가 존재하지 않거나 잘못된 멤버 ID 등이 들어오는 경우)를 고려해야 합니다. -
Testing: 변경 사항이 기존 기능에 영향을 미치지 않도록 단위 테스트가 필요합니다. 특히, 타입 변경으로 인한 코드 경로의 변화를 테스트하는 것이 중요합니다.
이러한 사항들을 고려하여 코드를 수정하면 좀 더 안정적이고 유지보수하기 쉬운 코드가 될 것입니다.
바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!
✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)
📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)
🔍 테스트 방법 (변경 사항을 확인하기 위한 테스트 방법을 기술합니다.)
ex)
npm test실행🛠️ 쓰이는 모듈
ex)
💬 기타 질문 및 특이 사항
ex) 백프 배포 nginx 검토