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] - 여행 계획 작성 API 구현 #50

Merged
merged 7 commits into from
Jul 18, 2024
Merged

Conversation

eunjungL
Copy link

✅ 작업 내용

  • TravelPlan 관련된 Entity 추가
  • 여행 계획 작성 API

📸 스크린샷

🙈 참고 사항

@eunjungL eunjungL added the BE label Jul 17, 2024
@eunjungL eunjungL added this to the sprint 2 milestone Jul 17, 2024
@eunjungL eunjungL requested review from hangillee and nak-honest July 17, 2024 09:21
@eunjungL eunjungL self-assigned this Jul 17, 2024
@eunjungL eunjungL linked an issue Jul 17, 2024 that may be closed by this pull request
2 tasks
Copy link

github-actions bot commented Jul 17, 2024

Unit Test Results

0 tests   0 ✔️  0s ⏱️
0 suites  0 💤
0 files    0

Results for commit 8f88297.

♻️ This comment has been updated with latest results.

Copy link

@hangillee hangillee left a comment

Choose a reason for hiding this comment

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

굿! Bean Validation과 적절한 Swagger 설정을 통해 API가 구현된 것 같습니다.

Apporve하겠습니다!

Copy link

@Libienz Libienz 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 +3 to +7
public class BadRequestException extends RuntimeException {

public BadRequestException(String message) {
super(message);
}
Copy link

Choose a reason for hiding this comment

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

cause와 message를 함께 받는 생성자도 열어두심이 어떨까요?
필요할 때 열 수도 있을 것 같지만 stack trace 타는 것이 예외의 기본적인 성질이라고 생각해서요..!

Copy link
Author

Choose a reason for hiding this comment

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

BadRequestException은 개발자가 직접 던지는 거라 cause를 추적할 일이 별로 없을 거라 생각합니다!
필요할 때 여는 방향으로 갈게용. 좋은 의견 👍

Comment on lines +36 to +40

public void validateStartDate() {
if (startDate.isBefore(LocalDate.now())) {
throw new BadRequestException("지난 날짜에 대한 계획은 작성할 수 없습니다.");
}
Copy link

Choose a reason for hiding this comment

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

검증이 도메인 내에서 실행되고 있지 않고 서비스에서 호출되고 있군요.
검증 로직 자체가 서비스에 있다면 메서드의 이름과 특성을 바꾸는 것은 어떤가요.

  • ex ) boolean isPastTravelPlan()

이를 기반으로 서비스에서 반환값을 확인하고 예외 상황을 컨트롤 하는 것이 좋아보여요
서비스에서 검증 로직을 확인하려면 도메인으로 이동해야 하니 로직이 한 눈에 들어오지 않을 것 같습니다.
이름 상 도메인 내에서 검증을 하는 것인지 헷갈릴 것 같기도 하고요!

Copy link
Author

Choose a reason for hiding this comment

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

현구막이 미션 진행 중에 달아줬던 리뷰가 있었는데요, 서비스에서 요구사항이 변경됐을 때(Ex. 지난 날짜에 대한 정책이 지난 7일전과 같이 구체화 된다거나..) 가장 먼저 열어볼 곳은 domain이라는 리뷰가 있었습니다. 또한 평문적으로 읽었을 때도 travelPlan의 시작 날짜를 검증하는구나가 자연스럽게 읽히기 때문에 위의 형식을 사용했습니다. 좋은 제안 감사해용 리비~

Copy link
Member

@nak-honest nak-honest left a comment

Choose a reason for hiding this comment

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

사소한 부분밖에 리뷰가 없어서, 바로 approve 드립니다!!
고생 많았어요 응로버~~~~~~~~~~~

@PostMapping
public ResponseEntity<TravelPlanCreateResponse> createTravelPlan(@Valid @RequestBody TravelPlanCreateRequest request) {
TravelPlanCreateResponse data = travelPlanService.createTravelPlan(request);
return ResponseEntity.ok().body(data);
Copy link
Member

Choose a reason for hiding this comment

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

한줄에는 점 하나만 있으면 좋을 것 같습니닷!!!

Copy link
Author

Choose a reason for hiding this comment

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

반영 완료!

@Min(value = 0, message = "날짜는 1 이상이어야 합니다.")
int day,
@Schema(description = "여행 장소 정보")
@NotNull(message = "여행 장소 정보는 비어있을 수 없습니다.") List<PlanPlaceCreateRequest> places
Copy link
Member

Choose a reason for hiding this comment

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

PlanPlaceCreateRequest의 order 값들이 연속되지 않았는지 검증하는 로직이 추가되면 좋을 것 같습니다!
예를들어 1, 3, 4, 5 로 들어오는 경우요!

Copy link
Author

Choose a reason for hiding this comment

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

지금 order의 경우 값 자체가 중요하기 보다는 순서 보장을 위해 사용되고 있어서 연속된 숫자 검증에 대한 필요성이 잘 느껴지지 않습니다! 들어올 때마다 반복문을 사용해서 검증하는 만큼 리소스가 들텐데 최적화할 수 있는 방법을 좀 더 생각해보겠습니다 👍

@eunjungL eunjungL requested a review from Libienz July 18, 2024 02:39
@eunjungL eunjungL merged commit a12eb33 into develop/be Jul 18, 2024
3 checks passed
@eunjungL eunjungL deleted the feature/be/#46 branch July 18, 2024 03:19
eunjungL added a commit that referenced this pull request Jul 30, 2024
* feat: TravelPlan 관련 Entity 추가

* feat: 여행기 작성 API 구현

* feat: swagger 설명 추가

* feat: 여행 계획 작성 시 지난 날짜에 대한 검증 추가

* refactor: TravelPlanService method 분리

* refactor: PlanPlaceRequest에 toPlace 추가

* style: TravelPlanControllere 개행 정리
hangillee pushed a commit to hangillee/2024-touroot that referenced this pull request Aug 20, 2024
* feat: TravelPlan 관련 Entity 추가

* feat: 여행기 작성 API 구현

* feat: swagger 설명 추가

* feat: 여행 계획 작성 시 지난 날짜에 대한 검증 추가

* refactor: TravelPlanService method 분리

* refactor: PlanPlaceRequest에 toPlace 추가

* style: TravelPlanControllere 개행 정리
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Feature] - 여행 계획 작성 API 구현
4 participants