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

[Spring Core] 최규원 미션 제출합니다. #271

Open
wants to merge 23 commits into
base: kyuwon-choi
Choose a base branch
from

Conversation

Kyuwon-Choi
Copy link

@Kyuwon-Choi Kyuwon-Choi commented May 27, 2024

안녕하세요 리뷰어님!
한 주간 미션 수행하시느라 고생 많으셨습니다.

학술제와 예비군이 겹쳐 과제에 많이 신경쓰지 못 한 것 같습니다..
예외 처리들이 조금 미숙한데 미리 양해 부탁드려요ㅠ

계층화가 잘 되어있는지를 신경써서 봐 주시면 감사하겠습니다!

감사합니다!

커밋 범위

Comment on lines 9 to 14
public record ReservationRequestDto(
String name,
@JsonFormat(pattern = "yyyy-MM-dd") LocalDate date,
@JsonFormat(pattern = "HH:mm") LocalTime time
Long time
) {
}
Copy link

Choose a reason for hiding this comment

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

dto가 적절하게 잘 사용된 것 같습니다!

Comment on lines 51 to 54
var ps = connection.prepareStatement(sql, new String[]{"id"});
ps.setString(1, reservation.getName());
ps.setDate(2, java.sql.Date.valueOf(reservation.getDate()));
ps.setTime(3, java.sql.Time.valueOf(reservation.getTime()));
ps.setLong(3, reservation.getTime().getId());
Copy link

Choose a reason for hiding this comment

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

저번 과제 피드백으로 받았던 NamedParameterJdbcTemplate 혹은 SimpleJdbcInsert을 사용하면 좋을 것 같아요!
SimpleJdbcInsert
NamedParameterJdbcTemplate

Comment on lines +38 to +44
jdbcTemplate.update(connection -> {
var ps = connection.prepareStatement(sql, new String[]{"id"});
ps.setTime(1, java.sql.Time.valueOf(time.getTime()));
return ps;
}, keyHolder);

time.setId(keyHolder.getKey().longValue());
Copy link

Choose a reason for hiding this comment

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

위와 마찬가지로 NamedParameterJdbcTemplate 혹은 SimpleJdbcInsert이 적용되면 좋을 것 같습니다!

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