🚀 2단계 - 수강신청(도메인 모델) #627

mingulee-devel · 2024-11-03T07:51:49Z

안녕하세요!
지난번에 피드백 주셨던 내용과 함께 2단계 PR 드립니다
진영님이 말씀해주신 대로 사다리 게임이 끝나고 PR드릴까 하다가 리뷰 기다리는 동안 올려봅니다!

요구사항에 적힌대로 한다고 했는데 제가 객체를 잘 나눈건지 모르겠습니다
스스로 해보고 싶어서 최대한 다른 분들의 코드를 확인하지 않고 했는데
진전이 없어서 PR 드립니다!

혹시 보시고 힌트를 주실 수 있을까요?!

코드에서 todo를 달아놓았는데요
SessionStrategy를 상속받는 FreeSession이랑 PaidSession 나누어서 PaidSession에 canEnroll이 true일 때 payment를 더해주려고 했는데 뭔가 이 부분이 어색하게 느껴집니다... 😢

오늘도 잘 부탁 드립니다 😄

1단계 피드백 반영

불필요 setDeleted 사용 제거
DeleteHistory 생성자가 아닌 정적 팩토리 메서드로 생성하도록 수정
QuestionBody, Deleted, Answers 분리

jinyoungchoi95

안녕하세요 민선님 😄

몇가지 코멘트 남겨두었는데 확인부탁드려요 :)

궁금하거나 고민이 되는 부분이 있으시다면 언제든 pr 코멘트 또는 dm으로 요청 부탁드립니다.
감사합니다 🙇‍♂️

jinyoungchoi95 · 2024-11-04T01:27:53Z

src/main/java/nextstep/courses/domain/session/FreeSession.java

+
+import nextstep.payments.domain.Payment;
+
+public class FreeSession implements SessionStrategy {


Session이라는 도메인이 이미 있고 현재 구현체는 Session에 따른 결제 전략이기 때문에 네이밍이 FreeSessionEnrollStrategy와 같이 조금 더 명확하면 좋을 것 같아요 :)

jinyoungchoi95 · 2024-11-04T01:38:32Z

src/main/java/nextstep/courses/domain/session/PaidSession.java

+    public void enroll(Payment payment){ //todo
+        if(canEnroll(payment)){
+            payments.add(payment);
+        }
+    }
+
+    @Override
+    public boolean canEnroll(Payment payment) {
+        return !isFull() && payment.isTuitionPaid(tuitionFee);
+    }


구조가 어색해보이는 이유는 PaidSessionEnrollStrategy를 어떻게 활용할건지 명확하지 않아서인 것 같아요. 지금 설계한 객체의 역할로보면,

강의가 결제가능한 상황인지 확인해준다.

이를 위해 현재 수강생 수, 최대 수강생 수, 강의료를 가진다.

결제를 받아 수강등록한다.

라고 바라본다면 SessionEnrollStrategy는 "수강 등록"이라는 명제만 동작해주면되고, canEnroll이 되지 않는다면 예외를 터트리는 등과 같은 행위로 생각해볼 수 있을 것 같아요.

public interface SessionEnrollStrategy { void enroll(Payment payment); }

가 되어야하고,

public class PaidSessionEnrollStrategy { // .. public void enroll(Payment payment) { // canEnroll이 아닌 경우 예외 // count를 올리고 결제정보 저장

이 될 수 있겠죠. 다만 해당 클래스도 전략으로 불리고 있기 때문에 혼동을 줄 수 있는데요. "전략"이라는 것은 어떤 행위를 어떻게 실행하느냐가 주요 관심사이지 상태를 들고 관리하면서 전략을 추구하는건 전략자체의 책임을 확장시켜버릴 수 있어요. 추상화를 통한 접근은 맞지만 전략이냐는 고민해보시면 좋을 것 같네요 :)

jinyoungchoi95 · 2024-11-04T01:40:01Z

src/main/java/nextstep/courses/domain/session/Sessions.java

+public class Sessions {
+    private List<Session> values;
+}


개인적으로 단순히 래핑만하는 클래스는 일급컬랙션으로 묶을 필요는 없다고 생각합니다. 특정 도메인 책임 없이 리스트이기 때문에 일급컬랙션으로 둔다는 것은 과한 래핑이 될 수 있어요.

jinyoungchoi95 · 2024-11-04T01:41:10Z

src/main/java/nextstep/courses/domain/session/coverImage/ImageDimensions.java

+    public ImageDimensions(int width, int height) {
+        this.width = width;
+        this.height = height;
+    }
+
+    public boolean validDimensions() {


생성자에서 validation을 하지 않으시는 이유가 있을까요? 객체의 생성은 객체가 담당하는 것이 객체 생성의 안정성을 보장해요 :)

jinyoungchoi95 · 2024-11-04T01:44:33Z

src/main/java/nextstep/courses/domain/session/coverImage/ImageExtension.java

+    BMP;
+
+    public static boolean validExtension(ImageExtension extension) {
+        if (!Arrays.asList(GIF, JPG, JPEG, PNG, SVG).contains(extension)) {


특정 확장자만 관리하고싶다면 상수로 빼면 좋을 것 같네요 :)

jinyoungchoi95 · 2024-11-04T01:44:55Z

src/main/java/nextstep/courses/domain/session/coverImage/ImageSize.java

+package nextstep.courses.domain.session.coverImage;
+
+public class ImageSize {
+    private int size;


불변일 수 있는 필드들은 모두 불변으로 수정해주시면 좋을 것 같아요 :)

jinyoungchoi95 · 2024-11-04T01:47:31Z

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

+
+import java.time.LocalDate;
+
+public class Session {


SessionService를 통해 enroll 로직을 구현하는것까지가 미션의 요구사항이에요 :)

jinyoungchoi95 · 2024-11-04T01:47:55Z

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

+    private SessionStrategy sessionStrategy;
+
+    public Session(SessionCoverImage sessionCoverImage, SessionStatus sessionStatus, SessionStrategy sessionStrategy) {
+        this(null, null, sessionCoverImage, sessionStatus, sessionStrategy);


둘다 null인 케이스를 생성자에서 지원하는 이유가 있을까요?

jinyoungchoi95 · 2024-11-04T01:49:31Z

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

+    private FreeSession freeSession;
+
+    @Test
+    void 수강신청_가능여부_확인() {
+        freeSession = new FreeSession();


필드들을 불변하게 가져가주시면 좋을 것 같아요 :) 가변적인 필드는 가변적일 수 있다라는 컨텍스트를 계속 안고 코드를 읽게 만들기 때문에 가능하면 지양해주시는 것이 좋습니다.

jinyoungchoi95 · 2024-11-04T01:51:37Z

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

+    @Test
+    void 수강신청_가능여부_확인__강의_수강료_확인() {
+        paidSession = new PaidSession(3, 2, 20000);
+        assertTrue(paidSession.canEnroll(new Payment(20000L)));
+        paidSession = new PaidSession(3, 2, 20000);
+        assertFalse(paidSession.canEnroll(new Payment(15000L)));
+    }
+
+    @Test
+    void 수강신청_가능여부_확인__인원초과_확인() {
+
+        paidSession = new PaidSession(3, 2, 20000);
+        assertTrue(paidSession.canEnroll(new Payment(20000L)));
+        paidSession = new PaidSession(2, 2, 20000);
+        assertFalse(paidSession.canEnroll(new Payment(20000L)));
+    }


전반적으로 테스트에서 어떤것을 테스트하고자하는지가 명확하지 않은 것 같아요. 테스트당 검증하고자하는 대상은 명확하여야하고, 이 기준이 어렵다고 느끼신다면 assertion이 하나여야된다고 생각해보시면 편할 것 같아요.

예를들면 강의 수강료확인에서 검증하고자하는 것은 canEnroll이 true이다인데 false인걸 테스트할 필요가 있을까요? 또 인원초과 확인인데 canEnroll이 true이다인걸 테스트할 필요가 있을까요?

mingulee-devel added 22 commits October 30, 2024 21:39

refactor: 1단계 피드백 반영 - 불필요 setDeleted 사용 제거

f89ccf9

refactor: 1단계 피드백 반영 - DeleteHistory 생성자가 아닌 정적 팩토리 메서드로 생성하도록 수정

372331e

chore: 불필요 코드 삭제

8928f60

docs: 불필요 TODO 삭제

d965af3

refactor: 1단계 피드백 반영 - Answers 일급컬렉션 분리, 테스트 코드 수정

a22f18e

refactor: QuestionBody 분리

e9340bd

refactor: Deleted 분리

1ec08ae

docs: 2단계 - 수강신청 TODO 작성

f320b7b

feat: 2단계 - 수강신청 강의 상태 도메인 구현

e379e60

test: 2단계 - 수강신청 강의 도메인 테스트 코드 작성

8311e79

test: 2단계 - 수강신청 강의 커버 이미지 도메인 테스트 코드 작성

c3837de

feat: 2단계 - 수강신청 강의 커버 이미지 도메인 구현

6a0964e

feat: 2단계 - 수강신청 강의 테스트코드 및 도메인 구현

bcae477

feat: 2단계 - 수강신청 강의 유료강의 테스트 코드 추가

dcea385

feat: 2단계 - 수강신청 코스에 강의 리스트 추가

3423226

feat: 2단계 - Course 기수 추가

2a43351

feat: 2단계 - SessionStrategy 수정, PaidSession List<Payment> 추가

82675be

feat: 2단계 - Payments 일급컬렉션 분리

f787e17

feat: 2단계 - Sessions 일급컬렉션 분리

26e958f

feat: 2단계 - SessionDate 분리

cf83180

feat: 2단계 - session 패키지 추가, Payments payments 패키지로 파일 이동

6355517

feat: 2단계 - ImageExtension(Enum), ImageSize, ImageDimensions 분리

786f774

jinyoungchoi95 requested changes Nov 4, 2024

View reviewed changes

refeactor: 2단계 피드백 반영 - 유효한 확장자 상수로 분리

950b4a8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚀 2단계 - 수강신청(도메인 모델) #627

🚀 2단계 - 수강신청(도메인 모델) #627

mingulee-devel commented Nov 3, 2024

jinyoungchoi95 left a comment

jinyoungchoi95 Nov 4, 2024

jinyoungchoi95 Nov 4, 2024

jinyoungchoi95 Nov 4, 2024

jinyoungchoi95 Nov 4, 2024

jinyoungchoi95 Nov 4, 2024

jinyoungchoi95 Nov 4, 2024

jinyoungchoi95 Nov 4, 2024

jinyoungchoi95 Nov 4, 2024

jinyoungchoi95 Nov 4, 2024

jinyoungchoi95 Nov 4, 2024


		import nextstep.payments.domain.Payment;

		public class FreeSession implements SessionStrategy {

🚀 2단계 - 수강신청(도메인 모델) #627

Are you sure you want to change the base?

🚀 2단계 - 수강신청(도메인 모델) #627

Conversation

mingulee-devel commented Nov 3, 2024

1단계 피드백 반영

jinyoungchoi95 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment