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

Feature/2단계 예외처리및리팩터링 #109

Open
wants to merge 12 commits into
base: koreacat
Choose a base branch
from

Conversation

koreacat
Copy link

@koreacat koreacat commented Jul 8, 2024

No description provided.

Copy link

@NewWisdom NewWisdom left a comment

Choose a reason for hiding this comment

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

안녕하세요 경민님~!
2단계 요구사항 잘 반영해서 미션 진행해주셨네요 👍
몇 가지 코멘트 남겼는데 확인 부탁드릴게요 :)
그럼 마지막까지 화이팅입니다!

import java.util.Objects;

@Repository
public class ReservationRepo {

Choose a reason for hiding this comment

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

ReservationRepository 에서 ReservationRepo 와 같이 약어로 변경한 이유가 있으신가요~?
개인적으로 네이밍에 약어를 쓰면 다른 사람과 함께 해당 프로젝트를 볼 때,
어떤 기준에서 약어를 쓸지 어떤 식의 약어로 변경해야할지 모호함이 생길 수 있어 지양하게 되는 것 같아요 ㅎㅎㅎ

@@ -17,6 +17,7 @@ dependencies {
implementation 'org.springframework.boot:spring-boot-starter-web'
implementation 'org.springframework.boot:spring-boot-starter-thymeleaf'
implementation 'org.springframework.boot:spring-boot-starter-jdbc'
implementation 'org.springframework.boot:spring-boot-starter-validation'

Choose a reason for hiding this comment

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

spring validation을 적용해주셨군요 👍

public class ReservationRq {
@NotEmpty(message = "Reservation name must not be empty")

Choose a reason for hiding this comment

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

문자열 검증에는 @NotNull, @NotEmpty, @NotBlank` 를 사용할 수 있는데,
각 어노테이션의 차이는 무엇일까요~?

import org.springframework.http.HttpStatus;
import org.springframework.web.bind.annotation.ResponseStatus;

@ResponseStatus(HttpStatus.NOT_FOUND)

Choose a reason for hiding this comment

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

실제로 ResourceNotFoundException 이 발생했을 때는 500 에러가 발생하며 @ResponseStatus 을 통해 정의한 404 에러로 핸들링이 되지 않고 있네요!
spring의 ExceptionResolver 의 우선순위에 대해 확인해보면 원인을 알 수 있을 것 같습니다 ㅎㅎ

import org.springframework.web.bind.annotation.ResponseStatus;

@ResponseStatus(HttpStatus.BAD_REQUEST)
public class InvalidReservationException extends RuntimeException {

Choose a reason for hiding this comment

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

해당 exception 은 사용되지 않고 있네요!

String sql = "SELECT COUNT(*) FROM reservation WHERE date = ? AND time_id = ? AND theme_id = ?";
try {
Integer count = jdbcTemplate.queryForObject(sql, Integer.class, date, timeId, themeId);
return count != null && count > 0;

Choose a reason for hiding this comment

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

queryForObject 의 반환값은 항상 non-null 한 값을 반환할 것 같아요!

import org.springframework.web.bind.annotation.ResponseStatus;

@ResponseStatus(HttpStatus.CONFLICT)
public class DuplicateReservationException extends RuntimeException {

Choose a reason for hiding this comment

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

현재 중복된 예약 등 비즈니스 에러가 발생했을 때 아래처럼 정리되지 않은 예외 메시지가 사용자에게 노출되고 있는데,
사용자에게 프레임워크에서 발생하는 오류 메시지는 숨기고, 조금 더 친절한 에외 메시지를 전달하도록 하는 것은 어떨까요~?
image

ReservationRs reservationRs = reservationService.addReservation(reservationRq);
return ResponseEntity.ok().body(reservationRs);
return ResponseEntity.created(URI.create("/reservations/" + reservationRs.getId())).body(reservationRs);

Choose a reason for hiding this comment

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

Http Status Code 를 적절히 잘 활용해주셨네요 👍

Long id = reservationTimeRepository.save(reservationTimeRq);
checkForDuplicateReservationTime(reservationTimeRq.getStartAt());

Long id = reservationTimeRepo.save(reservationTimeRq);
return new ReservationTimeRs(id, reservationTimeRq.getStartAt());
}

public void deleteReservationTime(Long id) {

Choose a reason for hiding this comment

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

예약이 존재하는 예약 시간을 삭제하면, ON DELETE SET NULL 조건 때문에 reservation의 time_id 가 null이 되고
이때 예약을 조회할 시 조회되지 않는 예약 시간 때문에 500 에러가 발생하네요!
이렇게 예측할 수 없는 예외를 발생 시키기보다, 예약 시간을 삭제할 때 애플리케이션에서 적절한 유효성 검증을 진행해주는 것은 어떨까요~?

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.

3 participants