refactor : refactor authorization request divide #235
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}
| @@ -15,6 +16,7 @@ | |||
| public class BaseEntity extends BaseTimeEntity { | |||
|
|
|||
| @CreatedBy | |||
| @Column(updatable = false) | |||
| private Long createdBy; // 생성자 | |||
|
|
|||
| @LastModifiedBy | |||
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 제공하겠습니다.
-
import추가:import jakarta.persistence.Column;이 코드에 추가되었습니다. 이는@Column어노테이션을 사용하기 위한 필수 절차이므로 올바른 수정입니다. -
@Column(updatable = false):createdBy필드에@Column(updatable = false)어노테이션이 추가되었습니다. 이는 생성자는 수정할 수 없다는 의미로, 적절한 사용입니다. 그러나 데이터베이스에서createdBy값을 한 번 설정한 후에는 변경되지 않도록 하는지 확인해야 합니다. 만약 해당 필드가 나중에 변경될 이유가 있다면, 이 설정이 문제를 일으킬 수 있습니다. -
주석:
// 생성자라는 주석이 추가되었습니다. 주석은 어느 정도 도움이 되지만, 다른 개발자에게 더 많은 맥락을 제공하기 위해 생성자의 역할이나 사용 의도를 간략하게 설명하는 것이 좋습니다. -
타입:
createdBy의 타입이Long으로 설정되어 있습니다. 데이터베이스에 저장되는 사용자 정보를 안전하게 표현하는지 확인해야 합니다. 예를 들어, 사용자가 삭제되고 나서도createdBy필드가 유효하게 남아있어야 한다면, 사용자 ID와 관련된 논리적 고려가 필요할 수 있습니다. -
통합 테스트: 패치된 사항이 시스템의 다른 부분에 미치는 영향을 평가하기 위해 충분한 통합 테스트를 작성하는 것이 중요합니다. 특히 생성자와 같은 중요한 데이터가 유지되고 있는지 검증하는 테스트 케이스가 필요합니다.
모든 변경 사항을 종합적으로 검토한 후, 문제를 방지하기 위한 추가적인 테스트와 문서화가 권장됩니다.
| @@ -15,6 +16,7 @@ | |||
| public class BaseTimeEntity { | |||
|
|
|||
| @CreatedDate | |||
| @Column(updatable = false) | |||
| private LocalDateTime createdAt; // 생성 시간 (수정 불가) | |||
|
|
|||
| @LastModifiedDate | |||
There was a problem hiding this comment.
이 코드 패치에 대한 간단한 코드 리뷰를 진행하겠습니다.
-
주석 추가: 주석에서 "생성 시간 (수정 불가)"이라는 설명은 좋습니다. 그러나 다른 필드나 메서드에 대해서도 주석을 추가하면 코드 이해에 도움이 될 것입니다.
-
의존성 관리:
jakarta.persistence.Column을 추가한 것은 좋습니다. 하지만LocalDateTime이 어디서 선언되는지 명확하지 않네요.import java.time.LocalDateTime;과 같은 import 문이 필요할 수 있습니다. -
업데이트 가능한 컬럼:
@Column(updatable = false)애너테이션을 추가한 것은createdAt필드가 수정되지 않도록 보장하는 좋은 방법입니다. 하지만 애너테이션의 의도에 맞는 적절한 테스트를 수행하여 예상한 대로 작동하는지 확인하는 것이 중요합니다. -
기타 필드의 동일한 처리 고려: 만약 클래스 내에 또 다른 필드들이 있다면, 이들과 관련하여 필요한 경우 동일한 애너테이션을 적용할 수 있습니다. 예를 들어, 수정된 날짜를 저장하는 필드에도 비슷한 처리가 필요할 수 있습니다.
-
Null 체크: 생성일 표시용 필드는 주로 null이 아니어야 하므로, 초기화 로직을 추가하거나 생성자에서 값을 설정하는지 확인하는 것이 좋습니다.
-
Serializable 인터페이스: 이 클래스가 JPA의 엔티티로 사용될 것이라면
Serializable인터페이스를 구현하는 것이 좋습니다. 이는 데이터베이스와의 상호작용 및 직렬화에 도움이 됩니다.
이러한 점들을 고려하면 코드의 안정성과 가독성을 개선할 수 있을 것입니다. 전체적으로는 좋은 방향으로 변화하고 있으며, 필요한 몇 가지 세밀한 검토가 추가로 필요해 보입니다.
| this.startDate = CustomDateFormatter.formatDate(project.getStartDate()); | ||
| this.endDate = CustomDateFormatter.formatDate(project.getEndDate()); | ||
| this.startDate = CustomDateFormatter.formatDate(project.getPlannedStartDate()); | ||
| this.endDate = CustomDateFormatter.formatDate(project.getPlannedEndDate()); | ||
| this.customerId = project.getCustomer().getId(); | ||
| this.customerCompanyName = project.getCustomer().getCompanyName(); | ||
| this.developerId = project.getDeveloper().getId(); |
There was a problem hiding this comment.
코드 패치를 살펴본 결과 몇 가지 개선 사항과 잠재적인 버그 위험 요소를 발견할 수 있습니다.
-
변경 내용 검토:
project.getStartDate()와project.getEndDate()를project.getPlannedStartDate()와project.getPlannedEndDate()로 변경한 부분은 논리적으로 올바른 변경으로 보입니다. 그러나,Planned관련 메소드가 최근 프로젝트의 실제 시작과 종료 날짜가 아닌 예정된 날짜를 반환한다는 점에 유의해야 합니다. 특정 비즈니스 로직이나 요구사항에 따라 이 변경이 적절한지 확인할 필요가 있습니다.
-
널 포인터 예외 (NullPointerException):
project.getCustomer()나project.getDeveloper()가 null을 반환할 경우, 이는 NullPointerException을 발생시킬 수 있습니다. 따라서 고객 및 개발자를 가져오는 부분에서 null 체크를 추가하는 것이 좋습니다.
-
날짜 포맷팅 예외 처리:
CustomDateFormatter.formatDate에서 날짜 포맷팅에 실패할 경우 예외가 발생할 수 있습니다. 이러한 경우를 처리하기 위한 예외 처리 로직을 추가하는 것이 좋습니다.
-
주석 및 문서화:
- 이 변경 사항이 왜 필요했는지에 대한 주석을 추가하는 것도 좋습니다. 나중에 코드를 유지보수하거나 검토할 때 도움이 될 수 있습니다.
-
테스트 케이스 추가:
- 변경된 로직에 대해 충분한 단위 테스트가 있는지 확인하십시오. 특히 예상되는 입력값과 경계 조건에 대한 테스트가 포함되어야 합니다.
이와 같은 점들을 고려하여 코드를 개선해 나가길 추천합니다.
| } | ||
|
|
||
| public static DeveloperMemberAuthorization toDto(ProjectAuthorization projectAuthorization) { | ||
| return new DeveloperMemberAuthorization(projectAuthorization); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰 결과는 다음과 같습니다.
버그 위험
-
NullPointerException:
projectAuthorizations리스트가 null일 경우,forEach메소드 호출 시 NPE가 발생할 수 있습니다. 리스트가 null인지 확인하는 로직이 필요합니다. -
코드 중복:
CustomerMemberAuthorization과DeveloperMemberAuthorization클래스에서 유사한 필드와 메소드가 있습니다. 코드 중복을 피하기 위해 상위 클래스를 만들어 공통된 필드를 정의할 수 있습니다. -
Enum 비교:
MemberType.CLIENT와의 비교에서 enum 값이 확실히 정의되어 있는지 확인해야 합니다. 이 경우, 잘못된 값을 가지거나 null일 경우 예외 상황이 발생할 수 있습니다.
개선 사항
-
메소드와 변수를 private로 선언:
private필드와 메소드가 기본적으로private로 선언되지 않은 것 같습니다. 캡슐화를 강화하기 위해 접근 제한자를 명확히 해야 합니다. -
의미 있는 변수명:
memberAuthorization을customerMemberAuthorizations로 변경하여 가독성을 높인 점은 좋지만, 각 클래스에도 명확하고 일관된 네이밍이 적용될 필요가 있습니다. 예를 들어,projectAuthorization이 아니라authorizationCode와 같은 구체적인 이름이 좋습니다. -
정적 팩토리 메소드의 일관성: 각 클래스에 대한 정적 메소드가 존재하지만, 일관된 네이밍 규칙을 적용하는 것이 좋습니다. 예를 들어
toDto메소드는 DTO 객체로 변환하는 일반적인 관례이므로, 다른 이름으로 변경할 여지가 있을 수 있습니다. -
JavaDoc 추가: 각 클래스와 메소드에 대해 JavaDoc 주석을 추가하여 코드의 목적과 사용법을 명확하게 하는 것이 좋습니다.
-
불변 객체 사용: 만약
CustomerMemberAuthorization과DeveloperMemberAuthorization객체가 상태를 변경할 일이 없다면 불변 객체로 만들면 스레드 안전성을 높일 수 있습니다.
이러한 점들을 고려하여 코드를 개선하면 더 안전하고 유지보수하기 쉬운 코드가 될 것입니다.
| this.memberId = baseMemberAuthorization.getMemberId(); | ||
| this.memberType = baseMemberAuthorization.getMemberType(); | ||
| this.projectAuthorization = baseMemberAuthorization.getProjectAuthorization(); | ||
| } | ||
| this.status = status; | ||
| this.message = message; | ||
| } |
There was a problem hiding this comment.
코드 패치에 대한 리뷰를 진행하겠습니다. 아래는 코드에서 발견한 잠재적인 버그 리스크와 개선 제안입니다.
잠재적 버그 리스크
-
타입 안전성:
FailList클래스에서member가BaseMemberAuthorization의 인스턴스인지 확인하고 변환하고 있지만, 이 과정에서 잘못된 타입이 들어올 경우member에 대한 접근이 이루어지지 않을 수 있습니다. 이는 NullPointerException을 유발할 수 있습니다.member타입을 더 안전하게 처리하는 방법을 고려해야 합니다. -
데이터 불일치: 클래스 간의
memberType필드가 서로 다르게 정의되어 있습니다. 고객과 개발자의memberType을 각각의 클래스에서 하드코딩하고 있어, 특정 변경이 있을 경우 모든 클래스에 대해 일관성을 유지하는 데 어려움이 생길 수 있습니다. 이 부분을 enum을 통해 중앙에서 정의하는 것이 좋습니다. -
toString메소드:toString메소드의 구현이 일부 클래스에서 누락되어 있습니다. 디버깅 시 유용하므로, 모든 클래스에 대한toString구현을 고려해야 합니다. TODO 주석으로 남긴 부분이 있기 때문에, 이 점을 잊지 말고 처리해야 합니다.
개선 제안
-
제네릭 사용:
MemberAuthorization인터페이스에 제네릭을 추가하여 각 멤버 클래스에서 요청/응답 데이터를 더 명확하게 처리할 수 있습니다. 이를 통해 코드의 가독성과 유지보수성을 높일 수 있습니다. -
객체 초기화: 클래스가 간단한 데이터를 보관하는 역할을 한다면, 모든 필드에 대한 초기화를 생성자에서 처리하는 것이 좋습니다. 특히
new ArrayList<>()를 생성자에서 처리하면 코드가 더 깔끔해질 수 있습니다. -
의미있는 메소드 명:
toEntity메소드는 목적이 명확하지만, 메소드 이름을 더 설명적인 이름으로 변경하면 코드의 의도를 더 잘 전달할 수 있습니다. 예를 들어,createProjectAuthorization와 같이 명명하는 것이 좋습니다. -
불필요한 주석 제거: 메소드와 클래스 이름이 이미 그 목적을 잘 설명하기 때문에 불필요한 주석은 제거해서 코드를 깔끔하게 유지하는 것을 고려하십시오.
위의 사항을 염두에 두고 코드를 수정한다면, 가독성이 높아지고 잠재적인 버그를 줄일 수 있을 것입니다.
| projectAuthorizationHistoryRepository.save(authorization.delete()); | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 아래에 정리하였습니다.
코드 리뷰
-
변수명 개선:
projectAuthorizations대신existingProjectAuthorizations로 변경하여 보다 명확하게 기존 권한을 나타내고 있습니다. 변수명의 변경은 코드 가독성을 높이는 좋은 방법입니다. -
Null 체크:
requestDto.getCustomerAuthorizations()와requestDto.getDeveloperAuthorizations()의 null 체크를 통해, 이러한 리스트가 null인 경우에도 안전하게 처리할 수 있도록 하고 있습니다. -
비즈니스 로직 분리:
forRequestDto와businessLogic메서드를 적절히 분리함으로써 코드의 읽기 쉬움과 유지보수성이 향상되었습니다. 특히, 권한 처리 로직을processAuthorization메서드로 모듈화한 점이 좋습니다. -
제어 흐름 명확화:
matchedAuthorization을 통해 어떤 권한이 요청에 의해 업데이트되었는지를 명확히 하고 있습니다. -
에러 처리:
BusinessException을 catch하여 에러에 대한 처리를 하고 있는 점은 좋은 접근 방식입니다.
버그 위험
-
ConcurrentModificationException:
existingAuthList.remove(matchedAuthorization);부분에서 해당 리스트를 순회하는 중에 객체를 제거하는 경우,ConcurrentModificationException이 발생할 수 있습니다. 리스트를 안전하게 수정하기 위해 Iterator를 사용하는 것이 권장됩니다. -
자원 정리: 삭제 처리하는
delete메서드에서는 모든 권한을 삭제하고 있습니다. 이 과정에서 예외가 발생할 경우, 관리하지 못하는 자원 누수 문제가 발생할 수 있습니다. 트랜잭션 관리를 통해 이 점을 보완하는 것이 좋습니다. -
null 체크:
request.getMemberId()에 대한 null 체크가 생략되어 있어,NullPointerException이 발생할 수 있습니다. 이 검사를 추가하는 것이 좋습니다.
개선점
-
주석 추가: 각 메서드에 대한 설명이 있지만, 메서드 내부 로직에는 추가 주석이 부족합니다. 특히 복잡한 로직에는 각 단계에 주석을 추가하여 이해를 도울 수 있습니다.
-
로깅 개선: 권한 관련 비즈니스 로직이 수행될 때마다 로그를 추가하여 추적을 용이하게 만드는 것이 좋습니다. 예를 들어, 권한이 새로 생성되거나 업데이트 될 때 로그를 남기는 것이 유용할 수 있습니다.
-
유효성 검사 추가:
requestDto의 필수 필드에 대한 유효성 검사를 추가하는 것이 좋습니다. 예를 들어, 고객 권한과 개발자 권한이 모두 기본값인지 확인하는 로직이 필요할 수 있습니다.
이와 같은 점들을 고려하면 코드의 품질과 유지보수성을 더욱 높일 수 있습니다.
바로 MERGE하지 마세요! 검토 후 MERGE해야합니다!
✅ 최근 작업 주제 (하나 이상의 주제를 선택해주세요.)
🏆 구현 목표
ex)
bucket post 및 bucket list view 초기 구현
📋 구현 사항 설명 (작업한 내용을 상세하게 기록합니다.)
🔍 테스트 방법 (변경 사항을 확인하기 위한 테스트 방법을 기술합니다.)
ex)
npm test실행🛠️ 쓰이는 모듈
ex)
💬 기타 질문 및 특이 사항
ex) 백프 배포 nginx 검토