Skip to content
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

Open
wants to merge 10 commits into
base: henry174
Choose a base branch
from
Open

Step3 #384

wants to merge 10 commits into from

Conversation

henry174
Copy link

@henry174 henry174 commented Dec 12, 2023

ORM없이 매핑하려니 매우 힘들군요 ㅠ
ORM 없을 때는 다들 이걸 매 테이블과 엔티티마다 하나하나 매핑을 해 주고 있었을텐데 힘들 시절이었겠습니다.

Copy link

@csh0034 csh0034 left a 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(?, ?, ?, ?)";
Copy link

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(
Copy link

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) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 Session 을 찾을때 RegisteredUsers 가 매번 비어있는 상태입니다.

하단 요구사항에 맞춰 같이 조회되도록 해보면 좋을것 같아요.
CRUD 쿼리와 코드를 구현하는데 집중하기 보다 테이블을 설계하고 객체 매핑하는 부분에 집중한다.

Copy link
Author

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 (
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RegisteredUsers 에 유저를 추가하는 부분도 구현해보면 좋을것 같습니다.

@henry174
Copy link
Author

henry174 commented Dec 14, 2023

Relation까지 처리하려니 한결 더 복잡해지는군요. ORM... 그는 신이야.

Copy link
Contributor

@javajigi javajigi left a 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())) {
Copy link
Contributor

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;
Copy link
Contributor

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() {
Copy link
Contributor

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return original.getCoverImage().getSize().getWidth();
return original.getImageWidth();

또는

Suggested change
return original.getCoverImage().getSize().getWidth();
return original.coverImage().width();

메서드 이름에 get을 빼고 구현하는 것은 어떨까?

import java.util.Optional;

@Repository("sessionRepository")
public class JdbcSessionRepository implements SessionRepository {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

orm이 없는 상태에서 칼럼 수가 많은 테이블과 객체를 매핑하는 것은 정말 지옥이죠. ㅠㅠ

Comment on lines +55 to +59
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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이미지와 관련된 정보를 Session과 CoverImage 테이블로 분리하고 1:1 관계를 가지도록 구현하는 것은 어떨까?
이는 좋은 접근 방법일까? 아니면 성능상 이슈가 있어 좋지 않은 방법일까?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants