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

티켓 재고 관리 도메인 설계 #3

Merged
merged 5 commits into from
Oct 31, 2024
Merged

티켓 재고 관리 도메인 설계 #3

merged 5 commits into from
Oct 31, 2024

Conversation

khabh
Copy link
Collaborator

@khabh khabh commented Oct 29, 2024

No description provided.

@khabh khabh requested a review from 3Juhwan October 29, 2024 13:43
@khabh khabh linked an issue Oct 29, 2024 that may be closed by this pull request
Copy link
Owner

@3Juhwan 3Juhwan left a 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> {
Copy link
Owner

Choose a reason for hiding this comment

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

R: Repository 어노테이션이 존재하는 곳이 있고, 없는 곳이 있어요!
컨벤션을 맞추면 좋을 것 같은데, 감자 의견은 어떤 쪽을 선호하나요?

Copy link
Collaborator Author

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)) {
Copy link
Owner

@3Juhwan 3Juhwan Oct 31, 2024

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를 사용하는 게 더 안전해 보이네요 :)

@3Juhwan
Copy link
Owner

3Juhwan commented Oct 31, 2024

아 그리고, 테스트의 한글 메서드에 경고 뜨는 게 불편하면 클래스 수준에 @SuppressWarnings("NonAsciiCharacters") 같은 어노테이션을 붙여도 좋을 것 같아요. (전 안 불편합니다 😁)


@Entity
@Getter
@EqualsAndHashCode(of = "id")
Copy link
Owner

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;

}

Copy link
Collaborator Author

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();
    }
}

위 테스트를 실행하면 다음과 같은 결과가 나옵니다.

image

즉, id가 모두 null인 경우는 true를 반환하는 거죠.

개인적으로 entity는 영속화가 되어 있는 상태에서는 id 필드만을 기준으로 비교하고, 영속화되지 않은 상태에서는 name이나 email 같은 비즈니스 키를 사용하는 방식으로 비교하는 것이 좋다고 생각해요. 비즈니스 키가 없는 경우에는 전체 필드를 비교하거나 아예 false를 반환할 수도 있겠죠? 그래서 아예 롬복 대신 직접 재정의를 하는 게 낫겠다 싶었어요. 동시성 학습이 목적이니 현황 유지해서 그냥 롬복을 쓰는 것도 괜찮다고 생각합니다, 그냥 제가 고민했던 부분을 공유하고 싶어서 코멘트 남겨 봐요.

+) 제안해 주신 onlyExplicitlyIncluded = true 옵션을 적용했을 때 어떤 장점이 있나요? 저는 모든 필드를 일일이 확인하면서 해당 필드가 동등성 비교에 영향을 미치는지 확인해 줘야 하는 불편함이 있다고 느꼈습니다 🥲 물론 확장성 측면에서는 훨씬 좋다고 생각해요! 다만 엔티티의 경우, 요구 사항 변경이 계속 일어나도 id라는 필드만으로 비교한다는 점은 변하지 않을 것 같아서 이에 대한 망쵸의 생각이 궁금합니다람쥐

@khabh khabh requested a review from 3Juhwan October 31, 2024 14:47
@3Juhwan 3Juhwan merged commit b6d3708 into main Oct 31, 2024
@3Juhwan 3Juhwan deleted the ticket/domain branch October 31, 2024 15:01
Copy link
Collaborator Author

@khabh khabh left a 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> {
Copy link
Collaborator Author

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")
Copy link
Collaborator Author

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();
    }
}

위 테스트를 실행하면 다음과 같은 결과가 나옵니다.

image

즉, id가 모두 null인 경우는 true를 반환하는 거죠.

개인적으로 entity는 영속화가 되어 있는 상태에서는 id 필드만을 기준으로 비교하고, 영속화되지 않은 상태에서는 name이나 email 같은 비즈니스 키를 사용하는 방식으로 비교하는 것이 좋다고 생각해요. 비즈니스 키가 없는 경우에는 전체 필드를 비교하거나 아예 false를 반환할 수도 있겠죠? 그래서 아예 롬복 대신 직접 재정의를 하는 게 낫겠다 싶었어요. 동시성 학습이 목적이니 현황 유지해서 그냥 롬복을 쓰는 것도 괜찮다고 생각합니다, 그냥 제가 고민했던 부분을 공유하고 싶어서 코멘트 남겨 봐요.

+) 제안해 주신 onlyExplicitlyIncluded = true 옵션을 적용했을 때 어떤 장점이 있나요? 저는 모든 필드를 일일이 확인하면서 해당 필드가 동등성 비교에 영향을 미치는지 확인해 줘야 하는 불편함이 있다고 느꼈습니다 🥲 물론 확장성 측면에서는 훨씬 좋다고 생각해요! 다만 엔티티의 경우, 요구 사항 변경이 계속 일어나도 id라는 필드만으로 비교한다는 점은 변하지 않을 것 같아서 이에 대한 망쵸의 생각이 궁금합니다람쥐

@3Juhwan
Copy link
Owner

3Juhwan commented Nov 1, 2024

고민한 내용 공유해 주셔서 감사해요 😀 고려하지 않던 내용인데 덕분에 새로 알게 되었어요 👍 이런 활동이 계속되면 좋겠어요. 저도 노력해 볼게요!

onlyExplicitlyIncluded = true 옵션에 대해, 어제 코멘트를 달고 고민해 보았고 컨벤션 영역이라 생각했어요. 해당 옵션을 사용하면 확장성 측면에서 좋겠지만, 감자가 언급한 단점도 있어요. 또, 생각해보면 엔티티의 경우에 주로 id만을 동등성 비교에 사용할테니 해당 옵션을 사용해도, 하지 않아도, 결과는 비슷하다고 생각해요.

엔티티 객체이기에 id 필드만을 동등성 비교에 사용한다id 필드명은 변경하지 않는다는 사실이 쉽게 변하진 않을 것 같아요. 그렇다면 onlyExplicitlyIncluded 옵션을 사용하더라도 id 필드만을 동등성 비교에 사용한다는 것을 아니까 어떤 필드가 동등성 비교에 사용되는지 확인하지 않아도 돼요. 언급해 주신 단점이 모호해지는 지점이라 생각해요.

그런데 만약에 동등성 비교 정책이 변경될 경우를 생각해 보면, onlyExplicitlyIncluded 옵션을 사용했다면 모든 필드를 확인해야 될거에요. 이때 휴먼 에러가 발생할 가능성이 있다고 생각해요.

결국 컨벤션 영역이라 생각 들고, 이런 경우에 공수가 가장 적게 드는 방향을 좋아하는데요. 현재 작성한 대로 유지하는 것이 좋아 보여요 :)

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