-
Notifications
You must be signed in to change notification settings - Fork 308
2단계 - 수강신청(도메인 모델) #806
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단계 - 수강신청(도메인 모델) #806
Conversation
- Session 도메인을 위한 하위 도메인도 추가 - BaseEntity - Period - SessionEnrollment - SessionStatus
- 개인별 수강 신청 가능 조건 값을 관리하는 도메인 추가 - 개인별로 확인해야 할 조건이 추가될 경우 해당 도메인을 이용
javajigi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
도메인 객체 설계 넘 잘 했네요. 💯
오랜 시간 고민한 것으로 알고 있는데 고민한 만큼의 객체 설계로 느껴질 만큼 좋네요.
역시나 db와 매핑하지 않은 상태로 도메인 객체 설계와 로직에 집중하니 완전히 다른 설계가 되는 것 같아요.
바로 db 매핑으로 넘어가려다 객체 설계와 관련해 추가 리팩터링한 후에 db 매핑하는 것이 좋을 것 같아 피드백 남겨봤어요.
추가 리팩터링 후에 db 매핑 도전해 보시죠.
| @@ -0,0 +1,13 @@ | |||
| package nextstep.courses.domain.image; | |||
|
|
|||
| public class Image { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
|
||
| import nextstep.courses.domain.session.EnrollmentCondition; | ||
|
|
||
| public class Paid implements SessionType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| checkEnrollable(request); | ||
|
|
||
| this.enrollment.enroll(request.getUser()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checkEnrollable()에 대한 확인까지 SessionEnrollment 객체가 담당하도록 구현하는 것은 어떨까?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그 전엔 뭔가 찜찜한 부분이 있었는데 확실히 이렇게 하니까 객체 설계가 깔끔해졌네요.. SessionEnrollment 객체가 Session 을 가지도록 변경해서 Enrollment 객체의 역할이 좀 더 뚜렷해진 것 같습니다!
| /** | ||
| * 강의 정원 및 현재 등록 인원을 관리하는 도메인 | ||
| */ | ||
| public class SessionEnrollment { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| * 강의 정원 및 현재 등록 인원을 관리하는 도메인 | ||
| */ | ||
| public class SessionEnrollment { | ||
| private final int maxCapacity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
원시 값을 포장한다면 역할과 책임을 부여할 수 있는 객체가 될 수 있을까?
| import java.util.HashSet; | ||
| import java.util.Set; | ||
|
|
||
| public class NsUsers { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일급 콜렉션 적용 👍
javajigi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
피드백 반영하느라 수고했어요. 👍
피드백 반영과 관련해 피드백 하나 남겼는데 다음 단계 db 매핑 작업할 때 함께 반영해 보세요.
db 매핑 작업이 생각보다 수월하지 않을 겁니다.
가능한 도메인 객체를 유지하면서 db 매핑 도전해볼 것을 추천합니다.
| public class SessionEnrollment { | ||
| private final Capacity maxCapacity; | ||
| private final NsUsers enrolledUsers; | ||
| private final Session session; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Session과 의존관계를 가지기보다 수강 신청 가능 여부를 판단할 때 필요한 아래 두 값을 가지도록 구현하는 것은 어떨까?
private SessionStatus status;
private final SessionType type;
Session에 의존할 경우 테스트 코드 구현할 때 Session 객체를 생성하는 등의 번거로운 작업도 많을 것으로 보여짐
안녕하세요!
2단계 수강신청 도메인 모델은 좀 더 현실적인 구조이기도 하고, 고민이 길어져서 리뷰를 생각보다 늦게 신청하게 되었네요..!
Session 클래스에 여전히 멤버변수가 좀 많은 편이기도 하고, 변수명이나 도메인 설계도 좀 더 좋은 방향이 있을 듯 한데.. 우선 리뷰 신청합니다!
감사합니다.