Skip to content
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

Feat/#15 Project 도메인 모델 구현 #29

Merged
merged 3 commits into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package info.logbat.domain.project.domain;

import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.GeneratedValue;
import jakarta.persistence.GenerationType;
import jakarta.persistence.Id;
import jakarta.persistence.Table;
import java.time.LocalDateTime;
import lombok.AccessLevel;
import lombok.Getter;
import lombok.NoArgsConstructor;
import org.hibernate.annotations.SoftDelete;
import org.springframework.data.annotation.CreatedDate;
import org.springframework.data.annotation.LastModifiedDate;

@Entity
@Getter
@SoftDelete
@Table(name = "projects")
@NoArgsConstructor(access = AccessLevel.PROTECTED)
tidavid1 marked this conversation as resolved.
Show resolved Hide resolved
public class Project {

@Id
@GeneratedValue(strategy = GenerationType.AUTO)
private Long id;

@Column(name = "name", nullable = false, length = 100)
private String name;
tidavid1 marked this conversation as resolved.
Show resolved Hide resolved

@CreatedDate
@Column(name = "created_at")
private LocalDateTime createdAt;

@LastModifiedDate
@Column(name = "updated_at")
private LocalDateTime updatedAt;
tidavid1 marked this conversation as resolved.
Show resolved Hide resolved

private Project(String name) {
this.name = name;
}

public static Project from(String name) {
if (name == null || name.isBlank()) {
throw new IllegalArgumentException("잘못된 이름 요청입니다.");
}
tidavid1 marked this conversation as resolved.
Show resolved Hide resolved
return new Project(name);
}

}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 리뷰 결과는 다음과 같습니다:

버그 위험

  1. SoftDelete 주석: @SoftDelete 어노테이션을 추가했는데, 이를 사용하려면 관련된 설정(예: hibernate.enable_lazy_load)이 필요할 수 있습니다. 설정 미비 시 의도한 대로 작동하지 않을 위험이 있습니다.

  2. 생성자 접근 제한: @NoArgsConstructor(access = AccessLevel.PROTECTED)와 같이 설정했지만, 이는 프레임워크에 의해 호출될 수 있으므로, 정적 팩토리 메소드를 사용하는 것이 좋습니다.

  3. 쪽지의 생성 및 수정 날짜: @CreatedDate@LastModifiedDate가 제대로 작동하기 위해서는 해당 필드들을 관리하는 적절한 리스너 또는 설정이 필요합니다. 그렇지 않으면 날짜가 올바르게 기록되지 않을 수 있습니다.

개선 사항

  1. Validation 강화: from 메소드에서 이름이 null인지 체크하고 빈 문자열인지 검사하지만, 추가적인 유효성 검사를 고려할 수 있습니다(예: 특정 형식, 길이 체크 등).

  2. Javadoc 추가: 클래스와 각 메소드에 대한 설명을 추가하여 코드 이해도를 높일 수 있습니다.

  3. Null 체크 통일성: from 메소드에서 null 체크 시 String을 직접 비교하기보다는 Optional을 사용할 수도 있습니다. 이렇게 하면 코드의 가독성이 향상될 수 있습니다.

  4. 테스트 케이스 작성: 이 설계가 올바르게 작동하는지 확인하기 위해 단위 테스트와 통합 테스트를 포함시키는 것이 좋습니다.

결론

전반적으로 잘 작성된 코드입니다. 몇 가지 유의해야 할 점과 더불어 개선할 여지가 있는 사항들이 있으며, 이를 통해 더 안정적이고 유지 보수가 쉬운 코드를 만들 수 있을 것입니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 리뷰 결과입니다:

  1. Null 체크: from 메소드에서 이름이 null인지 또는 비어있는지 확인하고 있지만, isBlank()는 자바 11 이상에서만 사용 가능합니다. 만약 호환성을 고려한다면 name.trim().isEmpty()를 사용하는 것이 좋습니다.

  2. 문자 길이 검사: UTF-8 인코딩에서의 바이트 길이를 검사하는 것은 문자 기반의 길이 제한(예: 100자)을 다루기가 복잡할 수 있습니다. 실제 사용하는 데이터베이스에서도 문자열의 길이를 체크하도록 설계를 고려해볼 것을 추천합니다.

  3. 예외 메시지: 예외 메시지가 한국어로 잘 작성되었습니다. 그러나 국제화(i18n)를 고려한다면 메시지를 외부 리소스로 분리하는 것도 좋은 방법입니다.

  4. 접근 제어자: 생성자를 protected로 설정한 것은 클래스의 상속을 고려할 때 적절합니다. 하지만 다른 패턴 (예: 빌더 패턴)을 고려하는 것도 좋습니다.

  5. 하드 코딩된 값: 최대 길이와 같은 값을 하드코딩하지 않고 상수로 빼내어 관리하는 것이 더 좋습니다. 이후 유지 보수나 변경 시 용이합니다.

  6. 테이블 이름 및 도메인 일관성: @Table(name = "projects")와 클래스명 간의 일관성을 잘 반영했으나, 테이블 이름도 상수로 관리하면 더욱 깔끔합니다.

  7. 필드에 대한 주석 추가: 각 필드에 대해 설명 주석을 추가하면 코드 가독성이 높아질 수 있습니다.

이러한 점들을 검토하시고 수정사항을 고려해보시면 좋겠습니다.

Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package info.logbat.domain.project.repository;

import info.logbat.domain.project.domain.Project;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.stereotype.Repository;

@Repository
public interface ProjectJpaRepository extends JpaRepository<Project, Long> {

}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 리뷰를 진행하겠습니다.

  1. 패키지 구조: 패키지 네이밍은 적절하지만, 모듈의 용도에 따라 추가적인 하위 패키지를 고려할 수 있습니다. 예를 들어, 특정 기준으로 분류하고자 하는 도메인이 있다면 더 세분화할 수 있습니다.

  2. JpaRepository 사용: JpaRepository를 상속받아 기본 CRUD 작업을 제공하는 것은 좋은 접근입니다. 하지만 추가적인 쿼리 메서드가 필요할 수 있으며, 이후 비즈니스 로직에 맞는 커스텀 메서드를 정의할 필요가 있습니다.

  3. 주석 부족: 현재 클래스에 문서화된 주석이 없습니다. 이 인터페이스의 기능과 용도를 설명하는 주석이 있으면 유지보수가 쉬워질 것입니다.

  4. 예외 처리: JPA를 사용할 때 데이터베이스와의 상호작용에서 발생할 수 있는 예외 상황에 대한 처리가 필요합니다. 필요한 경우 적절한 위치에 서비스 레이어에서 예외 처리를 구현하십시오.

  5. 테스트 코드: 이 리포지토리에 대한 유닛 테스트나 통합 테스트 코드가 없으면 실제 작동 여부를 검증하기 어렵습니다. 테스트 코드를 작성하여 안정성을 높이는 것이 좋습니다.

종합적으로, 이 리포지토리는 기본적인 구조를 잘 갖추고 있으나, 향후 확장성과 유지를 위해 위의 사항들을 고려하며 개선하는 것이 좋습니다.

Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package info.logbat.domain.project.domain;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.util.stream.Stream;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

@DisplayName("Project 도메인은")
class ProjectTest {

@DisplayName("생성시")
@Nested
class whenCreated {

@DisplayName("이름이 잘못된 경우 예외를 던진다.")
@ParameterizedTest
@MethodSource("exceptionValues")
void throwsExceptionForInvalidName(String name) {
// Act & Assert
assertThatThrownBy(() -> Project.from(name))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("잘못된 이름 요청입니다.");
}

@Test
@DisplayName("정상적으로 생성된다.")
void testFromMethodSuccess() {
// Arrange
String expectedProjectName = "이름";
// Act
Project actualResult = Project.from(expectedProjectName);
// Assert
assertThat(actualResult)
.hasFieldOrPropertyWithValue("name", expectedProjectName);
}


private static Stream<Arguments> exceptionValues() {
return Stream.of(
Arguments.of(""),
Arguments.of((String) null),
Arguments.of(" ")
);
}

}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 리뷰를 진행하겠습니다.

버그 리스크

  1. 얕은 복사 및 불변성: Project.from(name) 메소드에서 name 필드의 불변성을 보장해야 합니다. 예외 처리에서는 문제가 없지만, 이름이 변경되지 않도록 관리할 필요가 있습니다.

  2. 예외 메시지 일관성: 던지는 예외의 메시지가 하드코딩되어 있는데, 코드 수정 시 명시적으로 다루어야 할 부분입니다. 상수로 정의하여 관리하는 것이 좋습니다.

개선 제안

  1. 요구 사항 추가: Project 클래스의 생성 조건에 대해 더 구체적인 요구 사항을 문서화하거나 주석으로 남기는 것이 좋습니다. 예를 들어, 최대 길이나 허용되는 문자 등.

  2. 테스트 케이스 확장: 다른 잘못된 이름 값도 테스트 케이스에 추가해보세요. 예를 들어, 너무 긴 문자열이나 특정 특수문자를 포함한 경우 등.

  3. 예외 처리 테스트 강화: assertThatThrownBy()를 사용한 예외 테스트는 좋지만 다양한 예외 상황에 대한 검증을 추가하면 더 견고해질 것입니다.

  4. 테스트 메소드 네이밍: 테스트 메소드 이름을 짓는 데 있어 간결함과 명확성을 유지하세요. 예를 들어, throwsExceptionForInvalidName 대신 shouldThrowExceptionForInvalidName 같은 네이밍이 더 나을 수 있습니다.

전반적으로 기본적인 구조는 잘 되어 있으며, 작은 개선만 필요합니다. Test-driven 개발(TDD) 원칙을 잘 따르고 있는 것으로 보여지네요.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 리뷰 결과는 다음과 같습니다.

  1. 예외 메시지 유효성: 예외가 발생할 때의 메시지가 하드코딩 되어 있습니다. 메시지를 처리하는 상수를 정의해두면 유지보수에 더 좋습니다.

  2. null 체크: null에 대한 테스트 케이스가 이미 존재하지만, 클래스 구현에서 from 메소드가 null을 적절히 처리하고 있는지 확인이 필요합니다. 예외를 던지는지 여부를 명확히 해야 합니다.

  3. 테스트 케이스: 이름 길이에 대한 테스트가 포함되어 있지만, 특정 길이(P마찬가지로) 값에 대한 예외 발생 조건을 명시적으로 검증하는 것이 좋습니다.

  4. 메서드 명명 규칙: testFromMethodSuccess는 좀 더 의미 있는 이름으로 변경할 수 있습니다. 예를 들어, createsProjectSuccessfully_withValidName 같은 이름은 테스트 의도를 더 명확히 드러냅니다.

  5. 접근 제어자: exceptionValues 메소드는 테스트 내에서만 사용되므로 private로 남기는 것이 좋습니다.

  6. 주석 추가: 각 테스트 메서드에 간단한 설명 주석을 추가하면 코드 이해에 도움이 됩니다.

이 외에도 코드 실행 및 환경 설정에서의 버그 위험이 없는지 재확인하는 것이 좋습니다.