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

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

Open
wants to merge 1 commit into
base: developer-shkim
Choose a base branch
from

Conversation

developer-shkim
Copy link

@developer-shkim developer-shkim commented Nov 8, 2024

설명

  • 아래 다이어그램과 같이 설계하여 구현했습니다. 리뷰 부탁드립니다!

domain

Copy link
Contributor

@javajigi javajigi left a comment

Choose a reason for hiding this comment

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

2단계 구현하느라 수고했어요. 👍
도메인 객체 설계부터 구현해보니 어떤 느낌인가요?
Session에 대한 Test Fixture 생성하는데 어려움이 있었을 것 같은데요.
이번 기회에 Test Fixture를 생성하는 수현님 만의 방법을 찾아보면 어떨까요?

@@ -13,6 +14,10 @@ public class Course {

private LocalDateTime updatedAt;

private List<Session> sessions;
Copy link
Contributor

Choose a reason for hiding this comment

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

Session을 추가하는 add와 같은 메서드도 있어야 하지 않을까?

import java.io.IOException;
import java.util.List;

public class CoverImage {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines +11 to +21
protected LocalDateTime startDate;

protected LocalDateTime endDate;

protected CoverImage coverImage;

protected Long price;

protected SessionStatus status;

protected int numberOfApplicants;
Copy link
Contributor

Choose a reason for hiding this comment

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

특별한 이유가 없으면 객체의 모든 인스턴스 변수는 private final로 구현해 보는 연습을 하면 어떨까?

this.numberOfApplicants = numberOfApplicants;
}

public Session(Long price, SessionStatus status) {
Copy link
Contributor

Choose a reason for hiding this comment

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

이 생성자가 필요한 이유는?
반드시 필요하다면 부생성자는 주생성자를 호출하는 방식으로 구현한다.

this.status = status;
}

public void register(Payment payment) {
Copy link
Contributor

Choose a reason for hiding this comment

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

메서드가 있는 이 메서드를 추상 메서드로 구현하는 것은 어떨까?

Comment on lines +25 to +26
session = new FreeSession(now, now, coverImage, status, 0);
sessionWithApplicant = new FreeSession(now, now, coverImage, status, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Session 객체와 같이 인스턴스 변수가 많은 객체를 테스트하려면 객체를 생성하는데 어려움이 있다.
중복 코드 또한 많이 발생해 Session을 생성할 때 생성자의 인자가 변경되는 경우 변경할 부분이 많아진다.
https://www.arhohuttunen.com/test-data-builders/ 문서 참고해 Session과 같이 복잡한 객체의 테스트 데이터를 생성할 때 어떤 방법을 사용할 것인지 선택해 보면 좋겠다.
이번 기회에 내가 선호하는 방법을 적용해 보고 앞으로도 쭈욱 활용하는 방식이면 좋겠다.

Comment on lines +11 to +13
protected LocalDateTime startDate;

protected LocalDateTime endDate;
Copy link
Contributor

Choose a reason for hiding this comment

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

이 두 개의 값은 서로 연관되어 있는 값이다.
이 두 개의 값을 가지는 객체를 분리하는 것은 어떨까?
분리한 후 시작일은 종료일보다 빨라야 한다와 같은 유효성 체크 로직을 추가하는 것은 어떨까?

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