Skip to content

Conversation

idealHyun
Copy link

Spring MVC

Spring MVC 미션 제출합니다!
항상 꼼꼼한 리뷰 감사합니다. 😄

질문

  • 현재 @ExceptionHandler에서 IllegalArgumentException을 처리하도록 설정했고,
    NotFoundReservationException도 이를 상속하고 있기 때문에
    모든 예외가 400 상태 코드로 응답되도록 되어 있습니다.
    만약 예외에 따라 상태 코드를 다르게 나누지 않고,
    모든 예외를 동일하게 400으로 처리해야 한다면,
    이럴 경우 커스텀 예외를 세분화해서 나누는 것이 의미가 없는 것일까요?

this.time = time;
}

public static Reservation from(ReservationRequestDTO dto, Long id) {

Choose a reason for hiding this comment

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

Reservation이라는 domain이 ReservationDto에 의존하고 있군요.

상현님이 생각하기엔 DTO는 어떤 역할인가요?

Copy link
Author

Choose a reason for hiding this comment

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

DTO 클라이언트측에서 api를 통해 요청했을 때 필요한 내용만 보내어 내부 구조를 막는 느낌으로 알고 있었습니다.
Reservation 내부에 코드를 넣을 것이아니라 외부에서 Reservation에서 필요한 값만 꺼내서 담아서 보내주는식으로 고쳐야할 것 같습니다.

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(

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 {

Choose a reason for hiding this comment

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

reservation 패키지와 home 패키지를 나눴군요. 둘은 어떤 차이인가요?

Copy link
Author

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);
Copy link

@hong-sile hong-sile Jun 9, 2025

Choose a reason for hiding this comment

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

AtomicLong은 어떤 역할을 하고, 왜 쓰였나요?

Copy link
Author

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();

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) {

Choose a reason for hiding this comment

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

요런것들을 한번 테스트코드로 확인해보면 좋을 거 같아요.
아니면 이러한 테스트는 필요없을까요? MissionStepTest로 원하는 케이스를 모두 확인했으니까요

Copy link
Author

Choose a reason for hiding this comment

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

같은 날짜, 같은 시각 예약들어오는 부분도 예외 처리를 안해서 그부분까지 관련해서 테스트 코드를 작성했습니다.
이런 도메인의 유효성 관련 코드는 필요하다고 생각합니다.
그런데 api와 관련된 코드들은 주어진 테스트 코드처럼 테스트를 하는건가요?

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)
Copy link

@hong-sile hong-sile Jun 9, 2025

Choose a reason for hiding this comment

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

ExceptionHandler가 IllegalArgumentException을 받도록 하였군요.

별도로 정의한 NotFound가 아닌 Illegal로하신 이유가 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

지금 코드에서 모든 예외가 400으로 처리되는데 NotFound 예외도 같은 400이라 굳이 나눌 필요가 있을까? 라는 생각이 들었습니다.

Choose a reason for hiding this comment

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

알고계시다면 좋습니다. 더 세분화할 필요가 있을땐 따로 더 메서드를 만들고 별도의 Exception class를 넣으면되용

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

Choose a reason for hiding this comment

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

DataAnnotation은 어떤 역할을 해주나요?

Copy link
Author

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 은 잘 몰랐는데 이부분도 공부해야될 것 같습니다.

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;
Copy link

@hong-sile hong-sile Jun 9, 2025

Choose a reason for hiding this comment

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

요거는 한번 이야기했어야 했는데 조금 늦었군요.

혹시 자바의 final 키워드에 대해 아시나요? 변수에 대해서 reassign 을 막는 키워드인데, 객체의 불변을 유지하기에 좋습니다.

이 키워드를 이용해서 우리는 immutable하게 값을 사용할 수 있습니다. 최근 개발 패러다임에선 이런 immutable한 값을 사용하는게 선호되고 있는데 왜 그런걸까요?

Copy link
Author

Choose a reason for hiding this comment

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

변하지 않는다는 것을 가정하에 코드를 보고 유지보수하게되면 코드의 결과나 에러 예측 등이 훨씬 쉬워질 것 같습니다.

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 {

Choose a reason for hiding this comment

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

별도로 NotFoundException을 정의해주신 이유는 무엇인가요?

Copy link
Author

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와 관련된 예약이 없어서인지 알려주기 위해서일까요?

Choose a reason for hiding this comment

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

커스텀 예외를 만드는 것에서 외부(프론트)에게 전달되는 목적도 있지만 내부적으로 다루기 위함도 있어요.

사실 지금 우리가 NotFound의 경우에는 별도로 예외처리를 안 하고있지만, 이게 추후에 추가될수도 있으니까요.

Copy link

@hong-sile hong-sile left a comment

Choose a reason for hiding this comment

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

기본적으로 웹 환경에 대한 기본이 좋네요.

몇몇 부분 읽었고 리뷰 남겼습니다. 한번 보고 답변해주세요 늘 그렇듯 반박, 의문은 환영입니다

@hong-sile
Copy link

현재 @ExceptionHandler에서 IllegalArgumentException을 처리하도록 설정했고, NotFoundReservationException도 이를 상속하고 있기 때문에 모든 예외가 400 상태 코드로 응답되도록 되어 있습니다. 만약 예외에 따라 상태 코드를 다르게 나누지 않고, 모든 예외를 동일하게 400으로 처리해야 한다면, 이럴 경우 커스텀 예외를 세분화해서 나누는 것이 의미가 없는 것일까요?

모든 예외를 동일하게 400 코드로 처리한다고 가정한다해도, 구분을 하려면 아마 지금처럼 에러메시지 기준으로 나누는 거겠죠?
어쩃든 클라이언트는 이걸 보고 핸들링을 해야하니까요. -> 요게 중요합니다. 400에러가 고대로 유저에게 나가기도 하지만, 클라이언트 즉 프론트엔드는 이걸 보고 핸들링을 하기도 합니다.

물론 프론트엔드는 서버가 전달한 예외 메시지를 기반으로 동작을 제어할 수 있습니다. 그러나 다음과 같은 문제점이 있습니다:
• 에러 메시지는 변경에 취약합니다. 문자열 기반 로직은 유지보수가 어렵고 버그 발생 가능성이 높습니다.
• 다국어 지원 문제: 서비스가 다국어(예: 영어, 한국어)를 지원할 경우, 메시지는 번역될 수 있으므로 프론트에서 어떤 언어로 온 메시지를 기준으로 로직을 작성해야 할지 모호해집니다.
• 디자인 변경에 취약: 메시지 포맷이 바뀌거나 개발자 간 표현 방식이 달라져도 프론트 코드가 깨질 수 있습니다.

따라서 에러 메시지를 기반으로 핸들링하는 것은 추천하지 않는 방식입니다. 그래서 다른 방법을 선택해요.

많은 기업에서는 HTTP status code만으로는 복잡한 에러 케이스들을 구분할 수 없기 때문에, 별도의 에러 코드 체계를 도입합니다.
예약 요청 유효성 실패(1001)
존재하지 않는 사용자(1002)
중복된 닉네임(1003)
요런 식으로요.

이런식으로 각 예외마다 디테일한 정보를 넣어야 하기 때문에 커스텀 예외를 나누는 건 의미가 있습니다. 각 커스텀 예외 클래스는 더 많은 정보를 잘 표현할 수 있으니까요.
물론 지금처럼 예외케이스가 많지 않으면 이렇게 안해도 되지만 커지게 되면 커스텀 예외를 만들고 핸들링하는게 더 의미가 있어집니다.

Comment on lines 31 to 35
public void deleteReservation(Long id) {
Reservation target = reservations.stream()
.filter(reservation -> reservation.getId().equals(id))
.findFirst()
.orElseThrow(() -> new NotFoundReservationException("해당 ID의 예약이 존재하지 않습니다: "));

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)안에 찾게하려면 어떻게 해야할까요?

Copy link
Author

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

Choose a reason for hiding this comment

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

Service 클래스를 별도로 작성해주셨군요. Service는 어떤 역할을 해주나요?

Copy link
Author

Choose a reason for hiding this comment

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

이때까지 공부했던 Service는 비즈니스 로직을 처리한다고 알고 있습니다.
DB와 컨트롤러에서 넘어온 값들을 받아서 무언가를 처리하는 존재로만 알고 있었습니다.
계산하고 확인하고 무언갈 저장, 삭제, 수정 등을 하는 것 같습니다.

Copy link

@hong-sile hong-sile left a 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) {

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) {

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)

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

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;

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 {

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 {

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)

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) {

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에게 값이 같은지에 대한 작업을 처리하는 책임을 주면 어떨까요?

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