-
Notifications
You must be signed in to change notification settings - Fork 164
[Spring MVC] 이상현 미션 제출합니다. #463
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
base: idealhyun
Are you sure you want to change the base?
Changes from all commits
6a0445a
e9636eb
fe9f64e
e1af809
512b0f4
434c777
298d6b6
9892c41
d64dae2
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,12 @@ | ||
package roomescape.home.controller; | ||
|
||
import org.springframework.stereotype.Controller; | ||
import org.springframework.web.bind.annotation.GetMapping; | ||
|
||
@Controller | ||
public class HomeController { | ||
@GetMapping("/") | ||
public String displayHomePage() { | ||
return "home"; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
package roomescape.reservation.controller; | ||
|
||
import java.net.URI; | ||
import java.util.List; | ||
import lombok.RequiredArgsConstructor; | ||
import org.springframework.http.ResponseEntity; | ||
import org.springframework.stereotype.Controller; | ||
import org.springframework.web.bind.annotation.DeleteMapping; | ||
import org.springframework.web.bind.annotation.ExceptionHandler; | ||
import org.springframework.web.bind.annotation.GetMapping; | ||
import org.springframework.web.bind.annotation.PathVariable; | ||
import org.springframework.web.bind.annotation.PostMapping; | ||
import org.springframework.web.bind.annotation.RequestBody; | ||
import org.springframework.web.bind.annotation.ResponseBody; | ||
import roomescape.reservation.dto.ReservationRequest; | ||
import roomescape.reservation.dto.ReservationResponse; | ||
import roomescape.reservation.service.ReservationService; | ||
|
||
@Controller | ||
@RequiredArgsConstructor | ||
public class ReservationController { | ||
private final ReservationService reservationService; | ||
|
||
@GetMapping("/reservation") | ||
public String displayReservationPage() { | ||
return "reservation"; | ||
} | ||
|
||
@GetMapping("/reservations") | ||
@ResponseBody | ||
public ResponseEntity<List<ReservationResponse>> getReservations() { | ||
return ResponseEntity.ok(reservationService.getReservations()); | ||
} | ||
|
||
@PostMapping("/reservations") | ||
@ResponseBody | ||
public ResponseEntity<ReservationResponse> addReservation(@RequestBody ReservationRequest request) { | ||
ReservationResponse response = reservationService.addReservation(request); | ||
URI location = URI.create("/reservations/" + response.id()); | ||
return ResponseEntity.created(location).body(response); | ||
} | ||
|
||
@DeleteMapping("/reservations/{id}") | ||
public ResponseEntity<Void> cancelReservation(@PathVariable Long id) { | ||
reservationService.deleteReservation(id); | ||
return ResponseEntity.noContent().build(); | ||
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. 전체적으로 적절하게 status code를 써주셨군요! |
||
} | ||
|
||
@ExceptionHandler(IllegalArgumentException.class) | ||
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가 IllegalArgumentException을 받도록 하였군요. 별도로 정의한 NotFound가 아닌 Illegal로하신 이유가 있나요? 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. 지금 코드에서 모든 예외가 400으로 처리되는데 NotFound 예외도 같은 400이라 굳이 나눌 필요가 있을까? 라는 생각이 들었습니다. 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. 알고계시다면 좋습니다. 더 세분화할 필요가 있을땐 따로 더 메서드를 만들고 별도의 Exception class를 넣으면되용 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를 보통 Controller에서 정의하는 것보단 ConrollerAdvice라는 클래스를 정의해서 거기에 작업하는 경우가 많습니다. 이에 대해 한번 학습해보고 적용해보면 좋을 것 같아요 |
||
public ResponseEntity<String> handleIllegalArgument(IllegalArgumentException e) { | ||
return ResponseEntity.badRequest().body(e.getMessage()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
package roomescape.reservation.dto; | ||
|
||
import com.fasterxml.jackson.annotation.JsonFormat; | ||
import java.time.LocalDate; | ||
import java.time.LocalTime; | ||
|
||
public record ReservationRequest( | ||
String name, | ||
LocalDate date, | ||
@JsonFormat(pattern = "HH:mm") | ||
LocalTime time) { | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
package roomescape.reservation.dto; | ||
|
||
import com.fasterxml.jackson.annotation.JsonFormat; | ||
import java.time.LocalDate; | ||
import java.time.LocalTime; | ||
|
||
public record ReservationResponse( | ||
Long id, | ||
String name, | ||
LocalDate date, | ||
@JsonFormat(pattern = "HH:mm") | ||
LocalTime time) { | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
package roomescape.reservation.entity; | ||
|
||
import java.time.LocalDate; | ||
import java.time.LocalTime; | ||
import lombok.Data; | ||
|
||
@Data | ||
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. DataAnnotation은 어떤 역할을 해주나요? 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. 솔직하게 Getter, Setter, RequiredArgsConstructor 까지만 알고 사용했었는데 @EqualsAndHashCode, @tostring 은 잘 몰랐는데 이부분도 공부해야될 것 같습니다. 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. 아주 좋습니다!! 보고 공부하려는게 아주 좋은것 같아요 (개인적으로) Data 어노테이션 쓰는걸 별로 안좋아하는데요.. Setter가 꼭 필요한 상황이 아니면 안해야 도메인을 잘 지킬 수 있다고 생각해요. 물론 잘 인지하고 쓰면 상관없죠 |
||
public class Reservation { | ||
private Long id; | ||
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. 요거는 한번 이야기했어야 했는데 조금 늦었군요. 혹시 자바의 final 키워드에 대해 아시나요? 변수에 대해서 reassign 을 막는 키워드인데, 객체의 불변을 유지하기에 좋습니다. 이 키워드를 이용해서 우리는 immutable하게 값을 사용할 수 있습니다. 최근 개발 패러다임에선 이런 immutable한 값을 사용하는게 선호되고 있는데 왜 그런걸까요? 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. 변하지 않는다는 것을 가정하에 코드를 보고 유지보수하게되면 코드의 결과나 에러 예측 등이 훨씬 쉬워질 것 같습니다. 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. 맞습니다. 사실 가변한 값은 다루기가 굉장히 짜증나요. 디버깅할때 값이 어디서 바뀌었는지 파악하기가 힘들거든요. 개인적으론 일단 무조건 final 붙이고 가변한 값이면 final을 떼는 식으로 저는 사용해요. |
||
private String name; | ||
private LocalDate date; | ||
private LocalTime time; | ||
|
||
public Reservation(Long id, String name, LocalDate date, LocalTime time) { | ||
validate(name, date, time); | ||
this.id = id; | ||
this.name = name; | ||
this.date = date; | ||
this.time = time; | ||
} | ||
|
||
private void validate(String name, LocalDate date, LocalTime 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. 요런것들을 한번 테스트코드로 확인해보면 좋을 거 같아요. 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. 같은 날짜, 같은 시각 예약들어오는 부분도 예외 처리를 안해서 그부분까지 관련해서 테스트 코드를 작성했습니다. 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 를 테스트하는 건 저렇게 RestAssured로 테스트를 하기도 하지만, 쓰는 곳은 많이 못봤던 것 같아요. 그래서 다르게 테스트를 하기도 합니다. Service Layer 이하를 다 모킹해버려서 APi가 정상적으로 들어오는지만 확인하는 테스트를 짜기도 하고 그리고 RestAssured 큐컴버라고 또 다르게 인수테스트 도구가 있습니다. 암튼 말이 좀 길어졌는데, |
||
if (name == null || name.trim().isEmpty()) { | ||
throw new IllegalArgumentException("유효한 이름이 아닙니다."); | ||
} | ||
if (date == null || date.isBefore(LocalDate.now())) { | ||
throw new IllegalArgumentException("유효한 날짜 값이 아닙니다."); | ||
} | ||
if (date.equals(LocalDate.now()) && time.isBefore(LocalTime.now())) { | ||
throw new IllegalArgumentException("현재 시간보다 이전 시간으로는 예약할 수 없습니다."); | ||
} | ||
if (time == null) { | ||
throw new IllegalArgumentException("유효한 시간이 아닙니다."); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
package roomescape.reservation.exception; | ||
|
||
public class NotFoundReservationException extends IllegalArgumentException { | ||
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. 별도로 NotFoundException을 정의해주신 이유는 무엇인가요? 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. 자료에 주어지기도 했고, 있어서 나쁠건 없겠다라는 생각으로 만들었던 것 같습니다. 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. 커스텀 예외를 만드는 것에서 외부(프론트)에게 전달되는 목적도 있지만 내부적으로 다루기 위함도 있어요. 사실 지금 우리가 NotFound의 경우에는 별도로 예외처리를 안 하고있지만, 이게 추후에 추가될수도 있으니까요. |
||
public NotFoundReservationException(String message) { | ||
super(message); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
package roomescape.reservation.service; | ||
|
||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.concurrent.ConcurrentHashMap; | ||
import java.util.concurrent.atomic.AtomicLong; | ||
import java.util.stream.Collectors; | ||
import org.springframework.stereotype.Service; | ||
import roomescape.reservation.dto.ReservationRequest; | ||
import roomescape.reservation.dto.ReservationResponse; | ||
import roomescape.reservation.entity.Reservation; | ||
import roomescape.reservation.exception.NotFoundReservationException; | ||
|
||
@Service | ||
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. Service 클래스를 별도로 작성해주셨군요. Service는 어떤 역할을 해주나요? 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. 이때까지 공부했던 Service는 비즈니스 로직을 처리한다고 알고 있습니다. |
||
public class ReservationService { | ||
private final Map<Long, Reservation> reservations = new ConcurrentHashMap<>(); | ||
private final AtomicLong index = new AtomicLong(1); | ||
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. AtomicLong은 어떤 역할을 하고, 왜 쓰였나요? 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. 동시에 값을 추가할때 동시성을 보장하고 인조 인덱스처럼 사용하려고 쓰인 것 같습니다. |
||
|
||
public List<ReservationResponse> getReservations() { | ||
return reservations.values().stream() | ||
.map(this::toResponseDTO) | ||
.collect(Collectors.toList()); | ||
} | ||
|
||
public ReservationResponse addReservation(ReservationRequest request) { | ||
if (isDuplicate(request)) { | ||
throw new IllegalArgumentException("이미 해당 날짜와 시간에 예약이 존재합니다."); | ||
} | ||
|
||
Long id = index.getAndIncrement(); | ||
Reservation newReservation = new Reservation(id, request.name(), request.date(), request.time()); | ||
reservations.put(id, newReservation); | ||
return toResponseDTO(newReservation); | ||
} | ||
|
||
public void deleteReservation(Long id) { | ||
Reservation target = reservations.remove(id); | ||
if (target == null) { | ||
throw new NotFoundReservationException("해당 ID의 예약이 존재하지 않습니다: "); | ||
} | ||
} | ||
|
||
private ReservationResponse toResponseDTO(Reservation reservation) { | ||
return new ReservationResponse( | ||
reservation.getId(), | ||
reservation.getName(), | ||
reservation.getDate(), | ||
reservation.getTime() | ||
); | ||
} | ||
|
||
private boolean isDuplicate(ReservationRequest request) { | ||
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. 요기서 Duplicate 여부를 각각의 Date와 Time을 꺼내서 확인하고 있는데 reservation에서 isSame 같은 메서드를 만들어서 Reservation에게 값이 같은지에 대한 작업을 처리하는 책임을 주면 어떨까요? |
||
return reservations.values().stream().anyMatch(reservation -> | ||
reservation.getDate().equals(request.date()) && | ||
reservation.getTime().equals(request.time()) | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
package roomescape; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertThrows; | ||
|
||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.DisplayName; | ||
import org.junit.jupiter.api.Test; | ||
import roomescape.reservation.dto.ReservationRequest; | ||
import roomescape.reservation.exception.NotFoundReservationException; | ||
|
||
import java.time.LocalDate; | ||
import java.time.LocalTime; | ||
import roomescape.reservation.service.ReservationService; | ||
|
||
class ReservationServiceTest { | ||
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 ReservationService service; | ||
|
||
@BeforeEach | ||
void setUp() { | ||
service = new ReservationService(); | ||
} | ||
|
||
@Test | ||
@DisplayName("동일한 날짜, 시간에 예약하면 예외 발생") | ||
void throwExceptionWhenDuplicateReservation() { | ||
ReservationRequest request = new ReservationRequest("홍길동", LocalDate.now().plusDays(1), LocalTime.of(10, 0)); | ||
service.addReservation(request); | ||
|
||
ReservationRequest duplicateRequest = new ReservationRequest("김철수", LocalDate.now().plusDays(1), LocalTime.of(10, 0)); | ||
|
||
assertThrows(IllegalArgumentException.class, () -> service.addReservation(duplicateRequest)); | ||
} | ||
|
||
@Test | ||
@DisplayName("존재하지 않는 예약 ID로 삭제 시 예외 발생") | ||
void deleteNonExistingReservationThrows() { | ||
assertThrows(NotFoundReservationException.class, () -> service.deleteReservation(999L)); | ||
} | ||
} |
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.
reservation 패키지와 home 패키지를 나눴군요. 둘은 어떤 차이인가요?
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.
Controller 가 특정 페이지를 반환하는 것 같아서 프론트처럼 페이지처럼 나눴던 것 같습니다.