Step3 수강신청(DB 적용) #320

jikimee64 · 2023-12-05T15:51:28Z

용주님 안녕하세요.!!
수강신청(DB 적용) 리뷰 요청 드립니다.

이번 단계는 전 단계들보다 많이 헤맸네요..

아래는 질문 사항 입니다.!!

수강생 '수'만 관리하고 있어서 중복 신청 방지 기능은 도메인에서 해결하지 못할 것 같은데요
최대 인원, 모집 상태 처럼 중복 신청도 도메인에서 방지하면 어떨까요?

=> 위는 2차 리뷰 피드백 주신건데요!! 유저 중복 신청을 막기 위해 Session내 SessionUsers를 만들었습니다. 해당 구조가 올바른지 확신이 들진 않네요..

repository 테스트 코드 검증시 조회한 도메인 모델의 데이터를 모두 getter로 꺼내서 검증 해야 할까요?
위와 같은 필요성을 느끼질 못해서 id 혹은 테스트와 관련된 데이터만 검증했습니다..!
조회한 SessionUsers를 Session 생성과 같은 생명 주기로 생성하고 싶었습니다만.. 아래와 같은 코드로 구현을 하긴 했는데 도저히 아닌 것 같지만 올바른 방법이 떠오르질 않네요 ㅠㅠ JPA를 사용시 일급 컬렉션을 많이 쓰던데 그건 어떻게 가져오는건지 참..

    @Transactional
    public void register(long sessionId, NsUser user, Payment payment) {
        SessionUsers sessionUsers = sessionUsersRepository.findBy(sessionId);
        Session session = sessionRepository.findBy(sessionId, sessionUsers)

이번 단계는 피드백 20개 정도 받을 것 같네요 ㅎ_ㅎ 🤑

이번 Step에서도 많이 배우겠습니다.
감사합니다. :)

- SessionState 메시지 전달 - SessionPeriod 시작 및 종료날짜 검증 - EnumSource 활용 - Session 생성 메소드 추출 - isSameBy 테스트 케이스 추가

- 유저가 강의를 중복 신청시 예외처리

testrace

안녕하세요 우철님
요구사항에 맞게 잘 구현해 주셔서 예상하신 것보다 코멘트가 적네요 😃
다음 단계 전에 코멘트 확인 부탁드려요
마지막 단계 화이팅입니다 🔥

testrace · 2023-12-06T03:00:18Z

README.md

+```mermaid
+---
+title: lms
+---
+erDiagram


diagram 👍

testrace · 2023-12-06T03:01:26Z

src/main/java/nextstep/courses/domain/Session.java

+    public Session(Long id) {
+        this(id, null, null, null, null, null, null, null, null);
+    }


테스트에서만 사용되는 생성자네요 😃
null 초기화보단 기본 값을 할당해줘도 좋을 것 같아요

testrace · 2023-12-06T03:02:35Z

src/main/java/nextstep/courses/domain/SessionPeriod.java

+    private static void validateDateRange(LocalDate startDate, LocalDate endDate) {
+        if (startDate.isAfter(endDate)) {
+            throw new SessionPeriodException("시작일은 종료일 보다 작아야 합니다.");
+        }
+    }


피드백 반영👍

Suggested change

private static void validateDateRange(LocalDate startDate, LocalDate endDate) {

if (startDate.isAfter(endDate)) {

throw new SessionPeriodException("시작일은 종료일 보다 작아야 합니다.");

}

}

private void validateDateRange(LocalDate startDate, LocalDate endDate) {

if (startDate.isAfter(endDate)) {

throw new SessionPeriodException("시작일은 종료일 보다 작아야 합니다.");

}

}

testrace · 2023-12-06T03:28:39Z

src/main/java/nextstep/courses/domain/Session.java

+    public void register(NsUser user, Payment payment) {
        validateState();
        validateType();
        validatePriceEqualPayment(payment);
+        sessionUsers.addUser(user);
        sessionUserCount.plusUserCount();
    }


유저 중복 신청을 막기 위해 Session내 SessionUsers를 만들었습니다. 해당 구조가 올바른지 확신이 들진 않네요..

잘 구현하신 것 같아요 👍
다만 지금처럼 기능이 추가되거나 변경되면 필드도 추가되고 메서드 내부도 계속 늘어날 것 같은데요
수강신청 객체를 도출해서 관련 책임들을 모두 위임하는 구조도 고려해 보시면 좋을 것 같아요

Suggested change

public void register(NsUser user, Payment payment) {

validateState();

validateType();

validatePriceEqualPayment(payment);

sessionUsers.addUser(user);

sessionUserCount.plusUserCount();

}

public void register(NsUser user, Payment payment) {

enrollment.enroll(user, payment);

}

확실히 위임하니 더 좋네요!! 감사합니다~

testrace · 2023-12-06T03:45:58Z

src/main/resources/schema.sql

+    price           int         not null,
+    state           varchar(10) not null,
+    type            varchar(10) not null,
+    user_count      int default 0,


user_session 테이블의 데이터 건수를 참조해도 될 것 같은데요
어떤 부분에서 user_count 컬럼이 필요하다고 판단하셨는지 궁금합니다

😱 헉 그 생각을 못했네요..
불필요한 필드여서 지우는게 좋겠네요. 감사합니다!

testrace · 2023-12-06T04:36:36Z

src/main/java/nextstep/courses/service/SessionService.java

+        SessionUsers sessionUsers = sessionUsersRepository.findBy(sessionId);
+        Session session = sessionRepository.findBy(sessionId, sessionUsers)
+                .orElseThrow(() -> new SessionException("강의 정보를 찾을 수 없습니다."));
+        session.register(user, payment);


조회한 SessionUsers를 Session 생성과 같은 생명 주기로 생성하고 싶었습니다

session 조회 시 sessionUsers를 모두 포함하는 형태를 원하셨다면
sessionRepository 내부에서 sessionUserRepository를 활용해 보셔도 될 것 같아요

public class JdbcSessionRepository implements SessionRepository { private final SessionUserRepository sessionUserRepository; ... public Optional<Session> findBy(long sessionId) { final SessionUsers sessionUsers = sessionUsersRepository.findBy(sessionId); ... } }

테스트코드에서 SessionUsersRepository를 주입 못받아서 스킵했었는데
@import({JdbcSessionUsersRepository.class}) 방법이 있었네요..!
감사합니다~

testrace · 2023-12-06T04:39:37Z

src/main/resources/schema.sql

+    primary key (id)
+);
+
+create table user_session


코드에서는 sessionUser 라는 이름으로 사용되고 있으니 테이블명도 같은 형식이면 좋을 것 같아요
작성하신 erd에도 session_user 라고 되어 있어요

Suggested change

primary key (id)

);

create table user_session

primary key (id)

);

create table session_user

h2에서 SESSION_USER 이게 예약어 더라구요 ㅠㅠ
session_users로 테이블명 수정하겠습니다~!

testrace · 2023-12-06T04:47:54Z

src/test/java/nextstep/courses/domain/SessionTest.java

+    @Test
+    @DisplayName("실패 - 유저가 기존에 신청한 강의일 경우 중복으로 수강 신청을 할 수 없다.")
+    void fail_session_register_user() {
+        Session session = zeroAndOneThousandSession(SessionState.OPEN, SessionType.PAID);
+        assertThatThrownBy(() -> duplicateAddSessionRegister(session))
+                .isInstanceOf(SessionUserException.class)
+                .hasMessage("강의를 이미 신청한 유저이므로 중복으로 신청할 수 없습니다.");
+    }


개행을 활용해 단락을 구분하면 메서드 분리 없이도 가독성을 챙길 수 있을 것 같은데 어떻게 생각하시는지 궁금합니다

Suggested change

@Test

@DisplayName("실패 - 유저가 기존에 신청한 강의일 경우 중복으로 수강 신청을 할 수 없다.")

void fail_session_register_user() {

Session session = zeroAndOneThousandSession(SessionState.OPEN, SessionType.PAID);

assertThatThrownBy(() -> duplicateAddSessionRegister(session))

.isInstanceOf(SessionUserException.class)

.hasMessage("강의를 이미 신청한 유저이므로 중복으로 신청할 수 없습니다.");

}

@Test

@DisplayName("실패 - 유저가 기존에 신청한 강의일 경우 중복으로 수강 신청을 할 수 없다.")

void fail_session_register_user() {

Session session = zeroAndOneThousandSession(SessionState.OPEN, SessionType.PAID);

final Payment payment = paymentOneThousand();

session.register(JAVAJIGI, payment);

assertThatThrownBy(() -> session.register(JAVAJIGI, payment))

.isInstanceOf(SessionUserException.class)

.hasMessage("강의를 이미 신청한 유저이므로 중복으로 신청할 수 없습니다.");

}

좋은 것 같습니다!! 감사합니다 ㅎㅎ

testrace · 2023-12-06T04:58:37Z

src/test/java/nextstep/courses/infrastructure/JdbcSessionUsersRepositoryTest.java

+        assertThat(users).hasSize(2)
+                .extracting("id")
+                .containsExactly(1L, 2L);


repository 테스트 코드 검증시 조회한 도메인 모델의 데이터를 모두 getter로 꺼내서 검증 해야 할까요?
위와 같은 필요성을 느끼질 못해서 id 혹은 테스트와 관련된 데이터만 검증했습니다..!

지금처럼 필요한 값 위주로 검증해 주셔도 될 것 같아요 😃

testrace · 2023-12-06T06:39:24Z

src/main/java/nextstep/courses/domain/SessionRepository.java

+import java.util.Optional;
+
+public interface SessionRepository {
+    Optional<Session> findBy(long sessionId, SessionUsers sessionUsers);


Optional 👍

jikimee64 added 6 commits December 4, 2023 22:07

refactor: step2 피드백 반영

f42a742

- SessionState 메시지 전달 - SessionPeriod 시작 및 종료날짜 검증 - EnumSource 활용 - Session 생성 메소드 추출 - isSameBy 테스트 케이스 추가

refactor: step2 피드백 반영

2b60b40

- 유저가 강의를 중복 신청시 예외처리

docs: step2 피드백 관련된 기능 요구 사항 추가

1965be7

docs: erd 및 스키마 생성

38d912a

feat: JdbcRepository 구현

b37feb1

feat: JdbcRepository 및 SessionService 구현

e3a8e74

testrace approved these changes Dec 6, 2023

View reviewed changes

testrace merged commit 0767695 into next-step:jikimee64 Dec 6, 2023

jikimee64 mentioned this pull request Dec 8, 2023

Step4 수강신청(요구사항 변경) #344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Step3 수강신청(DB 적용) #320

Step3 수강신청(DB 적용) #320

jikimee64 commented Dec 5, 2023

testrace left a comment

testrace Dec 6, 2023

testrace Dec 6, 2023

testrace Dec 6, 2023

testrace Dec 6, 2023

jikimee64 Dec 7, 2023

testrace Dec 6, 2023

jikimee64 Dec 6, 2023

testrace Dec 6, 2023

jikimee64 Dec 6, 2023 •

edited

testrace Dec 6, 2023

jikimee64 Dec 6, 2023

testrace Dec 6, 2023

jikimee64 Dec 6, 2023

testrace Dec 6, 2023

testrace Dec 6, 2023

Step3 수강신청(DB 적용) #320

Step3 수강신청(DB 적용) #320

Conversation

jikimee64 commented Dec 5, 2023

testrace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jikimee64 Dec 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jikimee64 Dec 6, 2023 •

edited