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

[BE] feat: 축제 상세 정보 조회 Api 구현(#119) #169

Merged
merged 7 commits into from
Aug 2, 2023
Merged

Conversation

BGuga
Copy link
Member

@BGuga BGuga commented Aug 1, 2023

📌 관련 이슈

✨ PR 세부 내용

무대 정보는 무대 시작 시간 기준으로 정렬되도록 했습니다.

조회시 편의를 위해 Stage에 @OneToMany List<Ticket>를 추가했습니다.
(OneToMany관계를 걸지 않으면, for문을 돌면서 복잡한 연산을 수행해야하고, 이에 따라 DTO로 변환하는 과정이 상당히 복잡해집니다.)

@BGuga BGuga linked an issue Aug 1, 2023 that may be closed by this pull request
@github-actions
Copy link

github-actions bot commented Aug 1, 2023

Unit Test Results

79 tests  +4   79 ✔️ +4   2s ⏱️ ±0s
29 suites +1     0 💤 ±0 
29 files   +1     0 ±0 

Results for commit 0f14bcc. ± Comparison against base commit b7fb9be.

♻️ This comment has been updated with latest results.

@xxeol2 xxeol2 added BE 백엔드에 관련된 작업 USER labels Aug 1, 2023
Copy link
Collaborator

@seokjin8678 seokjin8678 left a comment

Choose a reason for hiding this comment

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

간단한 리뷰 남겼습니다!

Festival festival = festivalRepository.findById(festivalId)
.orElseThrow(() -> new NotFoundException(ErrorCode.FESTIVAL_NOT_FOUND));
List<Stage> stages = stageRepository.findAllByFestival(festival).stream()
.sorted(Comparator.comparing(Stage::getStartTime))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comparator 클래스는 static import를 하면 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

좋은 것 같습니다!

List<FestivalDetailTicketResponse> tickets) {

public static FestivalDetailStageResponse from(Stage stage) {
List<FestivalDetailTicketResponse> tickets = stage.getTickets().stream()
Copy link
Collaborator

Choose a reason for hiding this comment

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

stage.getTickets()를 호출하는 과정에서, SELECT 쿼리가 날아갈 것 같은데, 해당 Stage를 찾아올 때 fetch join으로 한 번에 찾아올 수 있으면 좋겠네요!

Copy link
Member Author

Choose a reason for hiding this comment

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

두 부분 적용했습니다!

Integer remainAmount) {

public static FestivalDetailTicketResponse from(Ticket ticket) {
TicketAmount ticketAmount = ticket.getTicketAmount();
Copy link
Collaborator

Choose a reason for hiding this comment

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

마찬가지로, 해당 TicketAmount는 EAGER로 조회되니까 이 부분도 fetch join으로 한 번에 들고올 수 있으면 좋겠네요!

Comment on lines +8 to +13
public record FestivalDetailResponse(Long id,
String name,
LocalDate startDate,
LocalDate endDate,
String thumbnail,
List<FestivalDetailStageResponse> stages) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

저희 이 부분 컨벤션을 논의해야 할 것 같네요!

Suggested change
public record FestivalDetailResponse(Long id,
String name,
LocalDate startDate,
LocalDate endDate,
String thumbnail,
List<FestivalDetailStageResponse> stages) {
public record FestivalDetailResponse(
Long id,
String name,
LocalDate startDate,
LocalDate endDate,
String thumbnail,
List<FestivalDetailStageResponse> stages) {

저는 이런 표현이 더 가독성이 높다고 생각이 들어서요!

Copy link
Member Author

Choose a reason for hiding this comment

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

Id 가 한 줄로 붙어 있으니까 1번의 첫 번째 인자가 잘 안보이는 부분이 있긴하네요

Copy link
Member

@xxeol2 xxeol2 Aug 1, 2023

Choose a reason for hiding this comment

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

조만간 회의를 잡아서 위 이슈에서 처리하면 좋을 것 같아요.
현재 정의되어있는 응답 객체들도 제각기라서, 한 번에 통일하는게 좋아보여요!

@Test
void 무대_시작시간순으로_정렬() {
// given
long festivalId = 1L;
Copy link
Collaborator

Choose a reason for hiding this comment

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

69번째 라인은 reference 타입인데, 해당 부분은 primitive 타입이네요!

Copy link
Member Author

Choose a reason for hiding this comment

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

reference 타입으로 통일했습니다!

Copy link
Collaborator

@seokjin8678 seokjin8678 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!

@@ -28,6 +31,9 @@ public class Stage {
@ManyToOne(fetch = FetchType.LAZY)
private Festival festival;

@OneToMany(mappedBy = "stage", fetch = FetchType.LAZY)
private List<Ticket> tickets = new ArrayList<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

지금 당장은 아니지만 결국 view를 위한 의존관계니 나중엔 아예 DTO로 repository에서 조회하는 것도 같이 고려해볼 수 있을 것 같아요.

Copy link
Member

Choose a reason for hiding this comment

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

인정합니다..
조회 dto들이 다소 복잡한데, 나중에 조회 / 명령 서비스 분리하고 조회 서비스에서는 dto로 바로 꺼내오는거 고려해보는거 좋을 것 같아요

@BGuga BGuga merged commit df07d02 into dev Aug 2, 2023
3 checks passed
@BGuga BGuga deleted the feat/#119 branch August 2, 2023 01:28
seokjin8678 pushed a commit that referenced this pull request Aug 2, 2023
* feat: 축제 상세 정보 조회 응답 dto 정의

* feat: 축제 상세 조회 기능 구현

* feat: 축제 상세 조회 Api 구현

* test: 축제 상세 조회 기능 통합 테스트

* feat: 축제 상세 정보 조회시 무대 시작 시간 기반으로 무대 정보 정렬

* style: 변수명 타입 통일화

* refactor: 무대 상세 조회 fetch join 적용
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE 백엔드에 관련된 작업 USER
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BE] 축제 상세 정보 조회 API를 구축한다.
4 participants