-
Notifications
You must be signed in to change notification settings - Fork 248
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
Step3 #384
base: henry174
Are you sure you want to change the base?
Step3 #384
Conversation
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단계 구현하시느라 수고많으셨습니다.
몇가지 코멘트 남겨두었는데 확인 부탁드려요 😃
@@ -19,8 +19,8 @@ public JdbcCourseRepository(JdbcOperations jdbcTemplate) { | |||
|
|||
@Override | |||
public int save(Course course) { | |||
String sql = "insert into course (title, creator_id, created_at) values(?, ?, ?)"; | |||
return jdbcTemplate.update(sql, course.getTitle(), course.getCreatorId(), course.getCreatedAt()); | |||
String sql = "insert into course (id, title, creator_id, created_at) values(?, ?, ?, ?)"; |
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.
id 는 자동생성되므로 따로 지정하지 않아도됩니다.
id bigint generated by default as identity
하단의 테스트가 실패하는데 확인 부탁드려요.
CourseRepositoryTest
maxUserCount = MaxRegister.infinite(); | ||
} | ||
|
||
return Session.createPaidSession( |
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.
메서드명을 보면 항상 PaidSession 이 생성되는것 처럼 보입니다.
fee 가 0 이더라도 해당 정적팩토리 메서드를 호출해야하는걸까요? 🤔
다른방법으론 PaidType 을 두는 방법도 있을것 같습니다.
} | ||
|
||
@Override | ||
public Session findById(Long 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.
현재 Session 을 찾을때 RegisteredUsers 가 매번 비어있는 상태입니다.
하단 요구사항에 맞춰 같이 조회되도록 해보면 좋을것 같아요.
CRUD 쿼리와 코드를 구현하는데 집중하기 보다 테이블을 설계하고 객체 매핑하는 부분에 집중한다.
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.
테이블 스키마까지 구성해 놓고 정작 매핑하는 걸 깜빡했네요.
@@ -18,6 +37,16 @@ create table ns_user ( | |||
primary key (id) | |||
); | |||
|
|||
create table user_session_registration ( |
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.
RegisteredUsers 에 유저를 추가하는 부분도 구현해보면 좋을것 같습니다.
Relation까지 처리하려니 한결 더 복잡해지는군요. ORM... 그는 신이야. |
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단계 구현 및 피드백까지 반영하느라 수고했어요. 👍
merge하려다 지금 단계에서 객체 설계, 테이블 설계 관련해 한번 고민해 봤으면 하는 내용이 있어 피드백 남겨봤어요.
throw new IllegalArgumentException("수강료와 지불 금액이 동일하지 않습니다."); | ||
} | ||
|
||
if (this.maxUserCount.isLargerThan(this.registeredUsers.theNumberOfUsers()) == false) { | ||
if (!this.maxUserCount.isLargerThan(this.registeredUsers.theNumberOfUsers())) { |
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와 같은 객체를 추가해 maxUserCount와 수강생 목록을 가지고 수강 신청 가능 여부를 Enrollment가 담당하도록 구현하는 것은 어떨까?
@@ -18,22 +20,32 @@ public class SessionImage { | |||
*/ | |||
private Rectangle size; |
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.
👍
return this.users.stream(); | ||
} | ||
|
||
public List<NsUser> toList() { |
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에 매핑을 하다보면 Session과 NsUser가 m:n 관계를 가지게 된다.
즉 sessionId와 nsUserId를 가지는 매핑 테이블이 필요하고, 이 매핑 정보를 담고 있는 Student와 객체로 바뀌지 않을까?
} | ||
|
||
public int imageWidth() { | ||
return original.getCoverImage().getSize().getWidth(); |
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.
return original.getCoverImage().getSize().getWidth(); | |
return original.getImageWidth(); |
또는
return original.getCoverImage().getSize().getWidth(); | |
return original.coverImage().width(); |
메서드 이름에 get을 빼고 구현하는 것은 어떨까?
import java.util.Optional; | ||
|
||
@Repository("sessionRepository") | ||
public class JdbcSessionRepository implements SessionRepository { |
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.
orm이 없는 상태에서 칼럼 수가 많은 테이블과 객체를 매핑하는 것은 정말 지옥이죠. ㅠㅠ
ps.setString(3, db.coverImageFilePath()); | ||
ps.setString(4, db.imageType()); | ||
ps.setInt(5, db.imageCapacity()); | ||
ps.setInt(6, db.imageWidth()); | ||
ps.setInt(7, db.imageHeight()); |
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과 CoverImage 테이블로 분리하고 1:1 관계를 가지도록 구현하는 것은 어떨까?
이는 좋은 접근 방법일까? 아니면 성능상 이슈가 있어 좋지 않은 방법일까?
ORM없이 매핑하려니 매우 힘들군요 ㅠ
ORM 없을 때는 다들 이걸 매 테이블과 엔티티마다 하나하나 매핑을 해 주고 있었을텐데 힘들 시절이었겠습니다.