-
Notifications
You must be signed in to change notification settings - Fork 142
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] 안금서 미션 제출합니다. #386
base: goldm0ng
Are you sure you want to change the base?
Changes from 63 commits
d888ea0
7adbc1c
a7e7b1c
5ffe0d1
3fce7dd
4e5b6c3
45a3092
87cd948
f2ceebe
0ce07cb
b80921f
b5096b7
272e633
e3ce304
2855044
4c61b9a
3262ef2
8254291
b747795
19e40f1
2bf90d0
51b6b55
51aadd1
9ee624c
9d1d2a0
8a1f8cd
2a9ee79
3012b26
f0156fa
e72a1d9
4b98968
8b552a6
98eeee3
a293899
ee1ec84
edf05cd
927c74c
7be63ff
552f755
8ede9a7
d6d1e9d
fe2f464
f5d8642
5f71d55
55a2797
795a778
08fa6f4
4151de6
f42cf40
1927a32
38998be
c1a3e00
d6f862d
2f0a216
8db1972
a2b40fc
d1aa20d
a9c87d7
47eabd6
4f8ec4e
3f36dff
6f25741
8d0ac6b
cff2f53
0efc638
50eb526
1a5e85c
8da0faf
b599e17
033d792
f7080fd
6a5885b
5712acf
5208497
bbc0aa7
92555ec
3157680
03b7b9e
73a68e4
d3bda44
0cfa4d4
1379add
50f9f7d
7f41fe9
46d524d
66148ce
fb00e38
f29f0c6
e739292
3b1e132
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
package roomescape; | ||
|
||
import org.springframework.stereotype.Controller; | ||
import org.springframework.web.bind.annotation.GetMapping; | ||
|
||
@Controller | ||
public class MainPageController { | ||
|
||
@GetMapping("/") | ||
public String showHomePage() { | ||
return "home"; | ||
} | ||
|
||
@GetMapping("/reservation") | ||
public String showReservationForm() { | ||
return "new-reservation"; | ||
} | ||
|
||
@GetMapping("/time") | ||
public String showTimeForm() { return "time"; } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
package roomescape.business; | ||
|
||
import lombok.RequiredArgsConstructor; | ||
import org.springframework.stereotype.Service; | ||
import roomescape.domain.Reservation; | ||
import roomescape.domain.Time; | ||
import roomescape.persistence.JdbcReservationRepository; | ||
import roomescape.persistence.JdbcTimeRepository; | ||
import roomescape.presentation.dto.ReservationDto; | ||
|
||
import java.util.List; | ||
|
||
@Service | ||
@RequiredArgsConstructor | ||
public class ReservationService { | ||
|
||
private final JdbcReservationRepository reservationRepository; | ||
private final JdbcTimeRepository timeRepository; | ||
|
||
public Reservation addReservation(ReservationDto reservationDto) { | ||
Reservation reservation = convertToReservationEntity(reservationDto); | ||
return reservationRepository.save(reservation); | ||
} | ||
|
||
public List<Reservation> checkReservations() { | ||
return reservationRepository.findAll(); | ||
} | ||
|
||
public void deleteReservation(Long reservationId) { | ||
reservationRepository.delete(reservationId); | ||
} | ||
|
||
private Reservation convertToReservationEntity(ReservationDto reservationDto) { | ||
Time time = convertToTimeEntity(reservationDto); | ||
return new Reservation(null, reservationDto.name(), reservationDto.date(), time); | ||
} | ||
|
||
private Time convertToTimeEntity(ReservationDto reservationDto) { | ||
Long timeId = reservationDto.time(); | ||
return timeRepository.findById(timeId); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
package roomescape.business; | ||
|
||
import lombok.RequiredArgsConstructor; | ||
import org.springframework.stereotype.Service; | ||
import roomescape.domain.Time; | ||
import roomescape.persistence.JdbcTimeRepository; | ||
import roomescape.presentation.dto.TimeDto; | ||
|
||
import java.util.List; | ||
|
||
@Service | ||
@RequiredArgsConstructor | ||
public class TimeService { | ||
|
||
private final JdbcTimeRepository repository; | ||
|
||
public Time addTime(TimeDto timeDto) { | ||
Time time = convertToEntity(timeDto); | ||
return repository.save(time); | ||
} | ||
|
||
public List<Time> checkTimes() { | ||
return repository.findAll(); | ||
} | ||
|
||
public void deleteTime(Long timeId) { | ||
repository.delete(timeId); | ||
} | ||
|
||
private Time convertToEntity(TimeDto timeDto) { | ||
return new Time(null, timeDto.time()); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
package roomescape.domain; | ||
|
||
import lombok.Getter; | ||
|
||
@Getter | ||
public class Reservation { | ||
|
||
private Long id; | ||
private String name; | ||
private String date; | ||
private Time time; | ||
|
||
public Reservation() { | ||
} | ||
|
||
public Reservation(Long id, String name, String date, Time time) { | ||
this.id = id; | ||
this.name = name; | ||
this.date = date; | ||
this.time = time; | ||
} | ||
|
||
public void setId(Long id) { | ||
this.id = id; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package roomescape.domain; | ||
|
||
import lombok.Getter; | ||
|
||
@Getter | ||
public class Time { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 데이터베이스의 스키마의 이름은 time 이지만, 도메인의 이름이 스키마의 이름을 꼭 따라가야할까요? 도메인은 비즈니스를 정의하는 영역이죠. 데이터베이스와는 별개의 영역이에요. 조금 더 비즈니스 적으로 직관적인 이름을 가지도록 해보면 어떨까요? 이를테면 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 그러네요,, |
||
|
||
private Long id; | ||
private String time; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 시간을 다루는 객체에서, 가장 중요한 필드 값의 자료형이 적절하지 않다는 생각이 들어요! 도메인 객체가 만들어질 때, 그 도메인의 특징에 맞는 데이터가 들어갈 수 있도록, 데이터 무결성을 보장해야 한다고 생각해요. 현재는 time 값에 대한 검증이 없는 상태이기 때문에, Time 객체가 시간을 나타내는 객체로서 유효하지 않을 수 있겠다는 생각이 듭니다. 시간을 표현할 때 String보다 더 적절한 자료형이 있을지 고민해보면 좋겠어요. |
||
|
||
public Time() { | ||
} | ||
|
||
public Time(Long id, String time) { | ||
this.id = id; | ||
this.time = time; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
package roomescape.exception; | ||
|
||
import lombok.extern.slf4j.Slf4j; | ||
import org.springframework.web.bind.annotation.ControllerAdvice; | ||
import org.springframework.web.bind.annotation.ExceptionHandler; | ||
import roomescape.MainPageController; | ||
|
||
@Slf4j | ||
@ControllerAdvice(assignableTypes = MainPageController.class) | ||
public class MainPageExceptionHandler { | ||
@ExceptionHandler(Exception.class) | ||
public String handleException(Exception e) { | ||
log.error("error: " + e.getMessage()); | ||
return "error/500"; //view 렌더링 페이지는 만들지 않음! | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
package roomescape.exception; | ||
|
||
public class NotFoundReservationException extends RuntimeException { | ||
|
||
private static final String NOT_FOUND_RESERVATION_MESSAGE = "예악을 찾을 수 없습니다."; | ||
|
||
public NotFoundReservationException() { | ||
super(NOT_FOUND_RESERVATION_MESSAGE); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
package roomescape.exception; | ||
|
||
public class NotFoundTimeException extends RuntimeException { | ||
|
||
private static final String NOT_FOUND_TIME_MESSAGE = "시간을 찾을 수 없습니다."; | ||
|
||
public NotFoundTimeException(){ | ||
super(NOT_FOUND_TIME_MESSAGE); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
package roomescape.exception; | ||
|
||
import lombok.extern.slf4j.Slf4j; | ||
import org.springframework.http.ResponseEntity; | ||
import org.springframework.http.converter.HttpMessageNotReadableException; | ||
import org.springframework.validation.FieldError; | ||
import org.springframework.web.bind.MethodArgumentNotValidException; | ||
import org.springframework.web.bind.annotation.ControllerAdvice; | ||
import org.springframework.web.bind.annotation.ExceptionHandler; | ||
import roomescape.presentation.ReservationController; | ||
|
||
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
@Slf4j | ||
@ControllerAdvice(assignableTypes = ReservationController.class) | ||
public class ReservationExceptionHandler { | ||
|
||
@ExceptionHandler(MethodArgumentNotValidException.class) | ||
public ResponseEntity<Map<String, String>> handleValidationExceptions(MethodArgumentNotValidException e) { | ||
Map<String, String> errors = new HashMap<>(); | ||
|
||
for (FieldError error : e.getBindingResult().getFieldErrors()) { | ||
errors.put(error.getField(), error.getDefaultMessage()); | ||
log.info("validation error on field {} : {}", error.getField(), error.getDefaultMessage()); | ||
} | ||
|
||
return ResponseEntity.badRequest().body(errors); | ||
} | ||
|
||
@ExceptionHandler(NotFoundReservationException.class) | ||
public ResponseEntity<String> handleNotFoundReservationException(NotFoundReservationException e) { | ||
return ResponseEntity.badRequest().body(e.getMessage()); | ||
} | ||
|
||
@ExceptionHandler(HttpMessageNotReadableException.class) | ||
public ResponseEntity<String> handleInvalidTypeException(HttpMessageNotReadableException e) { | ||
return ResponseEntity.badRequest().body(e.getMessage()); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PR 본문에도 남겨뒀지만, 제 생각에는 여기서 드는 한 가지 궁금증은, 이 예외를 Reservation에 대해서만 잡아도 괜찮을까? 인 것 같네요. TimeController에서도 같은 시나리오로 이 예외가 발생할 여지가 있어보여요. 어떻게 처리하면 좋을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저도 코코닭님의 생각에 동의합니다! Time의 경우 시간을 직접 키보드로 입력받지 않고 주어진 보기로부터 마우스로 선택하는 형태로 입력을 받다보니, 잘못된 타입의 값이 들어올 거라고는 생각을 못해서 처리를 안 해줬던 것 같네요. time과 date 같이, 주어진 보기를 마우스로 클릭하는 형태로 받는 것은 클라이언트 코드에 달려있는 것이겠죠? 그렇다면 잘못된 값이 들어올 수 있는 경우가 있을까요? 궁금해서 여쭤봅니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 좋은 질문을 남겨주셨었네요! 일단 제 생각은, 서버 개발자는 API 관점에서 이를 바라보는 게 좋을 것 같아요. 서버 개발자의 입장에서 클라이언트는 API를 사용하는 대상일 거예요. 이때 클라이언트는 아래와 같을 수 있어요.
첫 번째의 경우, 프론트 개발자의 설계대로(마우스로 클릭하는 형태)만 사용자가 움직여준다면 예외가 발생하지 않을 수 있겠네요. 다만, 프론트의 설계를 기반으로 백엔드 설계를 하게 된다면, 설계가 프론트의 구조에 의존하게 되면서 변화에 취약해질 수 있어요. 이를테면, 마우스로 클릭하는 형태에서 키보드로 입력하는 형태로 바뀐다면 이에 맞추어서 예외 처리 코드를 새로 만들어 주어야 될 거예요. 두 번째의 경우, 우리가 만든 API가 어떻게 사용될 지 예측할 수 없어요. 악의적으로 API 요청을 보내는 경우도 포함될 수 있구요. 이러한 것들을 모두 고려해봤을 때, 저는 서버 관점에서만 바라보았을 때 발생할 수 있는 모든 예외는 마땅히 처리되는 편이 좋다고 생각합니다~! |
||
|
||
@ExceptionHandler(Exception.class) | ||
public ResponseEntity<String> handleException(Exception e) { | ||
return ResponseEntity.internalServerError().body(e.getMessage()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
package roomescape.exception; | ||
|
||
import lombok.extern.slf4j.Slf4j; | ||
import org.springframework.http.ResponseEntity; | ||
import org.springframework.validation.FieldError; | ||
import org.springframework.web.bind.MethodArgumentNotValidException; | ||
import org.springframework.web.bind.annotation.ControllerAdvice; | ||
import org.springframework.web.bind.annotation.ExceptionHandler; | ||
import roomescape.presentation.TimeController; | ||
|
||
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
@Slf4j | ||
@ControllerAdvice(assignableTypes = TimeController.class) | ||
public class TimeExceptionHandler { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TimeExceptionHandler의 내용이 ReservationExceptionHandler와 중복적으로 겹치는 부분들이 보이네요. 왜 겹칠까요? 예를 들어, ReservationExceptionHandler와 TimeExceptionHandler와 같이 도메인 별로 예외 처리를 나누었을 때, 내부적으로 각각 어떤 예외를 처리하는 것이 가장 효과적일지 고민해보면 좋을 것 같아요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 엇 좋은 지적 감사합니다 코코닭님!
앞으로 예외처리를 할 때에는 예외 범위에 대해 잘 생각해서 공통 예외와 도메인별 예외를 잘 구별해야겠네요! |
||
|
||
@ExceptionHandler(MethodArgumentNotValidException.class) | ||
public ResponseEntity<Map<String, String>> handleValidationExceptions(MethodArgumentNotValidException e) { | ||
Map<String, String> errors = new HashMap<>(); | ||
|
||
for (FieldError error : e.getBindingResult().getFieldErrors()) { | ||
errors.put(error.getField(), error.getDefaultMessage()); | ||
log.info("validation error on field {} : {}", error.getField(), error.getDefaultMessage()); | ||
} | ||
|
||
return ResponseEntity.badRequest().body(errors); | ||
} | ||
|
||
@ExceptionHandler(NotFoundTimeException.class) | ||
public ResponseEntity<String> handleNotFoundTimeException(NotFoundTimeException e) { | ||
return ResponseEntity.badRequest().body(e.getMessage()); | ||
} | ||
|
||
@ExceptionHandler(Exception.class) | ||
public ResponseEntity<String> handleException(Exception e) { | ||
return ResponseEntity.internalServerError().body(e.getMessage()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
package roomescape.persistence; | ||
|
||
import lombok.RequiredArgsConstructor; | ||
import org.springframework.dao.EmptyResultDataAccessException; | ||
import org.springframework.jdbc.core.JdbcTemplate; | ||
import org.springframework.jdbc.core.RowMapper; | ||
import org.springframework.jdbc.support.GeneratedKeyHolder; | ||
import org.springframework.jdbc.support.KeyHolder; | ||
import org.springframework.stereotype.Repository; | ||
import roomescape.domain.Reservation; | ||
import roomescape.domain.Time; | ||
import roomescape.exception.NotFoundReservationException; | ||
|
||
import java.sql.PreparedStatement; | ||
import java.util.List; | ||
|
||
@Repository | ||
@RequiredArgsConstructor | ||
public class JdbcReservationRepository implements ReservationRepository { | ||
|
||
private final JdbcTemplate jdbcTemplate; | ||
|
||
@Override | ||
public Reservation save(Reservation reservation) { | ||
String sql = "insert into reservation (name, date, time_id) values (?,?,?)"; | ||
|
||
KeyHolder keyHolder = new GeneratedKeyHolder(); | ||
|
||
jdbcTemplate.update(connection -> { | ||
PreparedStatement ps = connection.prepareStatement(sql, new String[]{"id"}); | ||
ps.setString(1, reservation.getName()); | ||
ps.setString(2, reservation.getDate()); | ||
ps.setLong(3, reservation.getTime().getId()); | ||
return ps; | ||
}, keyHolder); | ||
|
||
Long generatedAutoId = keyHolder.getKey().longValue(); | ||
return new Reservation(generatedAutoId, reservation.getName(), reservation.getDate(), reservation.getTime()); | ||
} | ||
|
||
@Override | ||
public Reservation findById(Long reservationId) { | ||
String sql = "select id, name, date, time_id from reservation where id = ?"; | ||
|
||
try { | ||
return jdbcTemplate.queryForObject(sql, reservationMapperForFindById(), reservationId); | ||
} catch (EmptyResultDataAccessException e) { | ||
throw new NotFoundReservationException(); | ||
} | ||
} | ||
|
||
@Override | ||
public List<Reservation> findAll() { | ||
String sql = "select \n" + | ||
" r.id as reservation_id, \n" + | ||
" r.name, \n" + | ||
" r.date, \n" + | ||
" t.id as time_id, \n" + | ||
" t.time as time_value \n" + | ||
"from reservation as r inner join time as t on r.time_id = t.id"; | ||
|
||
return jdbcTemplate.query(sql, reservationMapperForFindAll()); | ||
} | ||
|
||
@Override | ||
public void delete(Long reservationId) { | ||
Reservation deletedReservation = this.findById(reservationId); | ||
|
||
String sql = "delete from reservation where id = ?"; | ||
jdbcTemplate.update(sql, deletedReservation.getId()); | ||
} | ||
|
||
private RowMapper<Reservation> reservationMapperForFindById() { | ||
return ((rs, rowNum) -> { | ||
|
||
Time time = new Time(rs.getLong("time_id"), null); | ||
|
||
return new Reservation( | ||
rs.getLong("id"), | ||
rs.getString("name"), | ||
rs.getString("date"), | ||
time); | ||
}); | ||
} | ||
|
||
private RowMapper<Reservation> reservationMapperForFindAll() { | ||
return ((rs, rowNum) -> { | ||
|
||
Time time = new Time(rs.getLong("time_id"), rs.getString("time_value")); | ||
|
||
return new Reservation( | ||
rs.getLong("id"), | ||
rs.getString("name"), | ||
rs.getString("date"), | ||
time | ||
); | ||
}); | ||
} | ||
} |
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.
ReservationRepository와 TimeRepository에 대한 인터페이스를 만드셔서 추상화를 해주셨던데, ReservationService 및 TimeService에서는 추상화 한 객체들에 의존하지 않고 구체 클래스에 의존하고 있네요. 어떤 이유에서 이런 선택을 해주셨나요?
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.
예리한 지적이시네요 코코닭!
Spring MVC 미션에서는 메모리를 기반으로 repository를 쓰다가 Spring Jdbc로 넘어가면서 데이터베이스를 기반으로 repositroy를 쓰는 방식을 변경했기 때문에 변경에 유연하게 대처하기 위한 방법 중 하나로 인터페이스를 두는 것을 생각했습니다!
하지만,,, 그걸 service에 적용해볼 생각은 못 해봤네요.. service 같은 경우에는 현재 미션으로 봐선 따로 인터페이스를 둘 필요가 있나 싶은데, 코코닭님은 어떻게 생각하시나요? 나중에 프로젝트 규모가 커지고 비즈니스 로직이 복잡해진다면 인터페이스를 두어야 할 상황이 생기기도 하나요? 궁금합니다!
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.
소통의 오류가 있던 것 같아요. 제가 의미했던 바는 아래와 같아요.
AS-IS
TO-BE
Repository를 추상화했다면, Service에서 의존하고 있는 Repository는 구체 클래스보다는 추상화한 클래스가 되는 편이 좋을 것 같습니다. 그래야 다형성을 이용해볼 수도 있을 것 같아요~!
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.
아하 그런 의미였군요! 이해 했습니다!!!!! 반영하도록 할게요 😃