Add(.remind): 피드백 가능한 계획 목록 조회 API 구현 #199
Open
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
Test Coverage Report
|
Hejow
reviewed
Mar 17, 2024
src/main/java/me/ajaja/module/feedback/mapper/FeedbackMapper.java
Outdated
Show resolved
Hide resolved
src/main/java/me/ajaja/module/feedback/application/FindUpdatableTargetService.java
Outdated
Show resolved
Hide resolved
src/main/java/me/ajaja/module/plan/adapter/out/persistence/FindUpdatableTargetsAdapter.java
Outdated
Show resolved
Hide resolved
JuwoongKim
reviewed
Mar 19, 2024
src/main/java/me/ajaja/module/feedback/application/FindUpdatableTargetService.java
Outdated
Show resolved
Hide resolved
src/main/java/me/ajaja/module/plan/adapter/out/persistence/FindUpdatableTargetsAdapter.java
Outdated
Show resolved
Hide resolved
src/main/java/me/ajaja/module/plan/adapter/out/persistence/FindUpdatableTargetsAdapter.java
Outdated
Show resolved
Hide resolved
Test Coverage Report
|
Hejow
reviewed
Mar 20, 2024
src/main/java/me/ajaja/module/feedback/application/LoadEvaluableFeedbacksService.java
Outdated
Show resolved
Hide resolved
| @@ -46,4 +50,14 @@ public AjajaResponse<FeedbackResponse.FeedbackInfo> getFeedbackInfo(@PathVariabl | |||
| FeedbackResponse.FeedbackInfo feedbackInfo = loadFeedbackInfoService.loadFeedbackInfoByPlanId(userId, planId); | |||
| return AjajaResponse.ok(feedbackInfo); | |||
| } | |||
|
|
|||
| @Authorization | |||
| @GetMapping("/updatable") | |||
There was a problem hiding this comment.
하 뭔가 좋은 엔드포인트가 있을 것 같은데... 고민되네요
그냥 컨트롤 URI로 쓰는것도 괜찮을 것 같은데 어떤가요??
// 전
curl -X GET https://api.ajaja.me/feedbacks/updatable
// 후
curl -X GET https://api.ajaja.me/feedbacks/추천좀요
그리고 변수명 수정하면서 엔드포인트는 수정 안되었군요!? ㅋㅋ
There was a problem hiding this comment.
6b96c34
"평가 가능한 피드백" 을 나타낼 수 있는게 뭔가 고민해보았는데 흠흠
평가 + able + 복수형 = evaluables 로 해보았는데 어떤가유 ㅋㅋ
이번꺼 네이밍이 전반적으로 애매하네요..
Test Coverage Report
|
Hejow
reviewed
Mar 23, 2024
Comment on lines
+15
to
+24
| @Getter | ||
| @RequiredArgsConstructor | ||
| private enum RemindTime { | ||
| MORNING(9), | ||
| AFTERNOON(13), | ||
| EVENING(22); | ||
|
|
||
| private final int time; | ||
| } | ||
|
|
There was a problem hiding this comment.
이렇게 내부에서 쓰도록 변경했다면, 다음과 같이 불필요한 코드라인 수를 줄일 수 있을 것 같아요!
또한 외부에서 안쓰이니까 접근지정자를 개선할 수 있을 것 같구요!
public int getRemindTime() {
// before
return remindTime.getTime();
// after
return remindTime.time;
}
Comment on lines
+154
to
+155
| return this.messages.stream().anyMatch(message -> now.isWithinAMonth( | ||
| message.parsePeriod(this.createdAt.getYear(), this.getRemindTime()) |
There was a problem hiding this comment.
가독성이 떨어져서 라인을 좀 바꿀까요?? 아니면 저처럼 map 쓰셔도 괜찮구!
|
|
||
| @Override | ||
| public List<UpdatableFeedback> findEvaluableTargetsByUserId(Long userId) { | ||
| BaseTime now = BaseTime.now(); |
There was a problem hiding this comment.
method 네이밍이 괜찮아서 바로 사용해도 괜찮을 것 같아요!
Comment on lines
+24
to
+32
| public List<Plan> findEvaluablePlansByUserIdAndTime(Long id, BaseTime now) { | ||
| return queryFactory.selectFrom(planEntity) | ||
| .where(planEntity.userId.eq(id) | ||
| .and(planEntity.createdAt.year().eq(now.getYear()))) | ||
| .fetch() | ||
| .stream() | ||
| .map(mapper::toDomain) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
네이밍과 쿼리 사이 연관성이 없는 것 같아요..!
그리고 querydsl을 이렇게 쓸거면 JPA로만 처리할 수 있을 것 같아요!
Comment on lines
+40
to
+46
| List<FeedbackEntity> entities = queryFactory.selectFrom(feedbackEntity) | ||
| .where(feedbackEntity.userId.eq(userId) | ||
| .and(feedbackEntity.createdAt.year().eq(BaseTime.now().getYear()))) | ||
| .orderBy(feedbackEntity.planId.asc()) | ||
| .fetch(); | ||
|
|
||
| return mapper.toDomain(entities); |
There was a problem hiding this comment.
다른 곳에서는 stream()으로 잘 넘기시는데 여기서는 달라서 코드에 통일성이 없어 보여요!
아마 원인이 mapper의 문제인 것 같은데 toDomain()으로 넘기는 것과 달리 결과는 List로 받으니 mapper 네이밍의 문제인 것 같습니다!
Hejow
reviewed
Mar 23, 2024
| public List<Feedback> findAllFeedbacksByUserId(Long userId) { | ||
| List<FeedbackEntity> entities = queryFactory.selectFrom(feedbackEntity) | ||
| .where(feedbackEntity.userId.eq(userId) | ||
| .and(feedbackEntity.createdAt.year().eq(BaseTime.now().getYear()))) |
There was a problem hiding this comment.
최저수준에서 직접적으로 application 코드를 사용하라는 의도는 아니였습니다!
repository의 재사용성을 고려해서 시간은 외부에서 받도록!!
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.
🔍 어떤 PR인가요?
😋 To Reviewer
✅ 작성한 테스트