-
Notifications
You must be signed in to change notification settings - Fork 3
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
티켓 재고 관리 도메인 설계 #3
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.
감자 도메인 코드 작성하느라 고생했어요 😀
동시성 테스트도 적절하게 잘 작성해 주었네요.
코멘트 확인 및 반영 후 머지하면 될 것 같습니다.
import com.aengdulab.ticket.domain.Ticket; | ||
import org.springframework.data.jpa.repository.JpaRepository; | ||
|
||
public interface MemberTicketRepository extends JpaRepository<MemberTicket, Long> { |
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.
R: Repository 어노테이션이 존재하는 곳이 있고, 없는 곳이 있어요!
컨벤션을 맞추면 좋을 것 같은데, 감자 의견은 어떤 쪽을 선호하나요?
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.
JpaRepository를 상속하는 인터페이스라 어노테이션을 붙이지 않아도 스프링에서 자동으로 빈 등록이 돼서 크게 차이는 없는데, 저는 어노테이션으로 명시적 의미 부여를 하는 쪽을 조금 더 선호합니다. 일단 통일성을 위해 다 추가해 놓았는데 나중에 같이 얘기해 보면 좋을 것 같아요
.toList(); | ||
|
||
CountDownLatch latch = new CountDownLatch(threadCount); | ||
try (ExecutorService executorService = Executors.newFixedThreadPool(threadCount)) { |
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.
A: try-catch with resources를 사용해서 indent가 추가되네요! 이게 가독성을 저하한다고 생각해서 ExecutorService를 직접 shutdown하는 방법을 찾아 보았는데, auto-close를 사용하는 게 더 안전해 보이네요 :)
아 그리고, 테스트의 한글 메서드에 경고 뜨는 게 불편하면 클래스 수준에 |
|
||
@Entity | ||
@Getter | ||
@EqualsAndHashCode(of = "id") |
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.
@EqualsAndHashCode(onlyExplicitlyIncluded = true)
를 적용하는 건 어떤가요? 그럼 코드가 아래와 같이 변경될 거에요.
@EqualsAndHashCode(onlyExplicitlyIncluded = true)
public class Member {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
@EqualsAndHashCode.Include
private Long id;
}
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.
이런 어노테이션도 있군요, 하나 배워갑니다 👍
망쵸 코멘트 보고 엔티티의 equals, hashCode 재정의에 대해 생각해 봤습니다. 그런데 롬복을 통한 재정의에서 조금 걸리는 부분이 있더라구요.
public class EqualsTest {
@Test
void id가_없는_경우_동등하지_않다고_판단한다() {
Member member = new Member("member");
Ticket ticket = new Ticket("ticket", 10L);
MemberTicket firstMemberTicket = new MemberTicket(member, ticket);
MemberTicket secondMemberTicket = new MemberTicket(member, ticket);
assertThat(firstMemberTicket.equals(secondMemberTicket)).isFalse();
}
@Test
void id가_같은_경우_동등하다고_판단한다() {
MemberTicket firstMemberTicket = new MemberTicket(1L, new Member("member"), new Ticket("ticket", 10L));
MemberTicket secondMemberTicket = new MemberTicket(1L, new Member("member1"), new Ticket("ticket1", 1L));
assertThat(firstMemberTicket.equals(secondMemberTicket)).isTrue();
}
@Test
void id가_다른_경우_동등하지_않다고_판단한다() {
Member member = new Member("member");
Ticket ticket = new Ticket("ticket", 10L);
MemberTicket firstMemberTicket = new MemberTicket(1L, member, ticket);
MemberTicket secondMemberTicket = new MemberTicket(2L, member, ticket);
assertThat(firstMemberTicket.equals(secondMemberTicket)).isFalse();
}
}
위 테스트를 실행하면 다음과 같은 결과가 나옵니다.
즉, id가 모두 null인 경우는 true를 반환하는 거죠.
개인적으로 entity는 영속화가 되어 있는 상태에서는 id 필드만을 기준으로 비교하고, 영속화되지 않은 상태에서는 name이나 email 같은 비즈니스 키를 사용하는 방식으로 비교하는 것이 좋다고 생각해요. 비즈니스 키가 없는 경우에는 전체 필드를 비교하거나 아예 false를 반환할 수도 있겠죠? 그래서 아예 롬복 대신 직접 재정의를 하는 게 낫겠다 싶었어요. 동시성 학습이 목적이니 현황 유지해서 그냥 롬복을 쓰는 것도 괜찮다고 생각합니다, 그냥 제가 고민했던 부분을 공유하고 싶어서 코멘트 남겨 봐요.
+) 제안해 주신 onlyExplicitlyIncluded = true
옵션을 적용했을 때 어떤 장점이 있나요? 저는 모든 필드를 일일이 확인하면서 해당 필드가 동등성 비교에 영향을 미치는지 확인해 줘야 하는 불편함이 있다고 느꼈습니다 🥲 물론 확장성 측면에서는 훨씬 좋다고 생각해요! 다만 엔티티의 경우, 요구 사항 변경이 계속 일어나도 id라는 필드만으로 비교한다는 점은 변하지 않을 것 같아서 이에 대한 망쵸의 생각이 궁금합니다람쥐
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.
바보 감자 눈동자에 건배 🫠🫠
import com.aengdulab.ticket.domain.Ticket; | ||
import org.springframework.data.jpa.repository.JpaRepository; | ||
|
||
public interface MemberTicketRepository extends JpaRepository<MemberTicket, Long> { |
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.
JpaRepository를 상속하는 인터페이스라 어노테이션을 붙이지 않아도 스프링에서 자동으로 빈 등록이 돼서 크게 차이는 없는데, 저는 어노테이션으로 명시적 의미 부여를 하는 쪽을 조금 더 선호합니다. 일단 통일성을 위해 다 추가해 놓았는데 나중에 같이 얘기해 보면 좋을 것 같아요
|
||
@Entity | ||
@Getter | ||
@EqualsAndHashCode(of = "id") |
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.
이런 어노테이션도 있군요, 하나 배워갑니다 👍
망쵸 코멘트 보고 엔티티의 equals, hashCode 재정의에 대해 생각해 봤습니다. 그런데 롬복을 통한 재정의에서 조금 걸리는 부분이 있더라구요.
public class EqualsTest {
@Test
void id가_없는_경우_동등하지_않다고_판단한다() {
Member member = new Member("member");
Ticket ticket = new Ticket("ticket", 10L);
MemberTicket firstMemberTicket = new MemberTicket(member, ticket);
MemberTicket secondMemberTicket = new MemberTicket(member, ticket);
assertThat(firstMemberTicket.equals(secondMemberTicket)).isFalse();
}
@Test
void id가_같은_경우_동등하다고_판단한다() {
MemberTicket firstMemberTicket = new MemberTicket(1L, new Member("member"), new Ticket("ticket", 10L));
MemberTicket secondMemberTicket = new MemberTicket(1L, new Member("member1"), new Ticket("ticket1", 1L));
assertThat(firstMemberTicket.equals(secondMemberTicket)).isTrue();
}
@Test
void id가_다른_경우_동등하지_않다고_판단한다() {
Member member = new Member("member");
Ticket ticket = new Ticket("ticket", 10L);
MemberTicket firstMemberTicket = new MemberTicket(1L, member, ticket);
MemberTicket secondMemberTicket = new MemberTicket(2L, member, ticket);
assertThat(firstMemberTicket.equals(secondMemberTicket)).isFalse();
}
}
위 테스트를 실행하면 다음과 같은 결과가 나옵니다.
즉, id가 모두 null인 경우는 true를 반환하는 거죠.
개인적으로 entity는 영속화가 되어 있는 상태에서는 id 필드만을 기준으로 비교하고, 영속화되지 않은 상태에서는 name이나 email 같은 비즈니스 키를 사용하는 방식으로 비교하는 것이 좋다고 생각해요. 비즈니스 키가 없는 경우에는 전체 필드를 비교하거나 아예 false를 반환할 수도 있겠죠? 그래서 아예 롬복 대신 직접 재정의를 하는 게 낫겠다 싶었어요. 동시성 학습이 목적이니 현황 유지해서 그냥 롬복을 쓰는 것도 괜찮다고 생각합니다, 그냥 제가 고민했던 부분을 공유하고 싶어서 코멘트 남겨 봐요.
+) 제안해 주신 onlyExplicitlyIncluded = true
옵션을 적용했을 때 어떤 장점이 있나요? 저는 모든 필드를 일일이 확인하면서 해당 필드가 동등성 비교에 영향을 미치는지 확인해 줘야 하는 불편함이 있다고 느꼈습니다 🥲 물론 확장성 측면에서는 훨씬 좋다고 생각해요! 다만 엔티티의 경우, 요구 사항 변경이 계속 일어나도 id라는 필드만으로 비교한다는 점은 변하지 않을 것 같아서 이에 대한 망쵸의 생각이 궁금합니다람쥐
고민한 내용 공유해 주셔서 감사해요 😀 고려하지 않던 내용인데 덕분에 새로 알게 되었어요 👍 이런 활동이 계속되면 좋겠어요. 저도 노력해 볼게요!
엔티티 객체이기에 id 필드만을 동등성 비교에 사용한다와 id 필드명은 변경하지 않는다는 사실이 쉽게 변하진 않을 것 같아요. 그렇다면 그런데 만약에 동등성 비교 정책이 변경될 경우를 생각해 보면, onlyExplicitlyIncluded 옵션을 사용했다면 모든 필드를 확인해야 될거에요. 이때 휴먼 에러가 발생할 가능성이 있다고 생각해요. 결국 컨벤션 영역이라 생각 들고, 이런 경우에 공수가 가장 적게 드는 방향을 좋아하는데요. 현재 작성한 대로 유지하는 것이 좋아 보여요 :) |
No description provided.