-
Notifications
You must be signed in to change notification settings - Fork 2
feat(be) : Clean up code and add tests #74
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
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.
🤖 AI 리뷰 - src/main/kotlin/com/back/koreaTravelGuide/domain/userChat/chatmessage/dto/ChatMessageResponse.kt
🟢 좋은점:
- Kotlin data class를 적절히 사용하며, null safety를 고려한 필드 정의 (e.g.,
id: Long?nullable 처리)가 잘 적용됨. 이는 Kotlin 최적화 규칙을 준수. - 타임존(Asia/Seoul)을 명시적으로 처리하여 ZonedDateTime으로 변경한 점은 시간 관련 로직의 안정성을 높여 Kotlin 최적화 측면에서 긍정적임.
🟡 개선사항:
createdAt의 기본값을ZonedDateTime.now(ZoneId.of("Asia/Seoul"))로 설정한 것은 편의성을 주지만, DTO 생성 시점에 따라 현재 시간이 아닌 엔티티의 실제 생성 시간을 반영하는 것이 더 나음. companion object의from()함수에서message.createdAt(Instant)을ZonedDateTime.ofInstant(message.createdAt, ZoneId.of("Asia/Seoul"))로 변환하여 명시적으로 매핑하는 것을 추천. 기본값은 제거하거나 옵셔널로 고려.- ktlint 규칙 준수를 위해 import 문을 알파벳 순으로 정렬 (e.g.,
java.time.Instant제거 후ZoneId와ZonedDateTime만 유지)하고, 불필요한 공백을 확인. 네이밍 컨벤션은 camelCase로 잘 지켜짐.
🔴 문제점:
- companion object의
from()함수가 변경되지 않은 상태로 보이는데,ChatMessage의createdAt이 여전히Instant타입이라면 ZonedDateTime으로의 변환 로직이 누락되어 컴파일 에러 또는 런타임 불일치가 발생할 수 있음. 반드시from()내부에서 타입 변환을 추가해야 함 (e.g.,createdAt = ZonedDateTime.ofInstant(message.createdAt, ZoneId.of("Asia/Seoul"))). 이는 Kotlin 최적화(타입 안전성)와 ktlint(일관된 포맷팅)를 위반할 위험이 있음.
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.
🤖 AI 리뷰 - src/main/kotlin/com/back/koreaTravelGuide/domain/userChat/chatmessage/entity/ChatMessage.kt
🟢 좋은점:
- Kotlin 최적화 측면에서 data class를 적절히 사용하며, 필드의 null safety를
@Column(nullable = false)로 명확히 보장하고 있음. ZonedDateTime으로 변경하여 타임존(Asia/Seoul)을 명시적으로 처리하는 것은 시간 데이터의 일관성과 지역성을 강화하는 좋은 선택으로, Kotlin의 타입 안전성을 활용한 최적화 예시임. - ktlint 규칙 준수: import 문의 순서와 포맷팅이 깔끔하며, 네이밍 컨벤션(예: createdAt)이 일관적임. 변경 diff에서도 불필요한 공백이나 들여쓰기 문제가 없음.
🟡 개선사항:
- Kotlin 최적화 관점에서 data class의 기본값(
ZonedDateTime.now(ZoneId.of("Asia/Seoul")))은 객체 생성 시점에 평가되므로, 여러 엔티티 인스턴스가 동시에 생성될 때 약간의 시간 차이가 무시될 수 있음. 더 정확한 생성 시간을 위해 JPA의@PrePersist라이프사이클 콜백을 추가하거나, 서비스 레이어에서 생성 시간을 주입하는 방식을 고려하면 좋을 수 있음 (예:createdAt: ZonedDateTime?으로 nullable하게 두고, 생성 시 설정). - 타임존 설정이 하드코딩되어 있으므로, 애플리케이션 프로퍼티(예: application.yml의
spring.jpa.properties.hibernate.jdbc.time_zone=Asia/Seoul)나 환경 변수로 동적으로 관리하면 유지보수성이 높아질 수 있음. 이는 Kotlin의 확장함수나 컴패니언 객체를 활용해 캡슐화할 여지가 있음.
🔴 문제점:
- 없음. 변경이 JPA 엔티티의 안정성을 해치지 않으며, 글로벌 익셉션 처리나 ApiResponse 규칙과 무관한 부분이므로 문제없음. 다만, 데이터베이스 스키마 변경(Instant → ZonedDateTime)이 발생할 수 있으니 마이그레이션 도구(Flyway/Liquibase)로 확인 필요.
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.
🤖 AI 리뷰 - src/main/kotlin/com/back/koreaTravelGuide/domain/userChat/chatroom/controller/ChatRoomController.kt
🟢 좋은점:
- ApiResponse를 사용해 응답을 표준화하고 있으며, ResponseEntity.ok()으로 성공 응답을 적절히 처리함. 이는 필수 검토 항목 2번을 잘 준수함.
- Kotlin의 null safety를 활용해
requesterId ?: throw AccessDeniedException(...)으로 인증 체크를 안전하게 처리함. 이는 필수 검토 항목 3번(Kotlin 최적화)의 null safety 부분을 잘 반영함. - ktlint 규칙(포맷팅, 네이밍 컨벤션)을 위반하지 않음. 메서드 호출과 변수 네이밍이 일관적임.
🟡 개선사항:
- 메서드 호출을
checkOneToOneRoom에서createOneToOneRoom으로 변경했으나, 이 변경이 기존 1:1 채팅방이 이미 존재할 때 중복 생성을 방지하는 idempotent 로직을 보장하는지 확인 필요. 만약 service 레이어에서 체크-생성 패턴(예: when 표현식으로 기존 방 조회 후 생성)을 사용한다면 더 명확히 문서화하거나 확장 함수로 추출하는 Kotlin 최적화(항목 3번)를 고려할 수 있음. - 글로벌 익셉션 처리(항목 1번)를 위해 AccessDeniedException 외에
createOneToOneRoom메서드에서 발생할 수 있는 다른 예외(예: 이미 존재하는 방 충돌, DB 오류)를 @ControllerAdvice에서 표준 에러 응답으로 매핑하는지 검토 추천. 현재 코드상으로는 직접 throw만 보이지만, service 레이어의 예외를 가정.
🔴 문제점:
- 없음. 변경된 부분이 간단하며, 주요 규칙을 위반하지 않음. 다만, 메서드 시그니처 변경으로 인한 컴파일/로직 오류(예: create 메서드가 check와 동일한 반환 타입을 보장하지 않음)가 발생할 수 있으니 빌드 테스트 필수.
YangHJ2415
left a comment
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.
확인 했습니다!
Work Details
refactor: Rename method and fix import path
feat: compute updatedAt in Asia/Seoul
-feat: Add Message,Room Test