-
Notifications
You must be signed in to change notification settings - Fork 247
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
3단계 - 수강신청(DB 적용) #642
base: lee-won-suk
Are you sure you want to change the base?
3단계 - 수강신청(DB 적용) #642
Conversation
1. NsUser 참조 제거 2. 기수 수정
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.
안녕하세요 원석님
기간이 끝나도 요청주시면 확인하겠습니다!
놓치신 부분이 있어서 코멘트 남겼어요
재요청 부탁드립니다!
String sql = "insert into course (title, creator_id, created_at, class_number) values(?, ?, ?,?)"; | ||
return jdbcTemplate.update(sql, course.getTitle(), course.getCreatorId(), course.getCreatedAt(),1); |
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.
순서를 지키면서 바인딩 시키는 것은 실수할 우려도 있고, 어려울 수 있어요
NamedParameterJdbcTemplate 을 참고하면 좋겠습니다.
참고만 해주세요!
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.
훨씬 편하네요.. SqlParameterSource라는 것도 있어서 이걸로 매핑해보니 훨씬 가독성도 좋았습니다. 감사합니다.
RowMapper<Course> rowMapper = (rs, rowNum) -> new Course( | ||
rs.getLong(1), | ||
rs.getString(2), | ||
rs.getLong(3), | ||
toLocalDateTime(rs.getTimestamp(4)), | ||
toLocalDateTime(rs.getTimestamp(5))); | ||
toLocalDateTime(rs.getTimestamp(5)), | ||
rs.getInt(6) | ||
); |
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.
컬럼 순서대신 이름으로 매핑시켜보면 어떨까요?
순서는 실수를 유발할 수 있고, 쿼리 변경 시 많은 변경이 요구됩니다.
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.
감사합니다. 이름으로 매핑하는 방식을 처음알았네요
이름으로 넣어보니 훨씬 가독성이 있었습니다.
} | ||
|
||
@Override | ||
public Course findById(Long id) { | ||
String sql = "select id, title, creator_id, created_at, updated_at from course where id = ?"; | ||
String sql = "select id, title, creator_id, created_at, updated_at, class_number from course where id = ?"; | ||
RowMapper<Course> rowMapper = (rs, rowNum) -> new Course( |
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.
매조회마다 매퍼를 생성할 필요는 없어보여요 상수로 두고 재사용할 수 있게 만들어보면 좋겠습니다.
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.
감사합니다. rowMapper는 각행을 어떻게 매핑할 것인지 정의하는 것이기 때문에 말씀하신 방법대로 하는게 매 생성시 new로 할당받는 것을 줄일 수 있을 것 같습니다. 수정했습니다.
|
||
@Repository("sessionRepository") | ||
public class JdbcSessionRepository implements SessionRepository { | ||
private JdbcOperations jdbcTemplate; |
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.
런타임 중 변경될일은 없으므로 final로 선언하면 어떨까요?
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.
감사합니다. 왜 불변일까를 생각해보았는데, 만약 jdbc가 가변이라고 하면 스레드 안정성등 여러가지에서 문제가 있었을 것입니다.
그러므로 말씀주신대로 final로 선언하는게 최적화 및 안정성에 좋을 것 같습니다.
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
@Repository("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.
빈 이름을 항상 적어주시는 편일까요?
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.
솔직히 맹목적으로 이름을 추가했습니다 🥹
빈이름을 적어주는 이유는 참조하기 위한 목적이 이유중 하나인 것으로 생각되는데,
그렇게 사용하고 있지는 않아서 제거했습니다.
@@ -14,23 +14,23 @@ public class Course { | |||
private Long creatorId; | |||
private LocalDateTime createdAt; | |||
private LocalDateTime updatedAt; | |||
private List<Session> sessionList; | |||
private List<Session> sessions; |
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.
연관관계를 가지도록 매핑하셨네요
Course를 조회하는 시점에 같이 조회해야하지 않을까요?
따로 Session 리스트를 주입하는 부분을 찾을 수 없는 것 같아서요
src/main/resources/schema.sql
Outdated
create table session ( | ||
id bigint generated by default as identity, | ||
session_amount int not null, | ||
is_premium boolean not null, | ||
session_state varchar not null, | ||
image_capacity int, | ||
image_type varchar, | ||
image_width int, | ||
image_height int, | ||
max_student_count int, | ||
start_date timestamp, | ||
end_date timestamp, | ||
primary key (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.
과정(Course)은 기수 단위로 운영하며, 여러 개의 강의(Session)를 가질 수 있다.
어떤 강의가 어떤 과정에 속해있다는 정보는 찾을 수 없네요
create table session_students ( | ||
id bigint not null, | ||
session_id bigint, | ||
primary key (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.
어떤 사용자가 어떤 강의에 수강 신청했는지 기록이 필요해 보입니다.
import nextstep.payments.domain.Payment; | ||
|
||
public class PricingType { | ||
|
||
private final int sessionAmount; | ||
private boolean isPremium; | ||
private final boolean isPremium; |
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.
지금은 두개의 타입만 존재하지만,
미래를 생각해서 boolean 타입대신 enum 타입으로 유연하게 변경하면 어떨까요?
요구사항이 추가되어 다른 타입의 강의도 생성될 수 있지 않을까요?
@@ -3,6 +3,9 @@ | |||
public enum SessionState { | |||
READY("READY"), START("START"), END("END"); |
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.
enum 이름과 상태를 동일하게 적어주셨는데
어떤 이유인지도 설명 부탁드립니다!
활용하여 가독성 및 안전하게 코드 수정
1. 불필요 어노테이션 네이밍 제거 2. Course 생성자 추가
1. Session 클래스에 courseId 추가
안녕하세요,
남은 기간이 얼마 남지 않았네요 ㅠ
DB매핑 경험이 거의 없어서 감을 잡기가 정말 어려웠습니다. (급하게 리뷰 올리는 점 양해부탁드립니다.ㅠ)
그리고 4단계는 아마 목요일을 넘어갈 것 같은데요, 4단계 리뷰는 어떻게 하면 되는걸까요??
NEXTSTEP 페이지의 리뷰 요청이 아니라 그냥 PR 올린 후, 포비님께 요청드리면 되는 것인지 문의드립니다.
그럼 부탁드립니다.