-
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?
Conversation
this.time = time; | ||
} | ||
|
||
public static Reservation from(ReservationRequestDTO dto, Long 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.
Reservation이라는 domain이 ReservationDto에 의존하고 있군요.
상현님이 생각하기엔 DTO는 어떤 역할인가요?
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.
DTO 클라이언트측에서 api를 통해 요청했을 때 필요한 내용만 보내어 내부 구조를 막는 느낌으로 알고 있었습니다.
Reservation 내부에 코드를 넣을 것이아니라 외부에서 Reservation에서 필요한 값만 꺼내서 담아서 보내주는식으로 고쳐야할 것 같습니다.
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.
맞아요. 내부구조를 숨기는 역할이 있죠.
그리고 DTO는 일종의 view역할을 합니다. 이게 외부가 요구하는 데이터의 양식이니까요.
그래서 자주 바뀌기도 해서 Domain인 Reservation -> Dto인 ReservationResponse를 의존하지 않게 둡니다.
도메인이 view에 대한 정보를 안 알게 하는 것이 중요하니까요.
import java.time.LocalDate; | ||
import java.time.LocalTime; | ||
|
||
public record ReservationRequestDTO( |
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.
이 class에 postfix로 DTO라는 이름을 붙여주셨군요. 이미 패키지로 이 클래스가 dto에 속해있다는 건 표현이 될 것 같아요. 중복되는 내용은 지워볼까요?
import org.springframework.web.bind.annotation.GetMapping; | ||
|
||
@Controller | ||
public class HomeController { |
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 가 특정 페이지를 반환하는 것 같아서 프론트처럼 페이지처럼 나눴던 것 같습니다.
@Service | ||
public class ReservationService { | ||
private final List<Reservation> reservations = new ArrayList<>(); | ||
private final AtomicLong index = new AtomicLong(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.
AtomicLong은 어떤 역할을 하고, 왜 쓰였나요?
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.
동시에 값을 추가할때 동시성을 보장하고 인조 인덱스처럼 사용하려고 쓰인 것 같습니다.
@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 comment
The reason will be displayed to describe this comment to others. Learn more.
전체적으로 적절하게 status code를 써주셨군요!
return new Reservation(id, dto.name(), dto.date(), dto.time()); | ||
} | ||
|
||
private void validate(String name, LocalDate date, LocalTime 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.
요런것들을 한번 테스트코드로 확인해보면 좋을 거 같아요.
아니면 이러한 테스트는 필요없을까요? MissionStepTest로 원하는 케이스를 모두 확인했으니까요
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.
같은 날짜, 같은 시각 예약들어오는 부분도 예외 처리를 안해서 그부분까지 관련해서 테스트 코드를 작성했습니다.
이런 도메인의 유효성 관련 코드는 필요하다고 생각합니다.
그런데 api와 관련된 코드들은 주어진 테스트 코드처럼 테스트를 하는건가요?
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.
음 테스트엔 다양한 형태가 있어요. 저희가 지금까지 배웠던 테스트는 하나의 도메인만 참여해서 흔히 단위 테스트라고 부르고,
여러개의 레이어(스프링 같은 프레임워크)까지 포함되는 테스트를 통합테스트라고 하기도 합니다.
프로그램 전체가 참여해 하나의 시나리오를 이루면 인수테스트라고도 하고요.
요 용어는 테스트 피라미드
라는 키워드를 한번 알아보면 좋을 것 같네요.
api 를 테스트하는 건 저렇게 RestAssured로 테스트를 하기도 하지만, 쓰는 곳은 많이 못봤던 것 같아요.
엄청 세팅하기가 번거롭고 관리하기가 힘들거든요. 그리고 테스트를 아무리 짜도 100% 보장이 되진 않으니까요.
그래서 다르게 테스트를 하기도 합니다. Service Layer 이하를 다 모킹해버려서 APi가 정상적으로 들어오는지만 확인하는 테스트를 짜기도 하고
또는, Spring에서 이렇게 테스트를 짜는게 아니라, 외부에서 주기적으로 API를 call해서 이상을 탐지하는 방식도 있습니다.
요건 BlazeMeter라는 도구가 있습니다.
이전에 있었던 회사에서는 RestAssured를 쓰기보단 이런방식으로 했네요. 아마 당시에 RestAssured랑 스프링에서 테스트 환경을 구축하는 것에 지식이 있는 사람이 있진 않아서 이런식으로 했던 것 같아요.
그리고 RestAssured 큐컴버라고 또 다르게 인수테스트 도구가 있습니다.
암튼 말이 좀 길어졌는데, api와 관련된 코드들은 주어진 테스트 코드처럼 테스트를 하는건가요?
라는 질문엔
네, api 와 관련된 코드 또한 테스트를 합니다. 하지만 이는 무거운 작업이라 회사의 상황에 따라서 어디까지 테스트할지가 갈립니다.
return ResponseEntity.noContent().build(); | ||
} | ||
|
||
@ExceptionHandler(IllegalArgumentException.class) |
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.
ExceptionHandler가 IllegalArgumentException을 받도록 하였군요.
별도로 정의한 NotFound가 아닌 Illegal로하신 이유가 있나요?
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.
지금 코드에서 모든 예외가 400으로 처리되는데 NotFound 예외도 같은 400이라 굳이 나눌 필요가 있을까? 라는 생각이 들었습니다.
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.
알고계시다면 좋습니다. 더 세분화할 필요가 있을땐 따로 더 메서드를 만들고 별도의 Exception class를 넣으면되용
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.
더불어 ExceptionHandler를 보통 Controller에서 정의하는 것보단 ConrollerAdvice라는 클래스를 정의해서 거기에 작업하는 경우가 많습니다.
이에 대해 한번 학습해보고 적용해보면 좋을 것 같아요
import roomescape.reservation.dto.ReservationRequestDTO; | ||
import roomescape.reservation.dto.ReservationResponseDTO; | ||
|
||
@Data |
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.
DataAnnotation은 어떤 역할을 해주나요?
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.
솔직하게 Getter, Setter, RequiredArgsConstructor 까지만 알고 사용했었는데 @EqualsAndHashCode, @tostring 은 잘 몰랐는데 이부분도 공부해야될 것 같습니다.
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.
아주 좋습니다!! 보고 공부하려는게 아주 좋은것 같아요
(개인적으로) Data 어노테이션 쓰는걸 별로 안좋아하는데요.. Setter가 꼭 필요한 상황이 아니면 안해야 도메인을 잘 지킬 수 있다고 생각해요.
물론 잘 인지하고 쓰면 상관없죠
|
||
@Data | ||
public class Reservation { | ||
private Long 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.
요거는 한번 이야기했어야 했는데 조금 늦었군요.
혹시 자바의 final 키워드에 대해 아시나요? 변수에 대해서 reassign 을 막는 키워드인데, 객체의 불변을 유지하기에 좋습니다.
이 키워드를 이용해서 우리는 immutable하게 값을 사용할 수 있습니다. 최근 개발 패러다임에선 이런 immutable한 값을 사용하는게 선호되고 있는데 왜 그런걸까요?
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.
맞습니다. 사실 가변한 값은 다루기가 굉장히 짜증나요. 디버깅할때 값이 어디서 바뀌었는지 파악하기가 힘들거든요.
그래서 최대한 값을 불변으로 두어서 변경을 하는 범위를 최소화시켜 예측하고 통제할 수 잇는 코드를 만들 수 있습니다.
개인적으론 일단 무조건 final 붙이고 가변한 값이면 final을 떼는 식으로 저는 사용해요.
실제로 최신 프로그래밍 언어는 별다른 키워드를 안붙이면 기본적으로 불변입니다.
@@ -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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
자료에 주어지기도 했고, 있어서 나쁠건 없겠다라는 생각으로 만들었던 것 같습니다.
어차피 400번 오류를 띄워야할거라면 커스텀예외를 만들필요가 없었을까요?
만들고나니 클라이언트측에서 예약 삭제에 관한 api 응답으로 400번 오류를 띄워주면 'id가 잘못되었구나'라는 것을 알수도 있을거란 생각이 들었습니다.
만약에 커스텀 예외를 만든다면 id 형태가 잘못된건지, 해당 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.
커스텀 예외를 만드는 것에서 외부(프론트)에게 전달되는 목적도 있지만 내부적으로 다루기 위함도 있어요.
사실 지금 우리가 NotFound의 경우에는 별도로 예외처리를 안 하고있지만, 이게 추후에 추가될수도 있으니까요.
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.
기본적으로 웹 환경에 대한 기본이 좋네요.
몇몇 부분 읽었고 리뷰 남겼습니다. 한번 보고 답변해주세요 늘 그렇듯 반박, 의문은 환영입니다
모든 예외를 동일하게 400 코드로 처리한다고 가정한다해도, 구분을 하려면 아마 지금처럼 에러메시지 기준으로 나누는 거겠죠? 물론 프론트엔드는 서버가 전달한 예외 메시지를 기반으로 동작을 제어할 수 있습니다. 그러나 다음과 같은 문제점이 있습니다: 따라서 에러 메시지를 기반으로 핸들링하는 것은 추천하지 않는 방식입니다. 그래서 다른 방법을 선택해요. 많은 기업에서는 HTTP status code만으로는 복잡한 에러 케이스들을 구분할 수 없기 때문에, 별도의 에러 코드 체계를 도입합니다. 이런식으로 각 예외마다 디테일한 정보를 넣어야 하기 때문에 커스텀 예외를 나누는 건 의미가 있습니다. 각 커스텀 예외 클래스는 더 많은 정보를 잘 표현할 수 있으니까요. |
public void deleteReservation(Long id) { | ||
Reservation target = reservations.stream() | ||
.filter(reservation -> reservation.getId().equals(id)) | ||
.findFirst() | ||
.orElseThrow(() -> new NotFoundReservationException("해당 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.
잘 짜셨습니다. 로직이 깔끔하군요.
하지만, 하나 더 개선해볼만한 포인트가 있을 것 같아요. List 형식으로 된 collection에 객체를 저장하게 되면, 조건에 맞는 값을 조회할 떄 이렇게 O(n)만큼의 시간이 걸립니다.
존재하지 않는 경우엔 더 걸리죠.
요걸 O(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.
List를 Map 형태로 변경했습니다.
import roomescape.reservation.entity.Reservation; | ||
import roomescape.reservation.exception.NotFoundReservationException; | ||
|
||
@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.
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.
이때까지 공부했던 Service는 비즈니스 로직을 처리한다고 알고 있습니다.
DB와 컨트롤러에서 넘어온 값들을 받아서 무언가를 처리하는 존재로만 알고 있었습니다.
계산하고 확인하고 무언갈 저장, 삭제, 수정 등을 하는 것 같습니다.
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.
코드 확인하고 리뷰 남겼습니다!! 한 번 확인해주세요~!!
늘 그렇듯이 의문 반박은 환영입니당.
this.time = time; | ||
} | ||
|
||
public static Reservation from(ReservationRequestDTO dto, Long 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.
맞아요. 내부구조를 숨기는 역할이 있죠.
그리고 DTO는 일종의 view역할을 합니다. 이게 외부가 요구하는 데이터의 양식이니까요.
그래서 자주 바뀌기도 해서 Domain인 Reservation -> Dto인 ReservationResponse를 의존하지 않게 둡니다.
도메인이 view에 대한 정보를 안 알게 하는 것이 중요하니까요.
return new Reservation(id, dto.name(), dto.date(), dto.time()); | ||
} | ||
|
||
private void validate(String name, LocalDate date, LocalTime 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.
음 테스트엔 다양한 형태가 있어요. 저희가 지금까지 배웠던 테스트는 하나의 도메인만 참여해서 흔히 단위 테스트라고 부르고,
여러개의 레이어(스프링 같은 프레임워크)까지 포함되는 테스트를 통합테스트라고 하기도 합니다.
프로그램 전체가 참여해 하나의 시나리오를 이루면 인수테스트라고도 하고요.
요 용어는 테스트 피라미드
라는 키워드를 한번 알아보면 좋을 것 같네요.
api 를 테스트하는 건 저렇게 RestAssured로 테스트를 하기도 하지만, 쓰는 곳은 많이 못봤던 것 같아요.
엄청 세팅하기가 번거롭고 관리하기가 힘들거든요. 그리고 테스트를 아무리 짜도 100% 보장이 되진 않으니까요.
그래서 다르게 테스트를 하기도 합니다. Service Layer 이하를 다 모킹해버려서 APi가 정상적으로 들어오는지만 확인하는 테스트를 짜기도 하고
또는, Spring에서 이렇게 테스트를 짜는게 아니라, 외부에서 주기적으로 API를 call해서 이상을 탐지하는 방식도 있습니다.
요건 BlazeMeter라는 도구가 있습니다.
이전에 있었던 회사에서는 RestAssured를 쓰기보단 이런방식으로 했네요. 아마 당시에 RestAssured랑 스프링에서 테스트 환경을 구축하는 것에 지식이 있는 사람이 있진 않아서 이런식으로 했던 것 같아요.
그리고 RestAssured 큐컴버라고 또 다르게 인수테스트 도구가 있습니다.
암튼 말이 좀 길어졌는데, api와 관련된 코드들은 주어진 테스트 코드처럼 테스트를 하는건가요?
라는 질문엔
네, api 와 관련된 코드 또한 테스트를 합니다. 하지만 이는 무거운 작업이라 회사의 상황에 따라서 어디까지 테스트할지가 갈립니다.
return ResponseEntity.noContent().build(); | ||
} | ||
|
||
@ExceptionHandler(IllegalArgumentException.class) |
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.
알고계시다면 좋습니다. 더 세분화할 필요가 있을땐 따로 더 메서드를 만들고 별도의 Exception class를 넣으면되용
import roomescape.reservation.dto.ReservationRequestDTO; | ||
import roomescape.reservation.dto.ReservationResponseDTO; | ||
|
||
@Data |
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.
아주 좋습니다!! 보고 공부하려는게 아주 좋은것 같아요
(개인적으로) Data 어노테이션 쓰는걸 별로 안좋아하는데요.. Setter가 꼭 필요한 상황이 아니면 안해야 도메인을 잘 지킬 수 있다고 생각해요.
물론 잘 인지하고 쓰면 상관없죠
|
||
@Data | ||
public class Reservation { | ||
private Long 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.
맞습니다. 사실 가변한 값은 다루기가 굉장히 짜증나요. 디버깅할때 값이 어디서 바뀌었는지 파악하기가 힘들거든요.
그래서 최대한 값을 불변으로 두어서 변경을 하는 범위를 최소화시켜 예측하고 통제할 수 잇는 코드를 만들 수 있습니다.
개인적으론 일단 무조건 final 붙이고 가변한 값이면 final을 떼는 식으로 저는 사용해요.
실제로 최신 프로그래밍 언어는 별다른 키워드를 안붙이면 기본적으로 불변입니다.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
커스텀 예외를 만드는 것에서 외부(프론트)에게 전달되는 목적도 있지만 내부적으로 다루기 위함도 있어요.
사실 지금 우리가 NotFound의 경우에는 별도로 예외처리를 안 하고있지만, 이게 추후에 추가될수도 있으니까요.
import java.time.LocalTime; | ||
import roomescape.reservation.service.ReservationService; | ||
|
||
class ReservationServiceTest { |
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.
예외케이스에 대한 테스트를 잘 해주셨군요...!
정상 케이스에 대한 테스트를 안하신 이유는 있을까요?
return ResponseEntity.noContent().build(); | ||
} | ||
|
||
@ExceptionHandler(IllegalArgumentException.class) |
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.
더불어 ExceptionHandler를 보통 Controller에서 정의하는 것보단 ConrollerAdvice라는 클래스를 정의해서 거기에 작업하는 경우가 많습니다.
이에 대해 한번 학습해보고 적용해보면 좋을 것 같아요
); | ||
} | ||
|
||
private boolean isDuplicate(ReservationRequest request) { |
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.
요기서 Duplicate 여부를 각각의 Date와 Time을 꺼내서 확인하고 있는데 reservation에서 isSame 같은 메서드를 만들어서 Reservation에게 값이 같은지에 대한 작업을 처리하는 책임을 주면 어떨까요?
Spring MVC
Spring MVC 미션 제출합니다!
항상 꼼꼼한 리뷰 감사합니다. 😄
질문
@ExceptionHandler
에서 IllegalArgumentException을 처리하도록 설정했고,NotFoundReservationException도 이를 상속하고 있기 때문에
모든 예외가 400 상태 코드로 응답되도록 되어 있습니다.
만약 예외에 따라 상태 코드를 다르게 나누지 않고,
모든 예외를 동일하게 400으로 처리해야 한다면,
이럴 경우 커스텀 예외를 세분화해서 나누는 것이 의미가 없는 것일까요?