Refactor: #4의 DataEntity 구현을 수정합니다 #6
Conversation
<description> memberFk의 ForeignKey 제한 조건은 설정하지 않았습니다. 위치 컬럼의 SRID를 추가하였습니다. 칼럼별 길이에 관한 제한 조건과 nullable 조건을 추가하였습니다.
<description> memberFk,trafficFk의 ForeignKey 제한 조건은 설정하지 않았습니다. 칼럼별 길이에 관한 제한 조건과 nullable 조건을 추가하였습니다.
<description> 별도의 id 전략이 필요한 경우 객체를 새로 생성하거나 해당 엔티티에서 id를 설정합니다.
<description> BaseEntity를 상속하지 않고 기본 정보를 엔티티 안에서 구현합니다. 위치 컬럼의 SRID를 추가하였습니다.
<description> web 패키지 아래가 아닌 api 패키지 아래로 위치하는 것이 적절하다 판단하였습니다.
<description> 해당 테스트의 경우 테스트 레포에서 진행하여 삭제합니다. Capstone-Walking/Capstone-Walking-BE-TEST#3
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -29,7 +29,7 @@ | |||
| public abstract class BaseEntity { | |||
|
|
|||
| @Id | |||
| @GeneratedValue(strategy = GenerationType.SEQUENCE) | |||
| @GeneratedValue(strategy = GenerationType.IDENTITY) | |||
| private Long id; | |||
|
|
|||
| @Column(nullable = false, updatable = false) | |||
There was a problem hiding this comment.
코드 리뷰:
- 변경된 부분에서 ID 생성 전략이 GenerationType.SEQUENCE에서 GenerationType.IDENTITY로 변경되었습니다. 데이터베이스에 따라 IDENTITY 전략은 메모리 부족 문제를 유발할 수 있으므로 주의해야 합니다.
- BaseEntity 클래스에 다른 필드나 메서드가 있는지 확인하세요. 그것이 명시되어 있지 않습니다.
- JavaDoc이나 주석을 추가하여 클래스 및 속성에 대한 설명을 더 자세히 달아보세요.
- 테스트 코드를 작성하여 새 전략이 예상대로 작동하는지 확인하세요.
- 타입 및 어노테이션 사용에 대한 커뮤니티의 권장 사항을 따르는 것이 좋습니다.
|
|
||
| public enum CertificationSubject { | ||
| KAKAO, | ||
| } |
There was a problem hiding this comment.
이 코드 패치는 간단한 enum을 선언하는 것으로 보입니다. 여러 가지 사항에 대해 고려할 수 있습니다:
- 다른 인증 주체나 유형의 경우를 나중에 추가할 계획이 있는지 확인해야 합니다. 그럴 경우,
KAKAO외에도 값을 추가해야 합니다. CertificationSubjectenum이 현재 한 값만 가지고 있어 확장 가능성을 고려하셔서 미래에 다른 값이 필요할 수 있음을 염두에 두세요.- Java naming conventions을 준수하고 있는지 확인하시기 바랍니다.
이외에는 이 코드 스니펫이 단순하고 문제가 없어 보입니다.
|
|
||
| @Builder.Default | ||
| @Column(nullable = false, columnDefinition = "json") | ||
| private String resource = "{}"; | ||
| } |
There was a problem hiding this comment.
이 코드 패치의 코드 리뷰:
@Enumerated를 사용할 때,EnumType.STRING을 명시하는 것은 좋은 접근 방식입니다. 그러나certificationSubject와status필드에 있는@Builder.Default는 필요 없어 보입니다.- 코드 상에서 나타나는 주석(Comment)은 코드 읽기를 돕지만, 오탈자가 있습니다. 수동 검토 및 수정이 필요합니다.
profile과resource필드의 컬럼 타입이json으로 지정되어 있으므로 ORM 프레임워크와 DB 버전 확인이 필요합니다.- 문자열 길이를 더 엄격하게 체크하거나 validation을 추가하는 것이 유용할 수 있습니다.
개선 제안:
- 모든 필드 옵션들을 일관된 스타일로 정렬하여 가독성을 향상시킵니다.
- 필요한 경우, Validation annotation을 추가하여 데이터 유효성을 강화합니다.
- 그 외 개인적인 코딩 스타일에 맞춰 코드 스타일 가이드를 엄격히 지키면서 작업합니다.
| MemberStatus(String description) { | ||
| this.description = description; | ||
| } | ||
| } |
There was a problem hiding this comment.
이 코드는 깔끔하고 간단한 열거형(MemberStatus)을 정의하고 있습니다. 개선할 점으로는 다음과 같은 사항을 고려할 수 있습니다:
- 주석: 코드에 주석을 추가하여 각 회원 상태가 무엇을 나타내는지 더 명확히 설명할 수 있습니다.
- 검증 및 예외 처리: 입력 값에 대한 유효성 검사나 처리를 추가해야 할 수 있습니다.
- toString() 메서드 오버라이드: 추가로 필요한 경우,
toString()메서드를 오버라이딩하여 좀 더 유용한 정보를 출력할 수 있습니다.
위 내용은 코드 리뷰에서 고려될 수 있는 세 가지 포인트입니다.
| private String startAlias; | ||
|
|
||
| @Column(nullable = false) | ||
| @Column(nullable = false, length = 50) | ||
| private String endAlias; | ||
| } |
There was a problem hiding this comment.
이 코드는 좋아보입니다. 몇 가지 제안사항은 다음과 같습니다:
startPoint,endPoint, 및path의nullable속성을false로 설정하여 필수 필드임을 명시적으로 표현합니다.- 외래 키 (
memberFk)에ConstraintMode.NO_CONSTRAINT를 사용하는 것은 데이터베이스 무결성을 일부 부여해 줍니다. - 이 코드에서 길이 제약이 있는 문자열 필드 (50자)를 정의했는데, 데이터의 적절한 길이를 파악 후 조정이 필요할 수 있습니다.
위의 제안을 고려하여 코드가 확장 가능하고 유지 보수가 쉬워질 수 있도록 하는 것이 좋습니다.
|
|
||
| @Column(nullable = false) | ||
| @Column(nullable = false, length = 50) | ||
| private String alias; | ||
| } |
There was a problem hiding this comment.
이 코드 패치는 다음과 같은 변경을 포함합니다:
memberEntity를memberFk로 변경하고, foreign key constraint 없이 설정되도록 합니다.trafficEntity를trafficFk로 변경하고, foreign key constraint 없이 설정되도록 합니다.alias필드에 길이 제한인length = 50을 추가했습니다.
개선 제안:
- 왜
memberEntity,trafficEntity가 이름에서 FK(Foreign Key)를 의미하는Fk로 변경되었는지 주석으로 설명하는 것이 도움이 됩니다. @Column어노테이션의 속성에 대해 주석으로 설명을 추가하는 것이 좋습니다. (예:nullable및length)
버그 리스크:
- 외래 키 제약 조건을 강제로 없앤다는 점에서 데이터 무결성 문제 발생 가능성이 있을 수 있습니다. 수정될 수 있습니다.
이 외에도 코드 구조 및 목적에 따라 다른 개선 사항이 있을 수 있습니다.
| private void preRemove(TrafficEntity entity) { | ||
| entity.delete(); | ||
| } | ||
| } |
There was a problem hiding this comment.
이 코드는 JPA Entity의 Soft Delete를 지원하기 위한 Listener로 보입니다. 해당 리스너는 TrafficEntity가 삭제되기 전에 delete() 메서드를 호출하여 소프트 삭제를 트리거합니다.
개선 제안:
delete()메서드에 대한 구현이 이 코드 조각에서만 사용되는 것으로 보이므로 해당 메서드가 어떻게 정의되었는지 확인이 필요합니다.- 코드가 완전하지 않으면 일관성을 유지하기 위해 주석을 추가하는 것이 좋습니다.
- 리스너 메서드가
private으로 선언되어 있으므로 JPA가 올바르게 작동하려면public으로 변경해야 합니다.
버그 리스크:
@PreRemove애노테이션은 파라미터 없는 메소드에 적용되어야 하며, 여기에서는TrafficEntity를 파라미터로 갖고 있는 메소드로 사용했습니다. JPA 스펙에 따라 이렇게 하면 예상대로 동작하지 않을 수 있습니다.
코드 리뷰 결과, 리스너 메서드 시그니처를 수정하고 접근 제어자를 public으로 변경하면 이 코드 조각은 더욱 안정적으로 작동할 것으로 예상됩니다.
|
|
||
| public void delete() { | ||
| this.deleted = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
이 코드는 대체로 잘 구성되어 있지만 몇 가지 개선 사항이 있습니다:
-
버그 및 위험 상황:
TrafficEntity클래스에 삭제 관련 로직이 있으나, 이를 저장하는 부분은 누락되었습니다. 해당 엔티티가 실제로 삭제되지 않고 데이터베이스에서 보존되는 문제가 있을 수 있습니다.id필드는@GeneratedValue(strategy = GenerationType.SEQUENCE)옵션으로 설정되어 있지만 SEQUENCE가 정의되지 않았습니다. 이는 ID 생성 방법에 대한 부적절한 설정입니다.
-
개선 제안:
delete()메서드가 있지만 이 메서드를 어떻게 사용할 지 명확하지 않습니다. 해당 메서드와 함께 사용해야 하는 다른 메서드나 프로세스가 필요합니다.createdAt및updatedAt필드를LocalDateTime으로 유지하는 것 대신Instant를 사용하는 것이 시간 관련 데이터를 다룰 때 더 안전하고 권장됩니다.deleted필드는 Boolean 값으로 갱신되며 ObjectMapper를 통해 수정되어 이슈를 초래할 수 있습니다. 따라서 delete() 메서드를 통해 논리적인 삭제를 처리하는 방법이 더 좋을 수 있습니다.
-
기타 고려 사항:
@Builder어노테이션을 사용하여 생성자 자동화를 사용하는 경우, 필수 입력 매개변수와 선택적 매개변수를 구분하는 것이 중요합니다.
좀 더 명확하고 안정적인 코드를 작성하기 위해 위의 제안을 고려해보세요.
| order_updates: true | ||
| jdbc: | ||
| batch_size: ${JDBC_BATCH_SIZE:50} No newline at end of file |
There was a problem hiding this comment.
주요 변경 사항은 Hibernate dialect가 MySQL8SpatialDialect로 설정되었으며 insert 및 update 문장이 순서화되었습니다. jdbc의 batch_size도 설정되었습니다.
개선 제안:
- 적절한 권한이 없는 상황에서 자바를 실행하는 것을 방지하기 위해 보안 강화가 필요할 수 있습니다.
- 설정 값들이나 버전 관리 시스템과 같은 곳에 연결된 값들을 하드코딩하는 대신, 외부 속성 파일을 사용하여 값을 설정하는 것이 좋습니다.
- 주석을 추가하여 각각의 설정이 어떤 역할을 하는지 설명하는 것이 도움이 될 수 있습니다.
- 예기치 않은 입력값 오류를 방지하기 위해 입력 유효성 검사를 추가하는 것이 바람직합니다.
이 코드는 전반적으로 안정적으로 보입니다.
| order_inserts: true | ||
| order_updates: true | ||
| jdbc: | ||
| batch_size: 50 |
There was a problem hiding this comment.
이 코드는 Hibernate 구성을 업데이트하고 있습니다. 개선 제안 및 버그 위험은 다음과 같습니다:
-
"order_inserts" 및 "order_updates" 옵션은 성능 향상을 위해 사용될 수 있지만 주의할 점이 있습니다. 이 옵션들은 쿼리 수행 순서를 제어하기 때문에 데이터베이스와의 상호작용이 변경될 수 있습니다. 주의하여 사용해야 합니다.
-
"jdbc" 아래의 "batch_size"를 50으로 설정하면 일괄 작업 시 데이터베이스와 한 번에 상호작용하는 레코드 수를 제한합니다. 이는 대량 처리 시에 유용할 수 있으나, 크기 조정이 필요할 수도 있습니다. 실제 환경에서 최적의 값을 찾아야 합니다.
-
오타나 잘못된 키워드 없이 YAML 구문이 정확하게 작성되었습니다.
-
주석을 추가하여 각 옵션이 왜 설정되었는지 명시하는 것이 좋을 수 있습니다. 코드를 이해하는 데 도움이 될 것입니다.
-
보안 상의 이슈가 있는지, Hibernate 버전이나 MySQL8SpatialDialect 호환성을 확인해볼 필요가 있습니다.
이러한 제안을 고려하여 위 코드 패치를 개선할 수 있습니다.
| order_updates: true | ||
| jdbc: | ||
| batch_size: ${JDBC_BATCH_SIZE:50} No newline at end of file |
There was a problem hiding this comment.
이 코드 패치를 간단히 검토해보겠습니다.
-
개선 사항:
properties섹션 내부의 들여쓰기를 일관되게 맞추어주세요.- 코딩 스타일을 위해 파일 끝에 빈 줄을 추가하는 것이 좋습니다.
-
버그 리스크:
- 주요한 버그 리스크는 없어 보입니다.
-
추가 변경 사항:
- 멋진 작업입니다.
dialect,order_inserts,order_updates, 그리고batch_size와 같은 속성을 추가하여 설정 내용을 확장했습니다.
- 멋진 작업입니다.
코드 리뷰 후에 수정이 필요하다고 생각되는 부분에 대해 언급했습니다. 이 변경 사항들을 고려하여 코드 품질을 향상시킬 수 있을 것입니다.
| order_updates: true | ||
| jdbc: | ||
| batch_size: ${JDBC_BATCH_SIZE:50} No newline at end of file |
There was a problem hiding this comment.
이 코드 패치에 대한 간단한 코드 리뷰:
-
개선 제안:
- "format_sql"의 중복 선언을 정리할 수 있습니다.
- "dialect"를 설정하는 것은 좋지만, 특정 MySQL 버전에 맞게 설정하는 것이 중요합니다.
- 일괄 삽입과 업데이트를 위한 "order_inserts"와 "order_updates" 사용은 좋은 방향입니다.
- "jdbc" 설정에 "batch_size"를 동적으로 변경 가능하도록 환경 변수로부터 값을 읽는 것이 좋습니다.
-
버그 리스크:
- 파일 끝에 새로운 줄이 없는 문제가 두 번 발생하고 있는데, 이는 코드 유지 관리 측면에서 주의해야 합니다.
- Hibernate 및 Spring 프레임워크 버전 간 호환성 체크가 필요합니다.특히, 사용 중인 Hibernate Spatial Dialect가 MySQL 8과 호환되는지 확인해야 합니다.
🎫 연관 티켓
이슈 없이 진행하였습니다.
🙏 작업
DB의 FK 조건이 없는 경우 장점
💁♂️ 어떻게?
🙈 PR 참고 사항
📸 스크린샷
🤖 테스트 체크리스트