Feat/#15 Project 도메인 모델 구현 #29
Conversation
- Project 도메인 엔티티를 구현했습니다. - Project 생성 시 전달받은 이름이 공백이거나 없을 경우 예외를 반환하는 검증로직을 추가했습니다. - 이에 따른 테스트를 진행했습니다.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return new Project(name); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
코드 리뷰 결과는 다음과 같습니다:
버그 위험
-
SoftDelete 주석:
@SoftDelete어노테이션을 추가했는데, 이를 사용하려면 관련된 설정(예:hibernate.enable_lazy_load)이 필요할 수 있습니다. 설정 미비 시 의도한 대로 작동하지 않을 위험이 있습니다. -
생성자 접근 제한:
@NoArgsConstructor(access = AccessLevel.PROTECTED)와 같이 설정했지만, 이는 프레임워크에 의해 호출될 수 있으므로, 정적 팩토리 메소드를 사용하는 것이 좋습니다. -
쪽지의 생성 및 수정 날짜:
@CreatedDate와@LastModifiedDate가 제대로 작동하기 위해서는 해당 필드들을 관리하는 적절한 리스너 또는 설정이 필요합니다. 그렇지 않으면 날짜가 올바르게 기록되지 않을 수 있습니다.
개선 사항
-
Validation 강화:
from메소드에서 이름이 null인지 체크하고 빈 문자열인지 검사하지만, 추가적인 유효성 검사를 고려할 수 있습니다(예: 특정 형식, 길이 체크 등). -
Javadoc 추가: 클래스와 각 메소드에 대한 설명을 추가하여 코드 이해도를 높일 수 있습니다.
-
Null 체크 통일성:
from메소드에서null체크 시String을 직접 비교하기보다는Optional을 사용할 수도 있습니다. 이렇게 하면 코드의 가독성이 향상될 수 있습니다. -
테스트 케이스 작성: 이 설계가 올바르게 작동하는지 확인하기 위해 단위 테스트와 통합 테스트를 포함시키는 것이 좋습니다.
결론
전반적으로 잘 작성된 코드입니다. 몇 가지 유의해야 할 점과 더불어 개선할 여지가 있는 사항들이 있으며, 이를 통해 더 안정적이고 유지 보수가 쉬운 코드를 만들 수 있을 것입니다.
| @Repository | ||
| public interface ProjectJpaRepository extends JpaRepository<Project, Long> { | ||
|
|
||
| } |
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다.
-
패키지 구조: 패키지 네이밍은 적절하지만, 모듈의 용도에 따라 추가적인 하위 패키지를 고려할 수 있습니다. 예를 들어, 특정 기준으로 분류하고자 하는 도메인이 있다면 더 세분화할 수 있습니다.
-
JpaRepository 사용:
JpaRepository를 상속받아 기본 CRUD 작업을 제공하는 것은 좋은 접근입니다. 하지만 추가적인 쿼리 메서드가 필요할 수 있으며, 이후 비즈니스 로직에 맞는 커스텀 메서드를 정의할 필요가 있습니다. -
주석 부족: 현재 클래스에 문서화된 주석이 없습니다. 이 인터페이스의 기능과 용도를 설명하는 주석이 있으면 유지보수가 쉬워질 것입니다.
-
예외 처리: JPA를 사용할 때 데이터베이스와의 상호작용에서 발생할 수 있는 예외 상황에 대한 처리가 필요합니다. 필요한 경우 적절한 위치에 서비스 레이어에서 예외 처리를 구현하십시오.
-
테스트 코드: 이 리포지토리에 대한 유닛 테스트나 통합 테스트 코드가 없으면 실제 작동 여부를 검증하기 어렵습니다. 테스트 코드를 작성하여 안정성을 높이는 것이 좋습니다.
종합적으로, 이 리포지토리는 기본적인 구조를 잘 갖추고 있으나, 향후 확장성과 유지를 위해 위의 사항들을 고려하며 개선하는 것이 좋습니다.
| } | ||
|
|
||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다.
버그 리스크
-
예외 메시지 검증:
IllegalArgumentException예외에서 특정 메시지를 검증하고 있습니다. 나중에 이 메시지가 변경될 경우, 테스트가 실패할 수 있습니다. 예외 메시지를 외부화하거나 상수로 정의하는 방식이 더 안전할 수 있습니다. -
파라미터 테스트의 경우의 수: 현재는 빈 문자열과 null을 처리하고 있지만, 공백 문자열과 같은 유효하지 않은 입력도 추가로 검증할 필요가 있습니다.
개선 사항
-
Assertions 메서드 사용:
assertThat(actualResult).hasFieldOrPropertyWithValue(...)대신, 대상 객체의 메소드를 사용하는assertThat(actualResult.getName()).isEqualTo(expectedProjectName);을 사용하는 것이 명시적이고 가독성이 좋습니다. -
코드 주석: "Act & Assert"와 같은 주석을 굳이 사용할 필요는 없으며, 메서드 이름만으로도 충분히 의도가 드러납니다.
-
테스트 이름 변경: 테스트 메서드 이름이 조금 길어지는 경향이 있습니다. 'testFromMethodFailWhenNameValueNotVerified' 대신 'throwsExceptionForInvalidName'과 같이 간결하게 표현할 수 있습니다.
-
파일 끝 줄 문제: 파일의 끝에 newline을 추가하는 것이 일반적인 관례입니다.
전체적으로 테스트 코드가 잘 구조화되어 있으며, 클린한 설계를 따르고 있습니다. 위의 몇 가지 사항만 반영한다면 더욱 개선된 코드가 될 것입니다.
- 일부 테스트 메서드 명을 변경했습니다.
| } | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다.
버그 리스크
-
얕은 복사 및 불변성:
Project.from(name)메소드에서name필드의 불변성을 보장해야 합니다. 예외 처리에서는 문제가 없지만, 이름이 변경되지 않도록 관리할 필요가 있습니다. -
예외 메시지 일관성: 던지는 예외의 메시지가 하드코딩되어 있는데, 코드 수정 시 명시적으로 다루어야 할 부분입니다. 상수로 정의하여 관리하는 것이 좋습니다.
개선 제안
-
요구 사항 추가:
Project클래스의 생성 조건에 대해 더 구체적인 요구 사항을 문서화하거나 주석으로 남기는 것이 좋습니다. 예를 들어, 최대 길이나 허용되는 문자 등. -
테스트 케이스 확장: 다른 잘못된 이름 값도 테스트 케이스에 추가해보세요. 예를 들어, 너무 긴 문자열이나 특정 특수문자를 포함한 경우 등.
-
예외 처리 테스트 강화:
assertThatThrownBy()를 사용한 예외 테스트는 좋지만 다양한 예외 상황에 대한 검증을 추가하면 더 견고해질 것입니다. -
테스트 메소드 네이밍: 테스트 메소드 이름을 짓는 데 있어 간결함과 명확성을 유지하세요. 예를 들어,
throwsExceptionForInvalidName대신shouldThrowExceptionForInvalidName같은 네이밍이 더 나을 수 있습니다.
전반적으로 기본적인 구조는 잘 되어 있으며, 작은 개선만 필요합니다. Test-driven 개발(TDD) 원칙을 잘 따르고 있는 것으로 보여지네요.
logbat/src/main/java/info/logbat/domain/project/domain/Project.java
Outdated
Show resolved
Hide resolved
logbat/src/main/java/info/logbat/domain/project/domain/Project.java
Outdated
Show resolved
Hide resolved
- VARCHAR(100) 이상의 값을 저장할 수 없게 검증 로직을 추가했습니다.
| return new Project(name); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
코드 리뷰 결과입니다:
-
Null 체크:
from메소드에서 이름이null인지 또는 비어있는지 확인하고 있지만,isBlank()는 자바 11 이상에서만 사용 가능합니다. 만약 호환성을 고려한다면name.trim().isEmpty()를 사용하는 것이 좋습니다. -
문자 길이 검사: UTF-8 인코딩에서의 바이트 길이를 검사하는 것은 문자 기반의 길이 제한(예: 100자)을 다루기가 복잡할 수 있습니다. 실제 사용하는 데이터베이스에서도 문자열의 길이를 체크하도록 설계를 고려해볼 것을 추천합니다.
-
예외 메시지: 예외 메시지가 한국어로 잘 작성되었습니다. 그러나 국제화(i18n)를 고려한다면 메시지를 외부 리소스로 분리하는 것도 좋은 방법입니다.
-
접근 제어자: 생성자를
protected로 설정한 것은 클래스의 상속을 고려할 때 적절합니다. 하지만 다른 패턴 (예: 빌더 패턴)을 고려하는 것도 좋습니다. -
하드 코딩된 값: 최대 길이와 같은 값을 하드코딩하지 않고 상수로 빼내어 관리하는 것이 더 좋습니다. 이후 유지 보수나 변경 시 용이합니다.
-
테이블 이름 및 도메인 일관성:
@Table(name = "projects")와 클래스명 간의 일관성을 잘 반영했으나, 테이블 이름도 상수로 관리하면 더욱 깔끔합니다. -
필드에 대한 주석 추가: 각 필드에 대해 설명 주석을 추가하면 코드 가독성이 높아질 수 있습니다.
이러한 점들을 검토하시고 수정사항을 고려해보시면 좋겠습니다.
| } | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰 결과는 다음과 같습니다.
-
예외 메시지 유효성: 예외가 발생할 때의 메시지가 하드코딩 되어 있습니다. 메시지를 처리하는 상수를 정의해두면 유지보수에 더 좋습니다.
-
null 체크:
null에 대한 테스트 케이스가 이미 존재하지만, 클래스 구현에서from메소드가null을 적절히 처리하고 있는지 확인이 필요합니다. 예외를 던지는지 여부를 명확히 해야 합니다. -
테스트 케이스: 이름 길이에 대한 테스트가 포함되어 있지만, 특정 길이(P마찬가지로) 값에 대한 예외 발생 조건을 명시적으로 검증하는 것이 좋습니다.
-
메서드 명명 규칙:
testFromMethodSuccess는 좀 더 의미 있는 이름으로 변경할 수 있습니다. 예를 들어,createsProjectSuccessfully_withValidName같은 이름은 테스트 의도를 더 명확히 드러냅니다. -
접근 제어자:
exceptionValues메소드는 테스트 내에서만 사용되므로private로 남기는 것이 좋습니다. -
주석 추가: 각 테스트 메서드에 간단한 설명 주석을 추가하면 코드 이해에 도움이 됩니다.
이 외에도 코드 실행 및 환경 설정에서의 버그 위험이 없는지 재확인하는 것이 좋습니다.
🚀 개발 사항
이슈 번호
특이 사항 🫶