Step2 #684
Step2 #684
Conversation
README.md
Outdated
| ### 프로그래밍 요구사항 | ||
| * DB 테이블 설계 없이 도메인 모델부터 구현한다. | ||
| * 도메인 모델은 TDD로 구현한다. | ||
| * 도메인 모델은 JPA를 사용하여 구현한다. |
There was a problem hiding this comment.
요구사항엔 없는 내용인데 임의로 추가하신걸까요?
도메인 모델을 JPA entity 객체로 구현한다는 의미로 작성하신건지 궁금합니다 🤔
| @@ -9,20 +9,25 @@ public class Course { | |||
|
|
|||
| private Long creatorId; | |||
|
|
|||
| private int batchNumber; | |||
There was a problem hiding this comment.
코스는 기수 단위로 운영된다의 기수인데 generation으로 변경하겠습니다.
| public Dimension(int width, int height) { | ||
| this.width = width; | ||
| this.height = height; | ||
| } |
There was a problem hiding this comment.
변수를 초기화하기 전에 유효성 검증을 먼저하면 어떨까요?
| @@ -0,0 +1,39 @@ | |||
| package nextstep.courses.domain; | |||
|
|
|||
| import java.nio.file.attribute.AttributeView; | |||
There was a problem hiding this comment.
사용하지 않는 import는 제거해 주시면 좋을 것 같아요
| this.policy = policy; | ||
| } | ||
|
|
||
| public void register(nextstep.payments.domain.Payment payment) { |
There was a problem hiding this comment.
import 하지 않고 풀 패키지를 작성하신 이유가 궁금합니다.
| @Test | ||
| @DisplayName("정원이 초과되면 등록할 수 없다.") | ||
| void register_fail_when_exceed_limit() { | ||
| EnrollmentManager manager = new EnrollmentManager(new PaidPolicy(1, 10000L)); | ||
| manager.updateStatus(SessionStatus.OPEN); |
There was a problem hiding this comment.
정원 초과는 유료 강의 한정이니 PaidPolicyTest로 이동하면 어떨까요?
| @Test | ||
| @DisplayName("결제 금액이 일치하지 않으면 예외 발생") |
There was a problem hiding this comment.
동일한 테스트가 EnrollmentManagerTest에도 이미 존재하니 중복으로 볼 수 있지 않을까요?
| public class Session { | ||
| private final SessionInformation info; | ||
| private final EnrollmentManager enrollment; |
There was a problem hiding this comment.
책임을 작게 나누어 클래스가 많아졌으니 패키지도 분리하면 어떨까요?
| import org.springframework.jdbc.core.JdbcTemplate; | ||
| import org.springframework.jdbc.core.RowMapper; | ||
|
|
||
| public class JdbcSessionRepository implements SessionRepository { |
There was a problem hiding this comment.
DB 적용하는 단계가 아닌데 미리 구현하셨네요 😃
도메인을 설계하는데 DB 테이블이 필요하셨던건지 궁금합니다 🤔
There was a problem hiding this comment.
Course쪽에 이미 Repository가 있어서 구현해야 되는 줄 알고 넣었습니다.
확인해보니 2단계 요구사항이었군요. 이쪽 코드는 2단계때 더 수정하겠습니다.
| @@ -28,7 +28,7 @@ void setUp() { | |||
|
|
|||
| @Test | |||
| void crud() { | |||
| @@ -9,7 +11,7 @@ public class Course { | |||
|
|
|||
| private Long creatorId; | |||
|
|
|||
| private int batchNumber; | |||
| private int generation; | |||
There was a problem hiding this comment.
기수였군요 😃 기수는 Session으로 이미 표현이 된게 아닐까요?
강의 소개 페이지에서 스크롤을 마지막까지 내려보시면 여러 기수가 진행되었음을 확인할 수 있는데요.
그러니 Course는 'TDD, 클린 코드 with Java', Session을 '20기'로 볼 수 있을 것 같은데 어떻게 생각하시나요?
| package nextstep.image.domain; | ||
|
|
||
| public class Dimension { |
There was a problem hiding this comment.
패키지를 분리해 주셨네요 👍
image, policy, sessions 패키지들은 모두 course와 같은 위치가 아닌 하위로 이동하면 어떨까요?
| @@ -55,7 +55,7 @@ private Session createSampleSession() { | |||
| @Test | |||
| @DisplayName("세션 저장 및 조회") | |||
| void save_and_find() { | |||
| Long courseId = 1L; | |||
| Long courseId = 3L; | |||
There was a problem hiding this comment.
중복키로 인해 id 값을 변경해 주셨네요 👍
값을 바꾸는 것도 좋지만 테스트마다 테이블을 초기화 하도록 @BeforeEach 등을 활용해 보셔도 좋을 것 같아요.
| @@ -1,4 +1,4 @@ | |||
| package nextstep.courses.domain; | |||
| package nextstep.image.domain; | |||
|
|
|||
| public class Image { | |||
| private static final long MAX_SIZE_IN_BYTES = 1_048_576; // 1MB | |||
There was a problem hiding this comment.
이젠 피드백 중에 확인을 못하신 내용이 있는 것 같아요.
PR 페이지 중간에 이렇게 생긴 버튼이 있습니다.
클릭하시면 추가 코멘트 들이 있으니 확인 부탁드려요.
No description provided.