Skip to content

Conversation

@ho-jun97
Copy link

피드백 반영 및 step3 리뷰 요청드립니다.

질문 1

JdbcSessionRepository를 만들때 findById 메소드에서 고민에 빠졌는데요.

(rs, rowNum) -> new Session(
rs.getLong("id"),
rs.getLong("image_id"),
...
)

했을 때, session의 생성자에는 image_id가 아니라 imageFile 을 받고 있고, image_id를 받는 생성자를 만든다해도 결국에 imageFile을 생성하거나 불러와야하는데 이 부분을 어떻게 하는지 감이 안잡혀서 일단 각각의 인스턴스를 만들어서 new Session을 했는데요.

ImageFile imageFile = new ImageFile(rs.getLong("image_id"));

위와 같이 했는데 session에는 image_id만 가지고 있어서 어떻게 생성해야하는지.. 잘 모르겠어서 질문 남깁니다.

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.

db 매핑 작업하느라 수고했어요. 👍
도메인 객체 설계 후에 db 매핑 작업하면서 어떤 점을 느끼셨나요?
repository를 추가하기는 했는데 repository와 도메인 객체를 연결하는 Service 클래스를 추가하다보니 부족한 부분이 없는지 파악하는데 어려움이 있었을 것 같아요.
수강신청을 담당하는 Service 클래스 추가해 repository와 도메인 객체를 연결해보면 좋겠습니다.

@@ -1,11 +1,15 @@
package nextstep.courses.domain;

public interface EnrollmentRule {
Copy link
Contributor

Choose a reason for hiding this comment

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

인터페이스에 너무 많은 메서드를 제공하고 있음.
이름 또한 EnrollmentRule이라 수강 신청 규칙을 판단하는 메서드만 가질 것으로 판단되는데 규칙 외의 메서드도 가지고 있는 것으로 판단됨
인터페이스의 메서드를 최소화하기 위한 설계 개선을 시도해 본다.

return jdbcTemplate.queryForObject(sql, (rs, rowNum) -> {

// 1️⃣ ImageFile (지금은 ID만 복원)
ImageFile imageFile = new ImageFile(rs.getLong("image_id"));
Copy link
Contributor

Choose a reason for hiding this comment

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

질문관련 피드백:
Session을 생성할 때 항상 ImageFile 상태 값을 함께 전달해야 한다면 image_file 테이블과 join한 후 ImageFile 값을 생성하는 것은 어떨까?

status,
enrollmentRule,
enrollments
);
Copy link
Contributor

Choose a reason for hiding this comment

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

이곳에서 모든 객체를 생성하기 보다 원시값, 문자열을 받을 수 있는 부 생성자를 추가한 후 생성자에서 객체를 생성하도록 추가하면 어떨까?
앞 단계의 미션에서 계속해서 연습한 내용임

import java.time.LocalDateTime;
import java.util.Objects;

public class Session {
Copy link
Contributor

Choose a reason for hiding this comment

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

Service Layer를 담당하는 Service 클래스도 추가해 보는 것은 어떨까?
도메인 객체로 로직을 구현한 만큼 Service Layer가 로직을 담당하지 않는지?
Service 클래스가 Repository와 도메인 객체를 어떻게 연결하는지 확인해 보면 어떨까?
이번 기회에 Service Layer의 역할은 무엇인지 고민해 보면 어떨까?

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.

2 participants