🚀 4단계 - 수강신청(요구사항 변경) #387

hynxp · 2023-12-13T14:21:01Z

안녕하세요 리뷰어님.
아직 수강 승인에 대한 구현이 되지 않았지만, 마지막 날이기도 하고 제가 전체적으로 4단계 이해를 못하고 있는 것 같아서..
PR보내봅니다!
오늘도 질문이 많네요 😢

질문

강의가 진행 중인 상태에서도 수강신청이 가능해야 한다 이 부분이 이해가 안가는데요..
기존에는 수강 신청 시 강의가 모집중일 때만 가능했잖아요? 저는 현재 날짜가 기간 내일때만 모집중으로 바꿀 수 있었어요.
(기존에도 진행 중일 때 수강신청이 가능했다는 말이에요!)
근데 저 부분이 추가되면서 "상태가 모집중이 아니어도 진행중이기만 하면 신청이 가능하게 하라는건가?" 생각이 들어서.. 일단 그렇게 구현은 했습니다만 좀 이상합니다 ㅠㅠ ..
전체적으로 뭘 요구하는지 모르겠어요.
session 생성자들의 파라미터가 너무 많고 지저분한 느낌입니다.
특히 new Status(LocalDate.now(), startDate, endDate); 이 부분이 중복되고 있는데 처음에는 Status 내부에서 startDate, endDate 2개만 가지고 값을 지정하려고 했는데, LocalDate.now()를 주입받지 않으면 멱등성이 깨진다고 해서요..

private Session(Long id, Long courseId, SessionType type, CoverImages coverImages, Period period, int maxStudents, Long fee, LocalDateTime createdAt, LocalDateTime updatedAt) {
      super(createdAt, updatedAt);
      validateNotNull(id, coverImages, period);
      this.id = id;
      this.courseId = courseId;
      this.type = type;
      this.coverImages = coverImages;
      this.period = period;
      this.status = new Status(period); //이렇게나
      this.status = new Status(LocalDate.now(), startDate, endDate) //이렇게
      this.students = new Students(); 
      this.paidCondition = new PaidCondition(maxStudents, fee);
}

Status를 파라미터로 받지않고 생성자 내에서 인스턴스 생성하는 방법은 별로인가요?

SessionStatus.of 메서드에서 Period를 받아서 메시지를 보내는게 더 좋을까요? (결합도가 강해질까봐 데이터를 나눠서 보냈어요..)

함께 완주는 못해서 아쉽지만 정말 많이 배웠습니다. 감사했습니다!

seokhong · 2023-12-14T01:23:56Z

경현님, 질문 남겨주신 내용에 대한 답변부터 코멘트로 남깁니다. :)

강의가 진행 중인 상태에서도 수강신청이 가능해야 한다 이 부분이 이해가 안가는데요..
기존에는 수강 신청 시 강의가 모집중일 때만 가능했잖아요? 저는 현재 날짜가 기간 내일때만 모집중으로 바꿀 수 있었어요.
(기존에도 진행 중일 때 수강신청이 가능했다는 말이에요!)
근데 저 부분이 추가되면서 "상태가 모집중이 아니어도 진행중이기만 하면 신청이 가능하게 하라는건가?" 생각이 들어서.. 일단 그렇게 구현은 했습니다만 좀 이상합니다 ㅠㅠ ..
전체적으로 뭘 요구하는지 모르겠어요.

기존에는 상태값이 "준비중, 모집중, 종료" 상태를 가질 수 있었지만, 이제는 "준비중, 진행중, 종료" 상태를 가질 수 있고, "모집중" 상태는 모집 상태로 따로 관리되어야 합니다. 설명을 위해 기존 데이터를 v1(모집중 상태를 가질 수 있음), 새로운 데이터를 v2라고 하겠습니다. 이렇게 변경되면 수강 신청을 할 때, v1 강의는 "상태값"이 모집중일 때 수강신청이 가능합니다. 한편, v2의 경우에는 "모집 상태"이 모집중일 때 수강신청이 가능합니다. v1 데이터도 DB에 데이터가 존재하기 때문에 "상태값"을 통해 수강신청이 가능한지 확인할 수 있어야 하고, v2 데이터도 만들어질 수 있으므로 "모집 상태"를 통해 수강신청이 가능해야 합니다.
v1, v2 두 종류의 데이터가 모두 잘 동작하도록 하면서도, 외부에서는 강의를 그대로 사용할 수 있도록 구현해보시면 좋겠습니다.

session 생성자들의 파라미터가 너무 많고 지저분한 느낌입니다.
특히 new Status(LocalDate.now(), startDate, endDate); 이 부분이 중복되고 있는데 처음에는 Status 내부에서 startDate, endDate 2개만 가지고 값을 지정하려고 했는데, LocalDate.now()를 주입받지 않으면 멱등성이 깨진다고 해서요..
Status를 파라미터로 받지않고 생성자 내에서 인스턴스 생성하는 방법은 별로인가요?

생성을 요청한 일시를 기준으로 판단하는 방법도 가능하고, 정책으로 문서화하면 괜찮다고 생각합니다. 다만, 이 경우에는 사용자가 경계 시간(e.g., 강의 시작 전날 11:59:59.998 에 요청하면 서버에서는 바로 모집중/진행중 상태로 생성될 수 있음)을 주의하도록 관리가 필요한데, 이는 정책이 복잡해질 우려가 있다고 생각합니다.
최초 강의를 생성하는 경우에는 "준비중"일 것이므로, 강의 생성 API에서는 상태를 요청받지 않고 항상 "준비중"으로 생성하도록 구현해보는 건 어떨까요?
생성자의 파라미터가 많아서 복잡한 문제는 Session.newSession과 같은 정적 생성자를 만들고, 강의를 신규로 생성할 때 기본 값은 정적 생성자 내에서 정하는 방법이 있을 것 같습니다. DB에서 조회한 데이터로 엔티티를 만들기 위해 모든 인자를 받는 생성자는 필요할 것으로 보이며, 이를 줄이고 싶다면 빌더 패턴을 사용하시는 방법도 있습니다.

SessionStatus.of 메서드에서 Period를 받아서 메시지를 보내는게 더 좋을까요? (결합도가 강해질까봐 데이터를 나눠서 보냈어요..)

위에 말씀드린 코멘트로 답이 될 것 같은데요. 혹시 추가로 설명이 필요하다면 말씀 부탁드립니다.

seokhong

경현님, 4단계 미션 잘 구현하셨습니다. :)
질문에 대한 답변 내용에 맞춰 수정해야 할 부분이 있는 것으로 보입니다. 답변과 코멘트 확인 부탁드립니다. 🙂

seokhong · 2023-12-14T10:12:17Z

src/main/resources/schema.sql

@@ -51,6 +51,7 @@ create table delete_history (

 create table cover_image (
    id bigint not null,
+    session_id bigint not null,


메신저로 말씀드린 것처럼 기존 cover_image, session 테이블에 데이터가 있다는 가정하에 진행되는 미션이므로 create table DDL을 수정하시지 말고, alter table 을 통해 구조를 변경하는 방법으로 진행해보시면 좋겠습니다.

seokhong · 2023-12-14T10:13:18Z

src/main/resources/schema.sql

    type varchar(10) not null,
-    status varchar(10) not null,


cover_image에 남긴 코멘트와 질문에 대해 답변 남긴 코멘트를 참고하시어, alter table을 사용해서 status는 유지하고 recruitment_status만 추가하면서 기존 데이터와 새로운 데이터가 모두 잘 동작하도록 구현해보시면 좋겠습니다.

앗 마이그레이션 과정을 남기면 된다고 생각했습니다.
알겠습니다!

seokhong · 2023-12-14T10:18:17Z

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

-    public static Session ofFree(Long id, Long courseId, CoverImage coverImage, LocalDate startDate, LocalDate endDate) {
-        return new Session(id, courseId, SessionType.FREE, coverImage, new Period(startDate, endDate), Status.NOT_OPEN, 0, 0L, LocalDateTime.now(), null);
+    public static Session ofFree(Long id, Long courseId, CoverImages coverImages, LocalDate startDate, LocalDate endDate) {
+        return new Session(id, courseId, SessionType.FREE, coverImages, new Period(startDate, endDate), new Status(LocalDate.now(), startDate, endDate), 0, 0L, LocalDateTime.now(), null);


Status가 조회해온 데이터의 날짜를 기준으로 설정되는데, 임의로 상태가 변경되면 강사에게 혼란이 발생할 수 있을 것으로 보입니다. Status의 변경은 명시적으로 강사가 진행한다는 전제하에 구현해보시면 좋겠습니다.

현업에서는 날짜가 경과되면 배치 등을 이용해서 자동으로 Status가 변경되도록 구현하실 수 있을텐데, 이 경우 데이터를 수정하실 것 같아요. 객체를 만들 때 임의로 상태를 만드는 것은 코드를 이해하거나 유지보수하기 어렵게 만들 수 있으며, 디버깅 과정에서도 어려움이 발생할 수 있습니다.

seokhong · 2023-12-14T10:21:49Z

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

+    }
+
+    public void validate() {
+        if (!sessionStatus.isInProgress() && !recruitmentStatus.isRecruiting()) {


sessionStatus만 존재했던 세션이라면(recruitmentStatus가 없거나, sessionStatus가 모집중인 경우) sessionStatus만을 이용하여 수강 신청이 가능한지 확인할 수 있을 것으로 보입니다. recruitmentStatus가 존재한다면, 이 값을 기준으로 수강 신청이 가능한지 확인할 수 있을 것으로 보입니다.
참고하시어 기존 데이터(sessionStatus만 있는 경우)와 새로운 데이터(recruitmentStatus가 있는 경우) 모두 동작하도록, 이 로직을 수정해보시면 좋겠습니다. :)

seokhong · 2023-12-14T10:23:29Z

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

+        this.recruitmentStatus = recruitmentStatus;
+    }
+
+    public void validate() {


Status 입장에서 이 메서드가 제공해야 하는 기능은 수강 신청이 가능한지 여부를 반환하는 것으로 보입니다.
Status는 수강 신청이 가능한 상태인지 여부만 반환하고, Session은 이를 참고하여 수강 신청이 불가할 경우 예외를 발생시키도록 해보면 어떨까요?

seokhong · 2023-12-14T10:36:16Z

src/main/java/nextstep/courses/repository/CoverImageRepository.java

@@ -2,8 +2,12 @@

 import nextstep.courses.domain.CoverImage;

+import java.util.List;
+
 public interface CoverImageRepository {


코드를 보면 -Repository postfix를 사용하셨는데, Repository 와 DAO의 차이는 무엇일까요? 관련해서 블로그 글을 참고해보셔도 도움이 될 것 같습니다. 현재의 구현을 보면, (CoverImageRepository의 구현체로 Jdbc를 사용한다는 것을 고려하더라도) Session이라는 도메인에 대한 Repository 보다는 매핑된 테이블을 추상화한 DAO에 좀 더 가까운 형태의 설계인 것처럼 보이기도 합니다. :)

CoverImage는 항상 Session과 함께 관리되어야 하지 않을까요? 그렇다면 Session을 저장할 때 함께 저장하고, 조회할 때도 함께 조회하는 것은 어떨까요?

seokhong · 2023-12-14T10:37:23Z

src/main/resources/data.sql

-
-INSERT INTO session (id, course_id, image_id, type, status, start_date, end_date, max_students, fee, created_at) VALUES (2, 2, 2, 'FREE', 'NOT_OPEN', '2023-12-01', '2023-12-31', 0, 0, CURRENT_TIMESTAMP());
-INSERT INTO session (id, course_id, image_id, type, status, start_date, end_date, max_students, fee, created_at) VALUES (3, 2, 2, 'PAID', 'NOT_OPEN', '2023-12-01', '2023-12-31', 30, 20000, CURRENT_TIMESTAMP());
+INSERT INTO cover_image (id, session_id, size, extension, width, height, created_at) VALUES (2, 3, 1, 'jpg', 300, 200, CURRENT_TIMESTAMP());


테스트를 위한 데이터도 기존 구조에 맞춰진 데이터(원래 있던 데이터), 변경한 테이블에 맞춰진 데이터를 모두 추가해서 동작을 확인해보면 어떨까요?

hynxp added 8 commits December 12, 2023 22:32

refactor: 수강신청 저장 시 아이디 반환하도록 수정, 테스트코드 수정

cd795cf

docs: 기능 요구사항 추가

8396e29

feat: 강의가 하나 이상의 커버 이미지를 가질 수 있게 수정

d8942f1

feat: 강의가 하나 이상의 커버 이미지를 가질 수 있게 수정 (as-is -> to-be)

3bc847f

remove: 사용하지않는 클래스 삭제

5750dc2

remove: 강의 상태가 진행중일때도 수강신청 가능하게 수정

8e77d9d

remove: 강의 상태가 진행중일때도 수강신청 가능하게 수정(as-is -> to-be)

e7af105

docs: 기능 요구사항 수정

0c96d79

seokhong requested changes Dec 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚀 4단계 - 수강신청(요구사항 변경) #387

🚀 4단계 - 수강신청(요구사항 변경) #387

hynxp commented Dec 13, 2023 •

edited

Loading

seokhong commented Dec 14, 2023

seokhong left a comment

seokhong Dec 14, 2023

seokhong Dec 14, 2023

hynxp Dec 15, 2023

seokhong Dec 14, 2023

seokhong Dec 14, 2023

seokhong Dec 14, 2023

seokhong Dec 14, 2023

seokhong Dec 14, 2023

🚀 4단계 - 수강신청(요구사항 변경) #387

Are you sure you want to change the base?

🚀 4단계 - 수강신청(요구사항 변경) #387

Conversation

hynxp commented Dec 13, 2023 • edited Loading

질문

seokhong commented Dec 14, 2023

seokhong 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

hynxp commented Dec 13, 2023 •

edited

Loading