feat: 식당 조회시 북마크 수량 조회 기능 #204
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PgmJun
reviewed
Jun 21, 2024
matzip-app-external-api/src/main/java/com/woowacourse/matzip/application/RestaurantService.java
Outdated
Show resolved
Hide resolved
matzip-app-external-api/src/main/java/com/woowacourse/matzip/application/RestaurantService.java
Show resolved
Hide resolved
...app-external-api/src/test/java/com/woowacourse/matzip/application/RestaurantServiceTest.java
Outdated
Show resolved
Hide resolved
jinyoungchoi95
requested changes
Jun 24, 2024
Comment on lines
345
to
350
| for (int i = 1; i <= 3; i++) { | ||
| Member member = createTestMember("githubId" + i); | ||
| memberRepository.save(member); | ||
|
|
||
| bookmarkRepository.save(createTestBookmark(member, restaurant)); | ||
| } |
There was a problem hiding this comment.
Suggested change
| for (int i = 1; i <= 3; i++) { | |
| Member member = createTestMember("githubId" + i); | |
| memberRepository.save(member); | |
| bookmarkRepository.save(createTestBookmark(member, restaurant)); | |
| } | |
| Member member1 = memberRepository.save(createTestMember("githubId1")); | |
| bookmarkRepository.save(createTestBookmark(member1, restaurant)); | |
| Member member2 = memberRepository.save(createTestMember("githubId2")); | |
| bookmarkRepository.save(createTestBookmark(member2, restaurant)); | |
| Member member3 = memberRepository.save(createTestMember("githubId3")); | |
| bookmarkRepository.save(createTestBookmark(member3, restaurant)); |
가능하면 테스트에는 for와 같은 로직보다는 차라리 하드코딩해서 각 값을 넣어주는 것이 좋을 것 같아요.
물론 로직이 단순할 수도 있지만 1~3이라는 로직자체가들어갔다는 것은 그 구문을 이해해야하는 범위가 늘어나 직관적이지 못하고 간단하더라도 오류를 범할 수 있기 때문이에요.
There was a problem hiding this comment.
3개 정도는 하드코딩으로 넣는 것이 훨씬 직관적으로 보이네요!
| @@ -79,7 +80,8 @@ public RestaurantResponse findById(final String githubId, final Long restaurantI | |||
| .orElseThrow(RestaurantNotFoundException::new), | |||
| reviewRepository.findAverageRatingUsingRestaurantId(restaurantId) | |||
| .orElse(0.0), | |||
| isBookmarked(githubId, restaurantId)); | |||
| isBookmarked(githubId, restaurantId), | |||
| bookmarkRepository.countByRestaurantId(restaurantId)); | |||
There was a problem hiding this comment.
Suggested change
| bookmarkRepository.countByRestaurantId(restaurantId)); | |
| bookmarkRepository.countByRestaurantId(restaurantId) | |
| ); |
hyeon0208
approved these changes
Jun 25, 2024
matzip-app-external-api/src/main/java/com/woowacourse/matzip/application/RestaurantService.java
Outdated
Show resolved
Hide resolved
| @@ -79,7 +80,8 @@ public RestaurantResponse findById(final String githubId, final Long restaurantI | |||
| .orElseThrow(RestaurantNotFoundException::new), | |||
| reviewRepository.findAverageRatingUsingRestaurantId(restaurantId) | |||
| .orElse(0.0), | |||
There was a problem hiding this comment.
기존에 작성되어 있는 코드 같은데
0.0이라는 값이 직관적으로 다가오지 않다고 느껴져
이를 DEFAULT_RATING 같이 상수로 관리하는 건 어떻게 생각하시나요 ?
There was a problem hiding this comment.
기본 별점이 0.0에서 바뀌는 경우가 없을 것 같고 바로 0.0점을 확인하는 것이 더 직관적일 것 같습니다.
상수로 관리하게 되면 기본값이 0.0이 아닐 수 있나? 라는 생각에 코드 상단으로 다시 올라가야하는 불편함이 있을 것 같습니다.
...ternal-api/src/main/java/com/woowacourse/matzip/application/response/RestaurantResponse.java
Show resolved
Hide resolved
PgmJun
approved these changes
Jul 1, 2024
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
issue: #203
as-is
to-be