step3 #386
step3 #386
Conversation
src/main/java/nextstep/courses/domain/RegistrationRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/nextstep/courses/infrastructure/JdbcRegistrationRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/nextstep/courses/infrastructure/JdbcRegistrationRepository.java
Outdated
Show resolved
Hide resolved
| private final CourseRepository courseRepository; | ||
| private final SessionCoverRepository sessionCoverRepository; | ||
| private final RegistrationRepository registrationRepository; |
There was a problem hiding this comment.
관계를 맵핑하기 위해 Repository간의 의존 관계가 생겨버렸군요! 🤔
이와 같이 구현할 경우 커플링이 높아지는 단점이 생기고, 훗날 요구사항이 추가되면서 유지보수하기 힘들어질 가능성이 높아보입니다.
Service Layer 에서 Repository 를 추가해 Repository간의 의존 관계를 분리해 보는건 어떠신가요? 🤔
그리고 이번 기회에 Repository에 대해 찾아보시는것은 어떠신가요?
Repository, Dao, Mapper 같은 데이터엑세스 쪽도 같이 공부해보시는 것도 추천드릴게요!
There was a problem hiding this comment.
repository간의 양방향 참조가 없어도 Repository에서 Repository를 의존하는 방식이 어떻게 문제가 될지 잘 와닿지가 않습니다😢
그동안 같은 레이어안에 있는 리소스간의 참조가 문제가 되는지 항상 궁금했었는데 이유를 알려주실 수 있으실까요!!?
같은 맥락으로 service와 service간의 참조도 문제가 되는지 궁금합니다!!
There was a problem hiding this comment.
지금은 interface를 의존하기에 큰 문제가 발생하진 않지만 결국 같은레이어를 참조한다는건, 순환참조가 발생할 여지가 있어서입니다!
There was a problem hiding this comment.
넵 repository간의 참조를 삭제하고 서비스에서 Course, SessionCover, Registration, User를 조합하도록 수정해봤습니다.
그런데 이 과정에서 매번 Session 테이블을 조회할때마다 동일 작업이 반복되기때문에 다른 서비스에서 SessionRepository를 사용할때 코드중복이 발생할 여지가 있어보이고, 또 서비스에서 따로따로 조회함으로써 데이터의 원자성을 해친다는 생각이 들어 실제 프로덕션에서는 조인을 활용하고 말씀해주신 aggregate와 같은 전략으로 묶어서 조회하는게 더 바람직한 것 같다는 생각이 들기도 했습니다 🤔
조언해주신대로 이것저것 방법을 실제로 적용해보니 인사이트가 생긴 것 같습니다 감사합니다!🙇♀️
| public class Capacity { | ||
| private final int capacity; | ||
|
|
||
| public Capacity(int capacity) { |
There was a problem hiding this comment.
수강 인원이 1명 이상일 때 의미가 있기 때문에 1이상의 값인지에 대한 검증 로직도 필요하지 않을까?
| public class Price { | ||
| private final long price; | ||
|
|
||
| public Price(long price) { |
There was a problem hiding this comment.
강의료도 1000원 이상과 같이 본인만의 정책을 만들고 유효성 처리하도록 구현하는 것은 어떨까?
| this.updatedAt = updatedAt; | ||
| } | ||
|
|
||
| public Registration(NsUser user, Session session, Long amount) { |
| public Registration(NsUser user, Session session, Long amount) { | ||
| this.user = user; | ||
| this.session = session; | ||
| this.amount = amount; |
There was a problem hiding this comment.
부생성자는 주생성자를 호출해 필드의 값을 초기화하는 것을 추천
|
|
||
| import nextstep.courses.utils.BaseEntity; | ||
|
|
||
| public class Semester extends BaseEntity { |
There was a problem hiding this comment.
이 객체는 Course를 가지는 것 이외에 아무 역할도 담당하고 있지 않다.
이 객체가 필요한 이유는?
|
|
||
| public SessionCover(int width, int height, long size, byte[] image) { | ||
| private Long id; | ||
| private byte[] image; |
There was a problem hiding this comment.
image 바이트 정보를 가지기보다 이미지 경로 정보만 가지는 것은 어떨까?
| id bigint generated by default as identity, | ||
| ns_user_id bigint not null, | ||
| session_id bigint not null, | ||
| amount bigint, |
There was a problem hiding this comment.
굳이 amount 칼럼이 필요할까?
session과 nsuser의 m:n 매핑을 위한 ns_user_id와 session_id만을 가지는 map 테이블이어도 되지 않을까?
|
|
||
| public void startEnrollment() { | ||
| this.status = SessionStatus.ENROLL; | ||
| } | ||
|
|
||
| public void validateStatus() { | ||
| public void enroll(NsUser participant, Long amount) { |
There was a problem hiding this comment.
이 메서드의 로직을 Enrollment와 같은 도메인 객체를 추가해 로직을 분리해 보는 것은 어떨까?
Enrollment는 수강신청 로직 구현을 위해 필요한 capacity와 수강생 목록(participants)을 인스턴스 변수로 가지면 어떨까?
안녕하세요. DB 연결 step3 PR 요청드립니다.