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

[ADD] entity 초기 구현 #2

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

[ADD] entity 초기 구현 #2

wants to merge 8 commits into from

Conversation

sunseo18
Copy link
Member

아직 엔티티 반 정도밖에 구현 못했구요!

그리고 전에 말했던 푸시 알림 세팅 fcmtoken 테이블에 합치려했던거 생각해보니까 fcm 토큰 삭제될 때마다 세팅 정보도 같이 사라지게 돼서
결국 분리하는 거로 결정했답니다 :)

Copy link
Contributor

@jinsu4755 jinsu4755 left a comment

Choose a reason for hiding this comment

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

일단 진행 중인 부분에 대해서 리뷰 남겼습니다~~

리뷰 중에 통일해주면 좋을 내용 등이 있기 때문에 이를 확인하고 수정해보는 것도 좋을 것 같아요~


@MappedSuperclass
@EntityListeners(AuditingEntityListener::class)
abstract class BaseTimeEntityCreated {
Copy link
Contributor

Choose a reason for hiding this comment

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

지금도 객체의 이름이 나쁘지는 않다고 생각은 들지만 조금 더 이해하기 쉬운 역할의 이름을 부여해 주는 것이 어떨까요?

이 추상 클래스가 해주고 싶은 역할은 결국 어떤 것일까요?
그 역할 이름을 부자연스러운 단어의 조합보다 그 역할 자체를 쉽게 풀어 쓰는 단어를 붙여주는 것이 좋지 않을까요?
마치 지금은 이름 자체만 보면 BaseTime 엔티티 생성된 과 같은 순서로 읽혀 정확히 어떤 일을 처리하고 싶은지 알기 힘든 것 같은 느낌이 듭니다.

Comment on lines 15 to 19
@CreatedDate
@Column(nullable = false, updatable = false)
@JsonFormat(pattern = "yyyy-MM-dd HH:mm:ss")
var createdAt: LocalDateTime = LocalDateTime.now()
protected set
Copy link
Contributor

Choose a reason for hiding this comment

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

modifiedAt 을 다루는 곳에서는 JsonFormat 을 사용하고 있는데 이로 인해 BaseTimeEntityCreated에서 createAt에 대한 포맷 문제가 있진 않을까요?
혹은 포맷 뿐 아니라 만약 컬럼명이 변하는 경우 두 파일을 모두 수정해야 하는 상황이 발생할 수 있는데
이 처럼 수정으로 인한 불일치가 생기지 않도록 처리가 필요하지 않을까요?

) : BaseTimeEntityModified() {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
var id: Long? = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

id에 대해 디폴트 초기화 값을 통일해주는 것이 좋을 것 같습니다!

본인만의 정확한 기준을 만들어 null 혹은 Default value 중 선택을 해보는 것이 어떨까요?

) : BaseTimeEntityModified() {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
val id: Long = 0; // ㅜㅜ 이거 어카지...
Copy link
Contributor

Choose a reason for hiding this comment

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

val 의 경우 자바의 final 변수와 동일하게 한번 선언 후 값을 변경할 수 없습니다.
만약 id 가 final 일 경우 어떤 문제가 발생할 수 있을까요? 혹은 예상되는 문제가 없을까요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants