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

♻ add field to MainRecipeResponse #315

Merged
merged 5 commits into from
Aug 17, 2024
Merged

♻ add field to MainRecipeResponse #315

merged 5 commits into from
Aug 17, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 16, 2024

MainRecipeResponse에 레시피가 조회자(로그인된 사용자)가 작성한 글인지 여부를 나타내는 필드 mine을 추가했습니다.

이 과정에서 현재 미사용중이면서 DTO 변경에 영항받는 readRecipesOfCategory, readRecipesOfUser 메서드도 삭제했습니다.

@github-actions github-actions bot added BE Backend ♻️ refactor refactor labels Aug 16, 2024
@oshyun00 oshyun00 linked an issue Aug 16, 2024 that may be closed by this pull request
Copy link
Contributor Author

Overall Project 91.99% 🍏
Files changed 100% 🍏

File Coverage
MainRecipeResponse.java 100% 🍏
RecipeService.java 100% 🍏
RecipeController.java 100% 🍏

RecipeDataResponse firstResponse = groupedResponses.getFirst();
boolean mine = firstResponse.authorId() == userInfo.getId();

Choose a reason for hiding this comment

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

이 부분은 이전에 아토가 얘기한대로 User나 UserInfo에서 확인하도록 해도 좋을 것 같습니다.
(도메인 담당이 다르고 향후 리팩토링할 부분인 것 같아 일단 두었습니다!)

Copy link
Contributor Author

Overall Project 91.99% 🍏
Files changed 100% 🍏

File Coverage
MainRecipeResponse.java 100% 🍏
RecipeService.java 100% 🍏
RecipeController.java 100% 🍏

Copy link
Contributor

@hyxrxn hyxrxn left a comment

Choose a reason for hiding this comment

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

좋습니다!
리뷰 하나는 메서드들때문에 너무 무거워져서... 역할에 대해 생각해봐야 할 것 같아서 달았습니다!
카테고리 검색은 옮겨간 끝에 결국 사라졌군요.
좋은 경험이었읍니다,,,

.sorted(Comparator.comparing(MainRecipeResponse::recipeId).reversed())
.collect(Collectors.toList());
}

private MainRecipeResponse getMainRecipeResponse(List<RecipeDataResponse> groupedResponses) {
private MainRecipeResponse getMainRecipeResponse(UserInfo userInfo, List<RecipeDataResponse> groupedResponses) {
Copy link
Contributor

Choose a reason for hiding this comment

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

MainRecipeResponse 생성자에서 userInfogroupedResponses 를 파라미터로 받아서 this로 생성해보는 건 어떨까요?

static 메서드가 이제는 필요한 것 같기도 하네요....
다음 오프라인 코드리뷰 때 얘기 한 번 해보시죠!

Choose a reason for hiding this comment

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

서비스 클래스에서 DTO로 변경하는 과정이 과하게 느껴지기는 하네요.
팀 컨벤션 정했을 때 static 메서드는 최대한 쓰지 않는 것으로 얘기했던 것 같아 그 부분은 아직 적용하지 않고,
대신 userInfo와 groupedResponses를 가공한 데이터를 파라미터로 하는 MainRecipeResponse 생성자를 추가했습니다.
static메서드를 쓰게 되면 DTO 변환들이 좀 더 깔끔해질 것 같아요!👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

오오 이정도도 많이 보기 좋아졌다고 생각합니다!!! 👍

Copy link
Contributor Author

Overall Project 92.05% 🍏
Files changed 100% 🍏

File Coverage
MainRecipeResponse.java 100% 🍏
RecipeService.java 100% 🍏
RecipeController.java 100% 🍏

Copy link
Contributor Author

Overall Project 92.05% 🍏
Files changed 100% 🍏

File Coverage
MainRecipeResponse.java 100% 🍏
RecipeService.java 100% 🍏
RecipeController.java 100% 🍏

Copy link
Contributor

@geoje geoje left a comment

Choose a reason for hiding this comment

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

mine 반영 고생하셨습니다!
필드 하나가 추가되었을 뿐인데 역시 이에따른 많은 file change 가 있군요...
연관된 파일들이 침범하지 않고 딱 서비스와 테스트 영역까지 영향이 있으니 적당하다고 생각됩니다!🙌

@oshyun00 oshyun00 merged commit ee73797 into be/dev Aug 17, 2024
1 check passed
@oshyun00 oshyun00 deleted the be/feat/314 branch August 17, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE Backend ♻️ refactor refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

♻ add field to MainRecipeResponse
3 participants