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] 한경준 미션 제출합니다. #269

Open
wants to merge 31 commits into
base: hkjbrian
Choose a base branch
from

Conversation

hkjbrian
Copy link

@hkjbrian hkjbrian commented May 27, 2024

안녕하세요 리뷰어님 개인 사정으로 현재 step 9까지만 진행된 상태로 step 10이 완료되면 다시 말씀드리겠습니다..!
최대한 빠르게 완료해보도록 하겠습니다 죄송합니다..!

커밋 범위 : step 8
커밋 범위 : step 9 ~ step 10

Han added 30 commits May 10, 2024 16:58
# Conflicts:
#	src/main/java/roomescape/SpringConfig.java
#	src/main/java/roomescape/controller/ReservationController.java
#	src/main/resources/application.properties
#	src/main/resources/schema.sql
#	src/test/java/roomescape/MissionStepTest.java
Copy link

@nova0128 nova0128 left a comment

Choose a reason for hiding this comment

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

이번주 과제도 고생하셨습니다!

아직 스프링이 익숙하지 않아서 경준님 코드를 보면서 많이 배울 수 있었습니다!
전체적으로 스터디 때 받은 피드백을 고려하신 것 같아 리뷰하기에 수월했습니다.
앞으로 함께 열심히 공부해 보아요 👍

try {
jdbcTemplate.update(sql, id);
} catch (DataAccessException e) {
throw new ReservationExceptionHandler.NoReservationException("존재하지 않는 시간입니다.");

Choose a reason for hiding this comment

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

시간에 대한 예외 처리를 다루는 클래스를 따로 만드는 게 더 좋을 것 같아요!

@@ -0,0 +1,7 @@
package roomescape.domain.dto;

Choose a reason for hiding this comment

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

dto를 잘 사용해주신 것 같아요 👍

return new ResponseTime(id, requestTime.time());
}

public List<ResponseTime> findAll() {

Choose a reason for hiding this comment

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

sql문을 잘 활용하신 것 같아요!

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