-
Notifications
You must be signed in to change notification settings - Fork 306
3단계 수강신청(DB) #800
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
3단계 수강신청(DB) #800
Conversation
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.
3단계가 생각보다 쉽지 않죠?
그럼에도 불구하고 빠르게 도전하고 리뷰 요청한 점 👍
db 매핑 후에 추가한 Repository와 도메인 객체를 어떻게 연결할 것인지를 SessionService를 추가해 구현해 봤으면 합니다.
아직 구현이 완성된 단계가 아니라 피드백이 그리 많지 않은데요.
SessionService 추가해보면 추가로 구현해야할 부분이 보일 겁니다.
| import java.util.Objects; | ||
|
|
||
| @Repository("courseRepository") | ||
| public class JdbcCourseRepository implements CourseRepository { |
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을 담당하는 SessionRepository를 분리하는 것은 어떨까?
|
|
||
|
|
||
|
|
||
| create table session_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.
session이 image_id를 가지는 것이 아니라 이 테이블이 session_id를 가지는 것이 맞지 않을까?
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.
제가 해당 구조로 진행했던건 nextstep에서 기수별로 강의이미지가 항상 같던부분을 생각하면서 진행했었고
session과 session_image는 N대1로 하였습니다.
Session이 sessionImage를 참조하도록 하였고 같은 이미지를 여러 session들이 재사용 가능하도록 한다고 생각하고 진행하였습니다.
말씀해주신 구조라면 SessionImage가 Session을 참조하고 1대1 관계가 될 것 같은데 이 구조로 변경하는게 괜찮은 구조일까요?
| foreign key (image_id) references session_image (id) | ||
| ); | ||
|
|
||
| create table session_enrollment |
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 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과 SessionRepository를 사용해 수강 신청을 담당하도록 SessionService까지 구현해 본다.
SessionService를 구현해보면 추가로 필요한 부분을 찾을 수 있을 것으로 보여짐
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.
피드백 반영하느라 수고했어요. 👍
지금 단계에서 바로 다음 단계로 넘어가려다 지금 상태의 코드에서 추가로 연습해 봤으면 하는 부분이 있어 피드백 남겨 봅니다.
| @@ -0,0 +1,5 @@ | |||
| package nextstep.courses.domain.session; | |||
|
|
|||
| public interface EnrollmentRepository { | |||
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 void enroll(Long courseId, int cohort, Long nsUserId, Payment payment) { | ||
| Sessions sessions = sessionRepository.findByCourseId(courseId); | ||
| Session session = sessions.findByCohort(cohort); |
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 void enroll(Long courseId, int cohort, Long nsUserId, Payment payment) { | |
| Sessions sessions = sessionRepository.findByCourseId(courseId); | |
| Session session = sessions.findByCohort(cohort); | |
| public void enroll(long sessionId, Long nsUserId, Payment payment) { | |
| Sessions session = sessionRepository.findById(sessionId); |
특정 세션에 수강신청하는 것이라 굳이 전체 Session 목록을 가져올 필요는 없지 않을까?
| if (session == null) { | ||
| throw new IllegalArgumentException("해당 기수의 강의를 찾을 수 없습니다."); | ||
| } | ||
|
|
||
| session.enroll(nsUserId, payment); | ||
|
|
||
| Long sessionId = sessionRepository.findSessionIdByCourseIdAndCohort(courseId, cohort); | ||
| enrollmentRepository.save(sessionId, nsUserId); |
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 객체 내에 두는 것이 아니라 아래와 같이 Session을 통해 수강신청에 필요한 객체를 아래의 Enrollment와 같은 객체로 생성한 후 위임함으로써 복잡도를 낮추는 방식으로 접근해 보는 것은 어떨까?
아래 Student는 승인님 코드의 Enrollment와 같다고 생각하면 됩니다.
Session session = ...
List<Student> students = ...
Enrollment enrollment = session.enrollment(students);
Student student = enrollment.enroll(user);
enrollmentRepository.save(student.getUserId(), student.getSessionId());위 코드에서 리팩터링 힌트를 얻을 수 있는 부분이 있다면 리팩터링해 본다.
|
|
||
| @Service | ||
| @Transactional | ||
| public class CourseService { |
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의 수강신청인만큼 Course는 고려하지 않아도 된다.
시간을 많이 쓰지 않아도 될 것 같아 피드백 남김
| Session session1 = new Session(1, new SessionPeriod(startDate, endDate), image, new Enrollment(SessionStatus.RECRUITING, new FreeSessionType())); | ||
| Session session2 = new Session(2, new SessionPeriod(startDate, endDate), image, new Enrollment(SessionStatus.RECRUITING, new FreeSessionType())); |
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 객체와 같이 인스턴스 변수가 많은 객체를 테스트하려면 객체를 생성하는데 어려움이 있다.
중복 코드 또한 많이 발생해 Session을 생성할 때 생성자의 인자가 변경되는 경우 변경할 부분이 많아진다.
https://www.arhohuttunen.com/test-data-builders/ 문서 참고해 Session과 같이 복잡한 객체의 테스트 데이터를 생성할 때 어떤 방법을 사용할 것인지 선택해 보면 좋겠다.
이번 기회에 내가 선호하는 방법을 적용해 보고 앞으로도 쭈욱 활용하는 방식이면 좋겠다.
|
#800 (comment) |
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.
현장에서 프로젝트 진행하다보면 객체를 어떻게 분리하고 역할/책임을 부여할지는 항상 고민인 것 같아요.
그런 고민을 해볼 수 있는 좋은 기회였기를 바랍니다. 👍
이 미션의 마지막 단계 진행해 보시죠.
마지막 단계 연습은 요구사항을 반영하면서 점진적인 리팩터링 연습을 해볼 것을 추천합니다.
| private final int cohort; | ||
| private final SessionPeriod period; | ||
| private final SessionImage coverImage; | ||
| private final Enrollment enrollment; |
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.
Enrollment에 sessionStatus와 sessionType을 포장하고 있는 역할 밖에 하고 있지 않아 Session이 sessionStatus와 sessionType을 필드로 가지고 있다 createEnrollment()를 통해 Enrollment를 생성해도 되지 않을까?
안녕하세요 자바지기님
이번 미션을 진행하면서 고민했던부분이 몇개있었던거같아요
고민1. Session 테이블이 너무 컬럼이 많은게아닌가?
테이블역시 너무 많은 책임을 가진게 아닌가란 생각을 했었습니다. join이 필요하지 않아 성능적으로도 우수하겠지만 성능보다 도메인 객체에 로직 구현하라고 하신부분이 있었고 lms을 nextstep을 생각하며 진행하다보니 image 테이블정도는 분리하면 이미지를 재사용할 수 있는 점에서도 이점이 있을 것 같아서 분리하였습니다. nextstep에서는 Course에서 사용하는 이미지를 Session별로 다르지않고 같은 이미지를 사용하는듯하여 생각하였습니다.
고민2. Enrollment의 enrolledStudentIds
Enrollment에서 enrolledStudentIds는 수강신청한 학생들 인데 이부분은 session_enrollment로 1대N관계의 명확하게 표현해주는게 좋을 것같아서 분리하였습니다.
CourseRepositoryTest테스트를 진행하다보니 이전 테스트와 충돌로 인해 정확한 아이디비교가 아닌 0보다 큰값을 가진다로 해놨는데 테스트 격리도 신경써야겠네요.
아직 구현을 못한 부분들이 존재합니다.
session_enrollment에서 사용자 저장하는 부분이나,
현재 잘 동작하는 지 확인하기위해 JDBCCourseRepository를보면 status나 type이나 이미지 크기, 타입등을 우선 하드코딩으로 한부분들이 있는데 이부분들은 추가 개선해서 리뷰요청 드리겠습니다.