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

refactor(thumbsup): thumbsup 도메인 리팩토링 #97

Merged
merged 9 commits into from
Apr 30, 2024
Merged

Conversation

jacobhboy
Copy link
Member

No description provided.

@jacobhboy jacobhboy linked an issue Apr 24, 2024 that may be closed by this pull request
@jacobhboy jacobhboy self-assigned this Apr 24, 2024
@jacobhboy jacobhboy requested a review from qlido April 24, 2024 13:00
qlido
qlido previously approved these changes Apr 30, 2024
Copy link
Member

@qlido qlido left a comment

Choose a reason for hiding this comment

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

수고하셨습니다.

private final QueryAuthService queryAuthService;

@LoginRequired
@PostMapping("/create")
Copy link
Member

Choose a reason for hiding this comment

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

create를 지우는 편이 더 restful한거 같습니다!!

Copy link
Member Author

Choose a reason for hiding this comment

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

헉 맞네요 고맙습니다

.toList();
}

@GetMapping("/{title}")
Copy link
Member

Choose a reason for hiding this comment

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

docs에 관련된 조회 api에서 docsId와 docsTitle 2가지를 사용하는데 통일 하는건 어떨까요?

Copy link
Member

Choose a reason for hiding this comment

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

그리고 2가지 api를 통합하는건 어떤가요?

Copy link
Member Author

@jacobhboy jacobhboy Apr 30, 2024

Choose a reason for hiding this comment

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

통일감을 주는 건 좋을 것 같아요! 그러나 이 부분은 제가 생각이 조금 다른게,

  1. 글 제목이 api의 path에 스트링이 오는 것보단, 숫자가 오는게 에러 확률이 더 적고,
  2. 조회에서 더 빠르고,
  3. 이미 프론트 백엔드 코드가 완성이 되어있기 때문에
    유지하는게 더 좋은 것 같습니다!! 어떻게 생각하시나요?

@qlido
Copy link
Member

qlido commented Apr 30, 2024

@nested 했을 때 ascii only규칙이 적용되어서 checkstyle에러뜨네여;;

Copy link
Contributor

@hw9402 hw9402 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 7 to 9
public DocsThumbsUpResponseDto(Long thumbsUpsCount) {
public ThumbsUpCountResponseDto(Long thumbsUpsCount) {
this.thumbsUpsCount = thumbsUpsCount;
}
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
Member Author

Choose a reason for hiding this comment

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

아니요! class에서 record로 전환하면서 생긴 코드인 것 같습니다. 지우겠습니다!

NameIsUser06
NameIsUser06 previously approved these changes Apr 30, 2024
Copy link
Member

@NameIsUser06 NameIsUser06 left a comment

Choose a reason for hiding this comment

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

테스트 코드에서 Assertions static import 정도만 수정하면 좋을 것 같아요. 수고하셨습니당

@jacobhboy jacobhboy dismissed stale reviews from NameIsUser06 and qlido via db88cc8 April 30, 2024 11:02
@jacobhboy jacobhboy merged commit feb1291 into master Apr 30, 2024
0 of 2 checks passed
@jacobhboy jacobhboy deleted the refactor/#93 branch April 30, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor(thumbsup): thumbsup 도메인 리팩토링
4 participants