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

feature-episodes-review PR #45

Merged
merged 16 commits into from
Dec 16, 2024

Conversation

dnwls16071
Copy link
Contributor

@dnwls16071 dnwls16071 commented Dec 13, 2024

추가된 기능

  • 에피소드별 리뷰를 작성(좋아요는 제외했습니다.)
  • 에피소드별 리뷰 조회

관련 이슈

Spring 3.x 버전에 따른 페이지네이션 JSON 성능 안정성 개선 링크

@dnwls16071 dnwls16071 added ✨기능 구현 기능 구현 📖 에피소드 도메인 Episodes 도메인 labels Dec 13, 2024
@dnwls16071 dnwls16071 self-assigned this Dec 13, 2024
@dnwls16071 dnwls16071 changed the base branch from main to dev December 13, 2024 01:52
@dnwls16071 dnwls16071 added the enhancement New feature or request label Dec 13, 2024
@kimunche
Copy link
Contributor

컨트롤러와 서비스에서 201 코드를 한번씩 각각 넣어주는 이유가 있을까요?
다음과 같이 책임을 분리하는게 좋을 것 같은데 어떤가요?

  • 서비스 레이어: 비즈니스 로직에 집중, HTTP나 웹과 관련된 세부 사항 모름
  • 컨트롤러 레이어: HTTP 응답에 집중, HTTP 상태 코드와 헤더 등을 관리

@dnwls16071
Copy link
Contributor Author

dnwls16071 commented Dec 13, 2024

void 타입으로 변경하여 개선하도록 하겠습니다.
공통 응답 포맷에 너무 꽂혀서 그런가 거기에 신경쓰다보니 서비스 측에선 작성할 필요가 없는 부분이었는데 잘못 작성했네요

@dnwls16071 dnwls16071 added the 👀코드리뷰 팀원 코드 리뷰 요청 label Dec 13, 2024
@kimunche
Copy link
Contributor

등록 성공시 response 를 ResponseEntity 로 변경하는게 어떨까요?? body 없이 header 에 코드값만 설정해서 내려주면 될것같습니다
BE FE response 최종 협의사항 첨부드립니다
image

@dnwls16071
Copy link
Contributor Author

dnwls16071 commented Dec 14, 2024

ResponseEntity<ApiResponse> 로 하도록 할게요.

@JoinColumn(name = "user_id", nullable = false)
private User user;

@Column(name = "rating", precision = 10, scale = 2)

Choose a reason for hiding this comment

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

별점도 필수인게 좋지 않을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

0점이 존재할 수 있다 라는 정책 설정이 먼저일것같아요

@Setter
public class EpisodeReviewRequest {

private BigDecimal rating;

Choose a reason for hiding this comment

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

rating이 BigDecimal을 써야할 만큼 민감하고 중요한 정보인지, 이로 인한 오버헤드가 더 손해일지 한번 생각하는것도 좋을 것 같아요

Copy link
Contributor

Choose a reason for hiding this comment

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

그러네요 double 사용도 고려해보아요

Choose a reason for hiding this comment

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

url이 일관되지 못한 것으로 느껴집니다.

어떤 url은 animeId가, 어떤 url은 episodeId가 사용되어 사용하는 부분에 있어 혼동이 있을 수 있을거같습니다.

private final EpisodesRepository episodesRepository;

public Page<EpisodesDto> getEpisodes(Long animeId, Pageable pageable) {
List<EpisodesDto> episodesList = queryFactory

Choose a reason for hiding this comment

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

사실 이건 스프링에 대한 지식이 조금 모자라서 그런건데

아마 문법만 보면 db에서 특정 내용을 읽어오는걸로 보이는데 따로 repo같은 곳으로 빠져야 하는거 아닌가요?

Copy link
Contributor

@kimunche kimunche Dec 14, 2024

Choose a reason for hiding this comment

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

이 부분은 저번 백엔드 회의에서도 얘기했던 부분이네요.
다시 위에 거론됐던 책임 분리 부분과 같은 문제인것 같습니다.

  • 서비스단 : 비즈니스 로직
  • 리포지토리 : db 작업
    이렇게 가는게 어떨까요?

@dnwls16071
Copy link
Contributor Author

계속 피드백 주셔서 감사합니다! 이해한 바로는 아래와 같아 해당 부분들을 수정했습니다.

근성님 : 별점 필드, URL 개선
운채님 : 레이어 간 명확한 책임 분리

@kimunche
Copy link
Contributor

피드백 반영 감사합니다! 수정된 부분확인했고, 머지해도 될 것 같습니다.

@dnwls16071 dnwls16071 merged commit 6896783 into Project-aniwhere:dev Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ✨기능 구현 기능 구현 👀코드리뷰 팀원 코드 리뷰 요청 📖 에피소드 도메인 Episodes 도메인
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature-episodes-review
3 participants