Conversation
nilgil
commented
Jul 3, 2025
nilgil
commented
- 공통 엔티티 Email, PhoneNumber 추가
- 판매자 관련 도메인 엔티티 작성
nilgil
force-pushed
the
feat/7
branch
3 times, most recently
from
ad186ba to
c7baf52
Compare
f-lab-seb
left a comment
f-lab-seb
left a comment
There was a problem hiding this comment.
수고많으셨습니다~ 몇가지 코멘트 남겼어요.
|
|
||
| @Embeddable | ||
| data class SellerContactInfo( | ||
| @AttributeOverride(name = "value", column = Column(name = "email")) |
There was a problem hiding this comment.
@AttributeOverride 는 @Embeddable 에 붙이는걸까요 @Embedded 에 붙이는걸까요?
https://jakarta.ee/specifications/persistence/3.2/jakarta-persistence-spec-3.2#a14084
There was a problem hiding this comment.
@Embeddable
@AttributeOverride(name = "value", column = Column(name = "email"))
data class SellerContactInfo(
...이런식으로 사용하는 것과의 차이를 말씀하신게 맞을까요?
보내주신 링크에서는 지금과 같이 @Embedded에 붙이고있는 것 같습니다.
사용하는 측에서 매핑 정보를 Override하는 용도로 사용하는 것으로 이해하고있습니다.
There was a problem hiding this comment.
위 코드 보시면, @Embeddable 에 붙이고 계신것 처럼 보여서 말씀드렸어용!
@Embedded 가 명시되어 있지 않아서 혼란했던 것 같슴당 ㅎㅎ
| @Entity | ||
| class SellerRegistrationRequest( | ||
| val businessInfo: SellerBusinessInfo, | ||
| val contactInfo: SellerContactInfo, | ||
| val bankAccount: SellerBankAccount, | ||
| ) : BaseEntity() { |
There was a problem hiding this comment.
@Embedded 키워드는 생략하는게 더 좋을까요?
There was a problem hiding this comment.
"동작 잘 하니 불필요한 코드를 줄여 보자" 였지만, JPA 표준에도 맞지 않고, 이게 잘 동작한다는 걸 모르는 사람이 본다면 혼란스러울 것 같습니다. 다른 사람들과 함께 개발하는 것이 전제가 되어야 하는데 이 부분을 놓치고 있었던 것 같습니다.
| fun changeBankAccount(bankAccount: SellerBankAccount) { | ||
| this.bankAccount = bankAccount | ||
| } | ||
|
|
||
| fun changeIntroduction(introduction: String) { | ||
| this.introduction = introduction | ||
| } |
There was a problem hiding this comment.
setter 보다 명확한 의미를 가지는 메소드로 뽑아내신 것 좋은 시도 같습니다 👍👍
근데 현재구조라면, 외부에서 seller.backAccount = backAccount 행위를 막을 수 없을 것 같은데요.
코틀린 프로퍼티 setter 접근을 제한하려면 어떻게 하면 좋을까요?
There was a problem hiding this comment.
@Entity
class Seller(
bankAccount: SellerBankAccount,
) : BaseEntity() {
@Embedded
private var _bankAccount: SellerBankAccount = bankAccount
val bankAccount: SellerBankAccount
get() = _bankAccount이런식으로 백킹 프로퍼티를 사용하여 가능할 것 같습니다. 그런데 모든 var에 대해 이렇게 해줘야 할 것 같은데 너무 복잡해질 것 같다는 생각이 듭니다. 세터 사용 금지라는 규약을 정해두고(CI/CD에서 검증 등) 세터를 열어두는 트레이드 오프를 하는 것도 좋아 보입니다.
| val value: String, | ||
| ) { | ||
| init { | ||
| require(value.matches(Regex("^[A-Za-z0-9+_.-]+@[A-Za-z0-9.-]+$"))) { "Invalid email format" } |
There was a problem hiding this comment.
현재구조대로면 Email 인스턴스를 만들 때마다 Regex 인스턴스를 생성할 것 같은데, 의도하신게 아니라면 더 좋은 방법은 없을까요?
| @Embeddable | ||
| data class SellerBankAccount( | ||
| val bankName: String, | ||
| val accountNumber: String, | ||
| val accountHolder: String, | ||
| ) |
There was a problem hiding this comment.
불변객체를 만들고 VO 로 관리하도록 신경써주신 것 같네요 👍👍👍
|
Closes: #7 |
|
2 participants
