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

[3 - 4단계 방탈출 예약 대기] 카피(김상혁) 미션 제출합니다. #107

Merged
merged 47 commits into from
May 27, 2024

Conversation

tkdgur0906
Copy link
Member

@tkdgur0906 tkdgur0906 commented May 22, 2024

안녕하세요 영이~ 카피입니다!
3 ~ 4단계에서도 잘 부탁드립니다!
궁금한 점은 리뷰 반영하면서 질문드리겠습니다!
감사합니다😀

구현 방법

예약

  • 특정 날짜, 테마, 시간에 예약이 이미 되어있는 경우 예약 불가

예약 대기

  • 특정 날짜, 테마, 시간에 이미 예약한 경우 예약 대기 불가
  • 특정 날짜, 테마, 시간에 이미 예약 대기한 경우 중복으로 예약 대기 불가

예약 대기 승인

  • 예약 취소가 발생하는 경우 예약 대기가 있을 때 우선순위에 따라 자동으로 예약

이전의 웹 개발 경험

스프링을 통한 프로젝트 개발 경험이 있습니다.
JPA도 사용해본 적이 있지만 흐릿한 기억으로 남아있습니다.
AWS를 통해 배포해본 적이 있습니다.

웹 테스팅 시 로그인 정보

일반 회원


관리자

Copy link

@choijy1705 choijy1705 left a comment

Choose a reason for hiding this comment

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

안녕하세요 카피
미션 잘 진행해주셨어요👍
컨벤션이 일관되지 않고 가독성이 조금 안좋은 방향인것 같아서 몇가지 코멘트 남겼는데
전체적으로 한번 고민해봐주시면 좋을것 같아요!

@@ -51,9 +53,9 @@ public ResponseEntity<List<ReservationResponse>> getReservations() {

@GetMapping("/reservations-mine")
public ResponseEntity<List<UserReservationResponse>> getUserReservations(@AuthenticatedMember Member member) {
List<Reservation> userReservations = reservationFindService.findUserReservations(member.getId());
List<ReservationWaitingWithRank> reservationWaitingWithRanks = reservationFindService.findMemberReservations(member.getId());

Choose a reason for hiding this comment

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

한줄에 120자는 넘지 않도록 해주세요!
다른곳들도 챙겨봐주세요 많은 곳들이 120자를 넘는것 같네요

Copy link
Member Author

Choose a reason for hiding this comment

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

구현에만 급급해서 가독성 측면에서 많이 부족했던 것 같습니다😅
전체적으로 수정해보았는데 다시 한번 확인해주실 수 있을까요?
파라미터 여러개 존재 하는 경우, 120자가 넘어가는 경우에 줄바꿈을 해주었습니다!

@PostMapping("/waiting")
public ResponseEntity<ReservationResponse> addWaiting(@RequestBody @Valid ReservationSaveRequest request,
@AuthenticatedMember Member member) {
Reservation newReservation = reservationCreateService.createReservation(request, member, ReservationStatus.WAITING);

Choose a reason for hiding this comment

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

service 를 create, delete, find 로 나누시게 된 이유가 어떤걸까요??

Copy link
Member Author

Choose a reason for hiding this comment

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

service 하나에서 모든 행동을 수행한다면 service가 너무 뚱뚱해지고 유지보수도 어려울 것이라 생각했습니다!
CRD로 나눔으로써 관리할 포인트를 세분화할 수 있을 것 같습니다.
현재 프로젝트는 크지 않아 비교적 필요성이 적을 수 있지만, 적용해본다는 측면해서 나누어 보았습니다!

Choose a reason for hiding this comment

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

범용적인 방법은 아니기 때문에 다른 사람들과 함께 작업을 한다면
굉장히 불편할것 같습니다🤔
read db와 write db가 다르게 관리되는경우 CQRS로 관리하는 경우는 있지만 어디까지나 read db, write db가 분리되는 상황에서나 적용할법한 내묭일것 같습니다

Copy link
Member Author

@tkdgur0906 tkdgur0906 May 26, 2024

Choose a reason for hiding this comment

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

일반적으로 사용되는 패턴이 아니기 때문에 팀원들에게 이해와 설명이 필요할 것 같습니다.
말씀하신 CQRS를 알아보았는데, 이 패턴이 결국 프로젝트가 커졌을 때 command와 query로 서비스를 분리하고 db를 추가하면서 부하도 줄일 수 있는 방법이겠네요!

CRD로 나누지 않고 하나의 클래스로 통일했습니다!

public abstract class BaseTime {

@CreatedDate
private LocalDateTime createdAt;

Choose a reason for hiding this comment

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

LocalDateTime 으로 시간을 관리하면
여러 국가에서 사용하면 어떤 문제가 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

LocalDateTime은 타임존을 반영하지 않기 때문에 특정 지역의 시간대를 나타낼 수 없습니다.
그래서 ZoneDateTime이라는 타임존에 따라 다른 시간을 나타내는 클래스가 있다고 합니다.

그런데 LocalDateTime이든 ZoneDateTime이든 결국 시간을 관리하기 위해서는 db에 저장을 해야할 것 같습니다.
그렇다고 여러 국가의 타임존마다 시간을 모두 저장하는 것은 좋은 방법이 아닌 것 같습니다.

그래서 현재 생각나는 방식은 db에 UTC를 기준으로 저장하고, 클라이언트의 요청에 저장된 타임존 정보를 db에 저장되어있는 시간에 반영하여 반환해주는 방식을 생각해보았습니다.

실제로는 어떤 방식으로 관리를 하는지 궁금합니다!

Choose a reason for hiding this comment

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

Instant 타입에 대해서 학습해보면 좋을것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

Instant 타입은 UTC 기반의 절대 시간을 나타내고, 타임존 정보가 없습니다.
따라서 Instant 타입으로 시간을 저장하고, 요청하는 사용자의 타임존에 맞게 변경하여 보여주면 여러 국가에서 사용할 수 있을 것 같습니다!

Comment on lines 43 to 44
@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.

Suggested change
@PathVariable
@Positive(message = "1 이상의 값만 입력해주세요.") long id) {
@PathVariable long id) {

Positive 는 동작안할것 같네요

Copy link
Member Author

@tkdgur0906 tkdgur0906 May 23, 2024

Choose a reason for hiding this comment

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

클래스 레벨에 @Validated를 안붙여서 동작 안한다고 말씀하신 건가요?
아니면 어플리케이션 플로우에서 일어나지 않는다는 것을 의미하신건가요?

Choose a reason for hiding this comment

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

애플리케이션 플로우에서 검증이 동작할 일이 없을것 같습니다
명시적으로 적는것도 id라 표현할 필요가 없을것 같고요
불필요한 어노테이션일것 같다는 뜻으로 남겼어요🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

애플리케이션 플로우에서 동작할 일이 없다는 것은 이해를 했습니다!

다만 궁금한 것은 postman을 통해 해당 delete 메서드의 path의 id에 명시적으로 0을 입력한 경우 @Positive가 동작하는 것을 확인했습니다. 물론 어노테이션이 없더라도 서비스의 검증 로직에서 잡히긴 합니다.
이런 상황을 검증하는 것에 대한 불필요성 인 것 인가요?

그리고 ReservationTimeApiControllergetReservationTimesIsBooked 메서드에서 @Positive로 requestParam으로 들어오는 id를 검증하고 있습니다. delete 메서드의 id 검증처럼 get 메서드에서 사용되는 id의 검증도 마찬가지로 불필요하다고 생각하시나요?

Choose a reason for hiding this comment

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

제가 해봤을때는 500에러가 나는데 "1이상의 값만 입력해주세요" 라는 에러가 뜨는건가요?
이 에러는 실제 사용자가 아닌 클라이언트 개발자가 잘못 입력했기때문에 (너무 말도 안되는 실수기도 하죠)
구체적인 에러 메세지를 담아서 처리할 필요는 없을것 같고요!

this.reservationDeleteService = reservationDeleteService;
}

@GetMapping("/admin/waitings")

Choose a reason for hiding this comment

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

이 클래스는 watings 인데 다른 클래스는 waiting이네요!
통일하는게 좋을것 같습니다

Copy link
Member Author

@tkdgur0906 tkdgur0906 May 23, 2024

Choose a reason for hiding this comment

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

기존의 뼈대 코드에서 view를 반환하는 컨트롤러의 엔드포인트가 단수여서, api 컨트롤러는 복수로 만들어 놓았습니다.
저도 통일하는 것이 좋다는 것에 동의하였고, 리소스이기 때문에 복수로 표현되어야한다고 생각하여 모두 복수로 통일하였습니다!

하지만 view 랜더링하는 컨트롤러의 엔드포인트와 api의 엔드포인트가 동일하게 되어, 둘을 명확히 구분하여 호출하고 중복을 피하기 위해 맨앞에 /api를 붙여 구분하게 하였습니다!

}
return new ReservationStatus(reservationStatus);
public boolean isReserved() {
return RESERVED.equals(this);

Choose a reason for hiding this comment

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

Suggested change
return RESERVED.equals(this);
return this == RESERVED;

enum 은 equals 비교를 사용안할 것 같네요!

reservation.getDate(),
reservation.getReservationTime().getStartAt(),
"예약");
public UserReservationResponse(ReservationWaitingWithRank reservationWaitingWithRank) {

Choose a reason for hiding this comment

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

User와 Member는 다른건가요?
같은 도메인이라면 네이밍 통일이 필요해보입니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

현재 도메인을 Member로 해놨기 때문에 네이밍을 Member로 통일해 주었습니다!

Comment on lines 36 to 37
public Reservation(Long id, Member member, LocalDate date, ReservationTime reservationTime,
Theme theme, ReservationStatus reservationStatus) {

Choose a reason for hiding this comment

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

Suggested change
public Reservation(Long id, Member member, LocalDate date, ReservationTime reservationTime,
Theme theme, ReservationStatus reservationStatus) {
public Reservation(
Long id,
Member member,
LocalDate date,
ReservationTime reservationTime,
Theme theme,
ReservationStatus reservationStatus
) {

지금 방법은 어떤 파라미터가 있는지 파악하기 힘든것 같아요ㅠ
파라미터가 여러개라서 120자를 넘어간다면 각 파라미터마다 개행을 하는걸 추천드립니다

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 +73 to +76
Optional<Reservation> findNextWaiting(@Param("theme") Theme theme,
@Param("reservationTime") ReservationTime reservationTime,
@Param("date") LocalDate date,
Limit limit);

Choose a reason for hiding this comment

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

41L 과 개행하는 방식이 다른데 통일하면 좋을것 같아요!
사소해 보이지만 이런 부분이 여러사람들과 작업을하게 되면 큰 혼란을 줄 수 있습니다 😭

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 +73 to +76
Optional<Reservation> findNextWaiting(@Param("theme") Theme theme,
@Param("reservationTime") ReservationTime reservationTime,
@Param("date") LocalDate date,
Limit limit);

Choose a reason for hiding this comment

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

Limit은 어떻게 동작하는건가요?

Copy link
Member Author

@tkdgur0906 tkdgur0906 May 24, 2024

Choose a reason for hiding this comment

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

Limit 타입은 spring data jpa 3.2 버전에서 추가되었고 쿼리에서 반환하는 결과의 수를 제한하기 위해 사용됩니다!
Limit.of(int max) 형태로 사용되고 max에 원하는 결과의 최대 개수를 넣을 수 있습니다.

쿼리가 어떻게 나가는지 로그를 통해 확인해 보았습니다.

offset ? rows fetch first ? rows only

offset은 0, rows는 1로 바인딩되어 나가는 것을 확인했습니다.

다만 offset 설정을 임의로 할 수 없기 때문에 offset이 필요한 경우에는 Pageable을 사용하면 될 것 같습니다!

Copy link

@choijy1705 choijy1705 left a comment

Choose a reason for hiding this comment

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

카피 피드백 잘 반영해주셨네요!
몇가지 추가 코멘트 남겼으니
확인해보시고 반영부탁드려요!

public abstract class BaseTime {

@CreatedDate
private LocalDateTime createdAt;

Choose a reason for hiding this comment

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

Instant 타입에 대해서 학습해보면 좋을것 같습니다!

Comment on lines 43 to 44
@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라 표현할 필요가 없을것 같고요
불필요한 어노테이션일것 같다는 뜻으로 남겼어요🤔

@PostMapping("/waiting")
public ResponseEntity<ReservationResponse> addWaiting(@RequestBody @Valid ReservationSaveRequest request,
@AuthenticatedMember Member member) {
Reservation newReservation = reservationCreateService.createReservation(request, member, ReservationStatus.WAITING);

Choose a reason for hiding this comment

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

범용적인 방법은 아니기 때문에 다른 사람들과 함께 작업을 한다면
굉장히 불편할것 같습니다🤔
read db와 write db가 다르게 관리되는경우 CQRS로 관리하는 경우는 있지만 어디까지나 read db, write db가 분리되는 상황에서나 적용할법한 내묭일것 같습니다

Reservation newReservation = reservationCreateService.createReservation(
request,
member,
ReservationStatus.WAITING

Choose a reason for hiding this comment

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

request에 status를 받는다면 이 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.

현재 status 외에 중복이기 때문에 클라이언트에서 해당 status 값을 request로 넘겨주는 방식으로 변경했습니다!

Comment on lines 47 to 54
@DeleteMapping("/api/waitings/{id}")
public ResponseEntity<Void> deleteWaiting(@AuthenticatedMember Member member,
@PathVariable
@Positive(message = "1 이상의 값만 입력해주세요.")
long id) {
reservationDeleteService.deleteReservation(id, member);
return ResponseEntity.noContent().build();
}

Choose a reason for hiding this comment

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

wating이라는 도메인은 존재하지 않고 reservation으로 충분히 처리할 수 있을것 같아요
reservationController에서 전체적으로 처리되면 사용성이 똑같은 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.

현재 Waiting이라는 엔티티가 존재하지 않기 때문에 엔드포인트에서 리소스로 관리하지 않도록 변경했습니다.
waiting api의 대부분이 reservations와 동일한 프로세스로 진행되기 때문에 하나의 예약 api에서 처리할 수 있도록 반영했습니다!

import java.util.List;

@RestController
public class AdminWaitingApiController {

Choose a reason for hiding this comment

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

여기도 AdminReservationApiController로 처리할 수 있지 않을까요?


@GetMapping("/api/admin/waitings")
public ResponseEntity<List<WaitingResponse>> getWaiting(@AuthenticatedMember Member member) {
List<Reservation> waitings = reservationFindService.findWaitings();

Choose a reason for hiding this comment

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

findByStatus(status) 형태로
처리하면 더 유연할것 같네요!

Comment on lines 27 to 28
return reservedTimes.stream()
.anyMatch(reservedTime -> reservedTime.equals(reservationTime));

Choose a reason for hiding this comment

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

contains로 충분히 처리 가능하지 않을까요??

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 36 to 39
WHEN COUNT(r) > 0
THEN true
ELSE false END
FROM Reservation r

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.

case 문을 들여쓰기 해서 가독성을 높였습니다!

@@ -22,13 +22,16 @@
<div class="collapse navbar-collapse" id="navbarSupportedContent">
<ul class="navbar-nav ml-auto">
<li class="nav-item">
<a class="nav-link" href="/admin/reservation">Reservation</a>
<a class="nav-link" href="/admin/reservations">Reservation</a>

Choose a reason for hiding this comment

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

여기 내용은 아니고 reservation-mine.html 에서 reservation 버튼을 눌렀는데
홈화면으로 가는건 좀 어색한것 같네요!

Copy link
Member Author

Choose a reason for hiding this comment

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

html 수정하여 바로 예약 페이지로 가도록 적용했습니다!

Copy link

@choijy1705 choijy1705 left a comment

Choose a reason for hiding this comment

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

카피 피드백 잘 반영해주셨습니다👍
validator클래스의 필요성에 대해서 코멘트 남겼는데 한번 고민해보시면 좋을것 같아요
이번 단계는 여기서 머지하겠습니다
다음 미션도 화이팅입니다!

Comment on lines +25 to +26
public ResponseEntity<MemberIdAndNameResponse> signup(@RequestBody @Valid
SignupRequest request) {

Choose a reason for hiding this comment

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

Suggested change
public ResponseEntity<MemberIdAndNameResponse> signup(@RequestBody @Valid
SignupRequest request) {
public ResponseEntity<MemberIdAndNameResponse> signup(
@RequestBody @Valid SignupRequest request
) {

파라미터는 어노테이션을 개행하면 어디에 적용하는지 쉽게 안보여서 한줄로 처리할 수 있도록 해주는게 좋을것 같아요!

public ResponseEntity<ReservationResponse> addReservationByAdmin(@RequestBody @Valid ReservationAdminSaveRequest request) {
Reservation newReservation = adminReservationCreateService.createReservation(request);
return ResponseEntity.created(URI.create("/admin/reservations/" + newReservation.getId()))
@GetMapping("/waiting-list")

Choose a reason for hiding this comment

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

"/api/admin/reservations?status=wating"
이 되도록 처리하는게 일반적으로 사용하는 방법일것 같아요!

Comment on lines 43 to 44
@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.

제가 해봤을때는 500에러가 나는데 "1이상의 값만 입력해주세요" 라는 에러가 뜨는건가요?
이 에러는 실제 사용자가 아닌 클라이언트 개발자가 잘못 입력했기때문에 (너무 말도 안되는 실수기도 하죠)
구체적인 에러 메세지를 담아서 처리할 필요는 없을것 같고요!

);
}

public List<ReservationWaitingWithRank> findMemberReservations(long memberId) {

Choose a reason for hiding this comment

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

ByMember로 되어야할것 같아요!

return reservationRepository.findReservationWaitingWithRankByMemberId(memberId);
}

public List<Reservation> findByReservationStatus(ReservationStatus reservationStatus) {

Choose a reason for hiding this comment

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

여긴 byStatus

ReservationTime reservationTime =
reservationCreateValidator.getValidReservationTime(request.timeId());
reservationCreateValidator.validateDateIsFuture(request.date(), reservationTime);
Theme theme = reservationCreateValidator.getValidTheme(request.themeId());

Choose a reason for hiding this comment

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

validator에서 theme을 반환하는건 어색하네요!

import roomescape.domain.ReservationTime;
import roomescape.domain.Theme;
import roomescape.repository.MemberRepository;
import roomescape.repository.ReservationRepository;
import roomescape.repository.ReservationTimeRepository;
import roomescape.repository.ThemeRepository;

import java.time.LocalDate;
import java.time.LocalDateTime;

@Component
public class ReservationCreateValidator {

Choose a reason for hiding this comment

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

이 클래스가 필요한가요??
오히려 가독성을 해치고 있는것 같아요

@choijy1705 choijy1705 merged commit 15a18e3 into woowacourse:tkdgur0906 May 27, 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.

2 participants