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

[1 - 2단계 방탈출 예약 대기] 리브(김민주) 미션 제출합니다. #13

Merged
merged 42 commits into from
May 20, 2024

Conversation

Minjoo522
Copy link
Member

@Minjoo522 Minjoo522 commented May 15, 2024

안녕하세요, 웨지!
리브라고합니다.

1 - 2 단계 구현 완료하여 PR 드립니다.
이번 미션은 페어 카피의 코드로 진행했습니다.

이번 미션 변경사항

링크😊

이전 웹 개발 경험

Django를 사용하여 로컬에서 동작하는 웹 애플리케이션을 구현해 본 경험이 있습니다.
Spring은 사용해 본 경험은 없습니다.

웹 테스팅 시 로그인 정보

Copy link

@sihyung92 sihyung92 left a comment

Choose a reason for hiding this comment

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

안녕하세요 리브! 리뷰어 웨지입니다
방탈출 예약 대기 미션 빠르게, 잘 구현해주셨네요 ㅎ 몇가지 리뷰 남겼으니 확인해주셔요!
감사합니다 🙇‍♂️

Comment on lines 20 to 21
@GetMapping("/members")
public ResponseEntity<List<MemberIdAndNameResponse>> getMembers() {

Choose a reason for hiding this comment

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

지난 미션의 범위라고 생각해서 반영하지 않으셔도 됩니다 :>

해당 API는 회원의 개인정보를 타인에게 노출할 수 있으므로 어드민 페이지와 마찬가지로 인증인가의 대상이 되어야 합니다. 개인정보 누출은 개인정보 보호법에 의거한 것 뿐만 아니더라도 서비스 브랜딩에 치명적일 수 있어 더 중요하다고 생각합니다

Copy link
Member Author

Choose a reason for hiding this comment

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

refactor: 회원 조회 관련 관리자만 접근 가능한 API로 변경

/admin으로 시작하도록 URl 변경하여 접근시 관리자임을 할 수 있도록 변경하였습니다. 😊

동일한 맥락으로 전체 예약 조회와 관리자 예약 조회도 관리자만 접근 가능해야하기에 URI 변경하였습니다.
refactor: 예약 관련 관리자 API 분리

}

@GetMapping("/reservations")
public ResponseEntity<List<ReservationResponse>> getReservations() {

Choose a reason for hiding this comment

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

API의 응답은 확장성을 고려하여 List 형태보단 ("[]" http body) Object 형식으로 응답하시길 권합니다. ("{}" http body)
즉 List를 감싼 응답객체를 하나 만들라는 리뷰인데, 이것은 API break change를 안 만드는 습관으로써 중요합니다.

최근 재밌게 본 아티클인데, 내용 중 "Don’t return arrays as top-level responses." 가 이에 해당합니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines 58 to 64
reservationStatus.getReservationStatus()
.keySet()
.stream()
.map(reservationTime -> new ReservationStatusResponse(
reservationTime,
reservationStatus.findReservationStatusBy(reservationTime))
).toList());

Choose a reason for hiding this comment

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

이 부분 독해가 상당히 힘들었는데요, 일급컬렉션에서 자료구조를 통째로 뽑아온 후, 거기서 keySet을 추출하고 맵 일급컬렉션에 key를 재요청하는 패턴이 낯설어 코드 이해에 2분 이상 소모했습니다
이렇게 사용될거라면 차라리 일급컬렉션 대신 Map인 게 더 나았을것 같아요.

그리고 현재의 ReservationStatus가 적절한 자료구조인가? 하는 생각도 들었어요.

public class ReservationStatus {

    private final ReservationTime time;
    private final boolean isBooked;

형태의 자료구조이고 List 형식을 주고 받았다면 클라이언트 코드(컨트롤러)가 훨씬 쉽게 처리할 수 있을거 같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

refactor: List를 담는 일급 객체 사용하도록 변경

이 부분은 저도 바로 이해하기 어려웠던 부분이긴합니다. 😅
하지만 이번 미션 요구사항과 관련이 없는 부분이라 변경하지 않았었습니다.
List<ReservationStatus>의 일급 객체 ReservationStatuses를 만들어 보았는데, 이렇게 변경한 후 가독성은 훨씬 나아보이는데 웨지가 보시기엔 어떠실지 궁금합니다. 😊

Choose a reason for hiding this comment

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

네 적절한 처리로 보입니다 ㅎ

import roomescape.service.dto.request.ReservationAdminSaveRequest;

@Service
public class AdminReservationCreateService {

Choose a reason for hiding this comment

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

AdminReservationCreateService / ReservationCreateService를 공통화 할 수 있는 방안을 찾아보세요.리브가 아닌 다른 개발자는 ReservationCreateService에서 예약 관련 내용을 수정할 때 Admin 쪽 코드도 똑같이 수정해야 한다는 생각을 못 합니다. (해당 미션을 수행한 지 2개월이 지난 리브도 마찬가지고요)

누군가 예약을 수정하면 매우 높은 확률로 서로 달라진 어드민 예약등록과 사용자 예약등록으로 인해 장애가 발생합니다.
물론 저희는 마감을 지켜야하고 어쩔 수 없이 중복코드를 만들어 낼 때도 많으니 개발자 개인의 잘잘못을 가릴 문제는 아니지만, 가능한 공통화 할 수 있는 부분은 공통화 해주었다면 발생하지 않았을 문제였겠죠.

Choose a reason for hiding this comment

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

리뷰 후 생각해보니 리브의 코드는 아니었겠네요 😅

다만 개인적으로 어프로브 하기 어렵다는 생각이 드는 코드여서요,
또 현업에서도 80%는 남의 코드를 수정하게 되기도 하니.. 리뷰 반영 부탁합니다요

Copy link
Member Author

@Minjoo522 Minjoo522 May 16, 2024

Choose a reason for hiding this comment

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

refactor: 예약 생성 관련 코드 하나로 통합

웨지의 리뷰 공감합니다! 😊
저도 왜 나눴는지 궁금했었는데 이전에 페어가 작성했던 코드는 admin 기능을 지원하는 서비스 레이어에서 생성할 수 없을까?라는 생각으로 시작하여 AdminReservationCreateService / ReservationCreateService로 나누게 되었다고 합니다.


공통화를 위해 관리자와 일반 회원의 예약 생성시 전달 받는 파라미터가 다르므로ReservationCreateService 한 객체 내부에서 create()메서드를 오버로딩하는 방식으로 변경했습니다.

    public Reservation create(ReservationAdminSaveRequest request) {
        Reservation reservation = request.toEntity(
                request,
                getReservationTime(request.timeId()),
                getTheme(request.themeId()),
                getMember(request.memberId()));
        return create(reservation);
    }

    public Reservation create(ReservationSaveRequest request, Member member) {
        Reservation reservation = request.toEntity(
                request,
                getReservationTime(request.timeId()),
                getTheme(request.themeId()),
                member);
        return create(reservation);
    }

    private Reservation create(Reservation request) {
        validateDateIsFuture(request.getDate(), request.getReservationTime());
        validateAlreadyBooked(request.getDate(), request.getReservationTime().getId(), request.getTheme().getId());
        return reservationRepository.save(request);
    }

다만 이 방식으로 사용하는 경우 public으로 공개된 메서드에서 리턴 값으로 create(reservatiton)을 호출하기 때문에 재귀 호출처럼 보일 수 있다는 다른 크루의 의견이 있었는데요.
그래서 다음과 같이 validate로직만 추출하는 방식도 생각해보았습니다.

    public Reservation create(ReservationAdminSaveRequest request) {
        Reservation reservation = request.toEntity(
                request,
                getReservationTime(request.timeId()),
                getTheme(request.themeId()),
                getMember(request.memberId()));
        validateReservation(reservation);
        return reservationRepository.save(reservation);
    }

    public Reservation create(ReservationSaveRequest request, Member member) {
        Reservation reservation = request.toEntity(
                request,
                getReservationTime(request.timeId()),
                getTheme(request.themeId()),
                member);
        validateReservation(reservation);
        return reservationRepository.save(reservation);
    }

    private void validateReservation(Reservation request) {
        validateDateIsFuture(request.getDate(), request.getReservationTime());
        validateAlreadyBooked(request.getDate(), request.getReservationTime().getId(), request.getTheme().getId());
    }

웨지는 어떤 방식이 더 낫다고 생각하시나요?
아니면 혹시 제가 미처 생각하지 못한 더 좋은 방식이 있을까요? ☺️

Choose a reason for hiding this comment

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

저는 1번이 더 취향이요 ㅎ 재귀처럼 느껴지는게 문제라면 메서드명을 변경해주면 된다고 생각합니당.

개인적으론 재귀를 매우 드물게 사용해서요 (저는 3년동안 2번 쓴거 같습니다) 재귀 코드엔 확실히 표지 (beacon)을 남기는 편입니다 (주석이든 메서드명이든) 그래서 제가 볼 땐 재귀보단 오버로딩의 가능성이 먼저 생각나긴 하네요

물론 애초에 메서드명을 조정하면 그럴 일이 없겠지만요

2번은 이전보단 훨씬 낫지만 여전히 휴먼에러 가능성이 남아있으니까요, 어드민 사용자간의 validation이 현재는 다를 게 없는데 섯부른 분리라고 생각합니당

Copy link
Member Author

Choose a reason for hiding this comment

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

감사합니다! 😊
메서드명 변경해주었습니다.

@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;

@ManyToOne

Choose a reason for hiding this comment

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

해당 객체 전체적으로 단방향으로 깔끔하게 묶어주셨네요 ㅎ

Comment on lines 17 to 19
private String name;
private String email;
private String password;

Choose a reason for hiding this comment

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

만약 이후 미션에서 회원 관련 로직이 추가된다면 해당 객체들도 VO로 맵핑해주면 좋겠네요~
레벨 1의 교훈들이 쭉 유지되었으면 하는 바람이 있습니다

Copy link
Member Author

Choose a reason for hiding this comment

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

refactor: Member의 name, email, password 포장

감사합니다! 반영하였습니다. 😊

Comment on lines 18 to 23
@Query("SELECT rt "
+ "FROM ReservationTime rt "
+ "INNER JOIN Reservation r "
+ "ON rt.id = r.time.id "
+ "WHERE r.date = :date "
+ "AND r.theme.id = :themeId")

Choose a reason for hiding this comment

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

Entity 구현은 ReservationTime에서 Reservation을 보지 않도록 결정하셨는데 여기선 JPQL로 join 처리하신 이유가 있을까요?
도메인 객체에서 정의하신대로 Reservation을 선 조회하신 다음 해당 예약에 묶인 Time을 찾아보는 방법도 있을텐데요.

필요한 경우에 한해 JPQL을 활용할 수 있지만 해당 방식이 전반적으로 적용된다면 JPA를 도입한 이점이 작아지기에 리브의 생각을 여쭤봅니다.

Choose a reason for hiding this comment

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

그리고 메서드명은 findReservationTimeByThemeIdAndDate 아닐까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

refactor: 예약된 테마 시간 JPQL 사용하지 않도록 변경

페어와 저도 서비스 레이어에서 처리를 해주는게 나을지 아니면 DB에서 처리된 정보를 받아오는 게 나을지 고민을 했었습니다.
이전 미션에서는 Reservation을 먼저 조회 후 서비스 레이어에서 처리를 해주는 방식을 선택했었습니다.
이번에 페어와 방식을 고민하면서 굳이 Reservation을 조회 후 또 다시 ReservationTime을 조회하기 위해 두 번의 쿼리를 날릴 필요가 있는가?라는 생각을 했었습니다.
추가로 기존 페어 코드가 join하여 가져오는 방식으로 구현되어 있어서 그 방식을 따르기도 했습니다. 😅

하지만,JPA 자체를 접한 것이 처음이라 어떻게하면 JPA의 장점을 최대한 이용할 수 있을까?에 대한 고민까지는 하지 못했던 것 같습니다. 😭

이 로직의 경우 Reservation을 전부 가져온다고 하여도 특정 날짜, 특정 테마에 관련한 정보만 가져오고 ReservationTime또한 개수에 한계가 있기 때문에 전부 가져와도 괜찮겠다는 판단을 내려 웨지가 리뷰 남겨주신 방향대로 변경하였습니다. 😊

Choose a reason for hiding this comment

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

넵 ㅎ findAll은 실제론 금기기술이지만 테이블 별로 한계 사이즈 등을 고려해서 코딩하는 건 좋은 습관입니다.

Choose a reason for hiding this comment

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

그리고 1번 날릴걸 2번 날린다고 해서 생각보다 성능에 영향을 주지 않습니다. N+1 처럼 지수적으로 던지는게 아니라면요.
각 도메인 별로 잘 분리되있는 쿼리는 가독성과 유지보수성이 좋을 뿐만 아니라 장기적으로 유연한 아키텍처를 지원합니다. DB 성능이냐 코드의 유지보수성이냐는 이런 경우엔 대치되면서 둘 다 포기할 수 없는 가치이므로 항상 중간지점을 잘 타색해야 합니다.

Comment on lines 97 to 99
@DeleteMapping("/reservations/{id}")
public ResponseEntity<Void> deleteReservation(@PathVariable
@Positive(message = "1 이상의 값만 입력해주세요.") long id) {

Choose a reason for hiding this comment

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

각각의 엔티티 ID를 VO화 하실 것이 아니라면 (ex. ReservationId) long 타입의 id 변수명은 reservationId 등으로 분명하게 명명해주세요.

Copy link
Member Author

Choose a reason for hiding this comment

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

refactor: 변수명 명확하게 변경

감사합니다!
반영하였습니다. 😊

import roomescape.service.dto.response.MemberIdAndNameResponse;
import roomescape.util.TokenGenerator;

@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT)

Choose a reason for hiding this comment

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

보통 RANDOM_PORT를 많이 활용하는 편인데요, 로컬 서버가 떠있으면 테스트가 실패하기도 하고 포트에 따라 동작이 달라지는 경우는 거의 없어 테스트 대상으로 보지 않기도 하기 때문입니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

refactor: SpringBootTest random port 사용 및 추상 클래스로 중복 제거

RANDOM_PORT로 변경하였습니다.
변경하면서 중복되는 부분을 제거하기 위해 추상 클래스를 사용하였습니다. 😊

import org.junit.jupiter.api.Test;
import roomescape.service.dto.request.ReservationSaveRequest;

class ReservationSaveRequestTest {

Choose a reason for hiding this comment

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

dto 생성 테스트 까진 잘 안해서 신기하기도 하면서, 또 의미가 있는 테스트인지 궁금했어요.
왜냐하면 not null validate를 스프링이 돌려주니까, 실패 케이스를 정의하는 게 단위테스트로는 불가능하니까요
(지우라는 건 절대 아니고 혼자 중얼중얼 한 겁니다 ^_^;;)

Copy link
Member Author

Choose a reason for hiding this comment

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

사실 저도 dto 생성 테스트는 안 하는 편입니다 😅
불필요한 테스트가 있는 것도 비용이라고 생각해서 삭제했습니다.

@Minjoo522
Copy link
Member Author

안녕하세요 웨지!
리뷰 주신 내용 반영하여 코멘트 달아두었습니다.
감사합니다. ☺️

Copy link

@sihyung92 sihyung92 left a comment

Choose a reason for hiding this comment

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

안녕하세요 리브! 리뷰어 웨지입니다.
리뷰 반영 잘 해주셨어요 👍
앞선 리뷰 땐 다루지 않은 오류 응답과 관련된 부분을 리뷰했으니 확인해주셔요.
감사합니다!

}

@ExceptionHandler(AuthenticationException.class)
public ResponseEntity<String> handleAuthenticationException(AuthenticationException ex) {

Choose a reason for hiding this comment

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

오류 응답으로 String, 즉 Http Response body에 문자열을 내리고 있는데요.
문자열 응답은 확장성이 떨어져 사용하지 않는 패턴입니다

예외 객체도 하나 생성해서 균일한 오류응답을 클라이언트가 받을 수 있도록 수정해볼까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines 21 to 23
return new ResponseEntity<>(
ex.getBindingResult().getFieldErrors().get(0).getDefaultMessage(),
HttpStatus.BAD_REQUEST);

Choose a reason for hiding this comment

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

기존 valid는 모든 필드에 대해 검증을 해주는데, 오히려 커스텀 과정에서 기능이 약화되서 아쉬워요.

Copy link
Member Author

Choose a reason for hiding this comment

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

refactor: 예외 응답 객체 사용 및 커스텀 애플리케이션 예외 추상 클래스 생성

이 커밋에서 모든 필드 이름과 예외 메세지를 받아 Map으로 저장하여 발생한 모든 예외 메세지를 응답하도록 변경하였습니다. 😊


@ExceptionHandler(value = IllegalArgumentException.class)
ResponseEntity<String> handleIllegalArgumentException(IllegalArgumentException ex) {
return ResponseEntity.badRequest().body(ex.getMessage());

Choose a reason for hiding this comment

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

IllegalArgumentException의 예외를 catch해버리는 경우 스프링이 의존한 수많은 라이브러리 중 특정한 라이브러리가 응답한 예외의 메시지가 출력될 수 있습니다.
이러면 서버 스택을 외부에 공개하게 되기 때문에, 보안 취약점으로 동작할 수 있어요.
(서버가 어떤 언어 스택이냐, 어떤 라이브러리를 의존 중이냐, 프레임워크는 무엇을 쓰느냐 등이 알려지는 것도 취약점입니다)

그래서 자바 SDK가 제공하는 흔히 쓰는 예외들 (IllegalArgument, NoSuchElement 등등) 을 그대로 사용하기 보단 한번 감싼 커스텀 예외로 사용하는 것이 일반적입니다.

해당 핸들러는 커스텀 예외 처리기로 바꾸고, "IllegalArgumentException"는 RuntimeException handler에서 처리해도록 해보시죠.

Copy link
Member Author

Choose a reason for hiding this comment

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

refactor: IllegalArgumentException -> 커스텀 예외로 변경

InvalidRequestException 커스텀 예외 생성하여 사용하도록 변경하였습니다! 😊
현재는 커스텀 예외가 한가지밖에 없지만 추후 다른 커스텀 예외가 생길 때 ExceptionHandler에 계속 정의할 필요 없도록 RoomescapeException이라는 추상 클래스 생성하여 커스텀 예외가 상속 받고 ExceptionHandler에서 RoomescapeException을 처리하도록 구현했습니다.


@DeleteMapping("/reservations/{reservationId}")
public ResponseEntity<Void> deleteReservation(@PathVariable
@Positive(message = "1 이상의 값만 입력해주세요.") long reservationId) {

Choose a reason for hiding this comment

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

@positive의 메시지가 반복되고 있어서요, 공통화 하는 방안으로 CustomValidator가 생각났는데 Positive 로직은 그대로 태울순 없을까 싶어서 gpt랑 잠깐 같이 짜봤는데요

@Positive(message = "1 이상의 값만 입력해주세요.")
@Constraint(validatedBy = {})
@Documented
@Target({ METHOD, FIELD, ANNOTATION_TYPE, CONSTRUCTOR, PARAMETER, TYPE_USE })
@Retention(RUNTIME)
public @interface IdPosivice {
  String message() default "";
  Class<?>[] groups() default { };
  Class<? extends Payload>[] payload() default { };
}

이런 식으로 객체 생성하고 여기 붙여주면 메시지 공통화도 가능하네요.
이게 왜 동작하는지는 GPT 선생님한테 자세히 설명 들었는데 첨부하진 않을게요. 저는 GPT를 대행자 보다는 학습도우미로 활용하는데, '해달라' 그러면 불만족스러운 결과를 자주 내놓는데 비해 설명해달라는 제가 만난 어떤 스승 보다 쉽게 설명해줘요. 우테코 크루들도 잘 활용했으면 좋겠네요.

Copy link
Member Author

Choose a reason for hiding this comment

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

refactor: id 유효성 검사 커스텀 어노테이션 생성 및 사용

GPT 사용 꿀 팁🍯 알려주셔서 감사합니다. 😆🍀
CustomValidator를 만드는 건 생각지도 못한 방향이었는데 중복을 줄일 수 있어서 훨씬 좋은 것 같습니다!
혹시 다른 메세지를 출력해야 할 때도 한 곳만 수정하면 되구요! 😊

해당 어노테이션은 현재 api 컨트롤러에서만 사용하기 때문에 패키지 위치를 controller > api > validator에 위치시켰는데 이 부분을 웨지는 어떻게 생각하시는지 궁금합니다.

Choose a reason for hiding this comment

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

리뷰로 남겼습니다!

import roomescape.service.dto.request.ReservationSaveRequest;

@Service
public class ReservationCreateService {

Choose a reason for hiding this comment

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

조회 / 삭제 / 등록 서비스를 모두 각각 쪼개주셨네요. 어떤 이점이 있을까요? 매번 서비스를 만들 때마다 쪼개주어야 할까요? (수정 서비스도 생길까요?)

Copy link
Member Author

Choose a reason for hiding this comment

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

사실 기존 제 코드에서는 조회 / 삭제 / 등록 서비스를 ReservationService 한 곳에서 구현했습니다. 😊

현재 규모에서는 굳이 나누지 않아도 될 것 같다고 생각합니다.
한 컨트롤러에서 조회 / 삭제 / 등록과 관련된 서비스 의존성을 다 따로 받아와야 하기 때문에 조금 번거롭다고 느껴지기도 했습니다. 😅

하지만 프로젝트 규모가 커진다면 쪼개는 게 좋다고 생각합니다.
예를 들어 현재는 ReservationFindService에 모든 예약 가져오기, 검색, 한 유저의 예약들 가져오기 세 가지 로직만 있지만,
요구 사항이 더 많아진다면 한 서비스에 메서드가 많아지게 되고 결국 유지보수 시 어려움을 가져올 가능성이 있다고 생각합니다.

현재 구조에서는 일관성을 위해 수정 서비스도 필요하게 된다면 따로 만들어 주는게 좋다고 생각합니다. 😊

Choose a reason for hiding this comment

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

넵 케바케로 생각해주세요 ㅎ
find 1개, save 1개, update 1개 씩 있는데 굳이 쪼개놓을 필요가 있나 싶고요
말씀주신 것처럼 너무 복잡해지면 쪼갤 맛도 날거고요

command와 query 수준으로 분리하는건 자주 봤습니다.

import roomescape.domain.Reservation;
import roomescape.repository.ReservationRepository;

@Service

Choose a reason for hiding this comment

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

쪼개주신김에 Trsactional도 학습해서 각각의 서비스에 적용해볼까요? ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

refactor: @transactional 사용

생성 / 삭제 서비스에 적용하였습니다. 😊

현재 로직에서 다른 테이블을 수정한 뒤 또 다른 테이블에 행을 추가하거나 하는 로직은 없어서 롤백이 필요하지는 않습니다.
하지만 한가지 서비스 처리에 대한 데이터베이스 접근을 한 트랜잭션에서 관리하면 데이터베이스 작업이 일관적이고 효율적이라고 생각해서 적용해주었습니다. 😆

Comment on lines 58 to 64
reservationStatus.getReservationStatus()
.keySet()
.stream()
.map(reservationTime -> new ReservationStatusResponse(
reservationTime,
reservationStatus.findReservationStatusBy(reservationTime))
).toList());

Choose a reason for hiding this comment

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

네 적절한 처리로 보입니다 ㅎ

import roomescape.service.dto.request.ReservationAdminSaveRequest;

@Service
public class AdminReservationCreateService {

Choose a reason for hiding this comment

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

저는 1번이 더 취향이요 ㅎ 재귀처럼 느껴지는게 문제라면 메서드명을 변경해주면 된다고 생각합니당.

개인적으론 재귀를 매우 드물게 사용해서요 (저는 3년동안 2번 쓴거 같습니다) 재귀 코드엔 확실히 표지 (beacon)을 남기는 편입니다 (주석이든 메서드명이든) 그래서 제가 볼 땐 재귀보단 오버로딩의 가능성이 먼저 생각나긴 하네요

물론 애초에 메서드명을 조정하면 그럴 일이 없겠지만요

2번은 이전보단 훨씬 낫지만 여전히 휴먼에러 가능성이 남아있으니까요, 어드민 사용자간의 validation이 현재는 다를 게 없는데 섯부른 분리라고 생각합니당

Comment on lines 18 to 23
@Query("SELECT rt "
+ "FROM ReservationTime rt "
+ "INNER JOIN Reservation r "
+ "ON rt.id = r.time.id "
+ "WHERE r.date = :date "
+ "AND r.theme.id = :themeId")

Choose a reason for hiding this comment

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

넵 ㅎ findAll은 실제론 금기기술이지만 테이블 별로 한계 사이즈 등을 고려해서 코딩하는 건 좋은 습관입니다.

Comment on lines 18 to 23
@Query("SELECT rt "
+ "FROM ReservationTime rt "
+ "INNER JOIN Reservation r "
+ "ON rt.id = r.time.id "
+ "WHERE r.date = :date "
+ "AND r.theme.id = :themeId")

Choose a reason for hiding this comment

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

그리고 1번 날릴걸 2번 날린다고 해서 생각보다 성능에 영향을 주지 않습니다. N+1 처럼 지수적으로 던지는게 아니라면요.
각 도메인 별로 잘 분리되있는 쿼리는 가독성과 유지보수성이 좋을 뿐만 아니라 장기적으로 유연한 아키텍처를 지원합니다. DB 성능이냐 코드의 유지보수성이냐는 이런 경우엔 대치되면서 둘 다 포기할 수 없는 가치이므로 항상 중간지점을 잘 타색해야 합니다.

@Minjoo522
Copy link
Member Author

안녕하세요, 웨지!

리뷰 주신 내용 반영하여 코멘트 남겨두었습니다. 😊
감사합니다!

Copy link

@sihyung92 sihyung92 left a comment

Choose a reason for hiding this comment

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

안녕하세요 리브! 리뷰어 웨지입니다
리뷰 반영 잘 해주셨네요! 이만 머지하겠습니다 🙏
다음 step에서 뵈어요~~

@@ -0,0 +1,24 @@
package roomescape.controller.api.validator;

Choose a reason for hiding this comment

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

dto에 붙이는 거니까 dto와 같은 뎁스에 유지해주면 괜찮지 않나 생각합니다 ㅎ

import java.lang.annotation.Documented;
import java.lang.annotation.Retention;
import java.lang.annotation.Target;

Choose a reason for hiding this comment

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

그리고 해당 객체는 코드만 보면 왜 존재하는지 알기 어려워요. 직관적인 패턴은 아니니까요

이런 코드는 주석을 첨가해서 실제로 어떤 역할을 수행하는지 알려주면 좋겠습니다.

@sihyung92 sihyung92 merged commit 7dac10c into woowacourse:minjoo522 May 20, 2024
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