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

feat: implement getSectorRatio API #22

Merged
merged 31 commits into from
Feb 13, 2024
Merged

feat: implement getSectorRatio API #22

merged 31 commits into from
Feb 13, 2024

Conversation

songyi00
Copy link
Member

@songyi00 songyi00 commented Feb 8, 2024

Issue Number

close: #19

작업 내역

구현 내용 및 작업 했던 내역

  • swagger 설정
  • 포트폴리오 내 섹터 비중 조회 API 구현
  • 테스트 코드 작성 (통합, 단위)

변경사항

  • RestAssured 의존성 추가
  • JPA 의존성 추가
  • spring-validation 의존성 추가
  • domain testFixture 의존성 추가
  • springdoc 의존성 추가

PR 특이 사항

PR을 볼 때 주의깊게 봐야하거나 말하고 싶은 점

  • 예외 처리를 어디까지 해야할 지 고민이 되더라구요! 최대한 유연하게 구현하긴 했는데 혹시 필요한 예외처리가 보이면 말씀 부탁드려요 :>
  • BaseEntity 에서 idequals, hashcode를 제거하고 각각 엔티티에서 관리하는게 더 좋을 것 같아요ㅠㅠ 테스트 코드 작성시에 id 값을 직접 넣을 수 없다보니 객체 생성하기가 까다로워지더라구요,, 테스트코드를 위해 코드를 변경하는 것은 좋지 않다고 보지만 꽤 큰 번거로움이라는 생각이 들었어요! 어떻게 생각하시는지 궁금합니다! (일단은 주석처리해놓았어요)

@songyi00 songyi00 requested a review from chominho96 as a code owner February 8, 2024 13:58
@songyi00 songyi00 self-assigned this Feb 8, 2024
@songyi00 songyi00 added the enhancement New feature or request label Feb 8, 2024
Copy link
Member

@chominho96 chominho96 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!! 리뷰가 늦어서 죄송해요ㅠㅠ 궁금한 점들 코멘트 달아놓았습니다!!

Copy link
Member

Choose a reason for hiding this comment

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

그러면 id를 BaseEntity에 유지하고 GeneratedValue를 빼고 생성자에서 UUID를 주입해주는게 어떨까요?!
그러면 객체 생성 방식도 일관되게 유지할 수 있고, fixture 객체 생성에도 용이할 것 같아서요!!

Copy link
Member Author

@songyi00 songyi00 Feb 13, 2024

Choose a reason for hiding this comment

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

@chominho96 오 그 방법도 고민해보았는데 UUID를 미리 생성해서 객체를 만들 경우 id가 0이나 null이 아니기 때문에 JPA에서 새로운 엔티티로 인식하지 못하는 문제가 있어요ㅠㅠ 그래서 save시 persist가 아닌 매번 merge를 진행하게 되는 문제가 있슴니다.. 그래서 일단 기본 전략을 따라가자고 생각했습니닷,,!
Persistable 인터페이스 구현해서 isNew를 오버라이드해주면 되긴 하는데 그렇게 하는게 나을까요?!

Copy link
Member

Choose a reason for hiding this comment

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

앗 그러네요 save할 때마다 merge하는 방식은 비효율적일거 같아요..!!
Persistable 인터페이스까지 구현하면 배보다 배꼽이 커지는 느낌인 것 같네요 ㅋㅋㅋ큐ㅠ 그렇다면 구현해주신 방향으로 가시죠!!

import jakarta.validation.constraints.Min;

public record TickerShare(
String ticker,
Copy link
Member

Choose a reason for hiding this comment

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

사소하지만 Validaion 체크하는 김에 NotEmpty도 체크하시는건 어떠신가요?!

Copy link
Member Author

Choose a reason for hiding this comment

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

좋습니다 👍

Copy link
Member

Choose a reason for hiding this comment

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

따로 DomainService 인터페이스를 적용하신 이유가 궁금해요!! 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

저희 서비스 특성 상 배당금 관련해서 계산 로직이 많기 때문에 이건 서비스 로직으로 넣을게 아니라 도메인 패키지에 넣어야한다고 생각했어요!
특정 도메인 엔티티 안에 넣을 수 없는 비즈니스 로직의 경우 도메인 서비스로 분리하였습니다!

Copy link
Member

Choose a reason for hiding this comment

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

오호 그러네요 Stock하고 Dividend가 대부분 같이 쓰이게 될 것 같아서 좋은 방식인 것 같아요!

@chominho96 chominho96 self-requested a review February 13, 2024 14:07
Copy link
Member

@chominho96 chominho96 left a comment

Choose a reason for hiding this comment

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

getSectorRatio API 구현 수고하셨습니다~!

@songyi00 songyi00 merged commit 20fc590 into develop Feb 13, 2024
1 check passed
@songyi00 songyi00 deleted the feat/#19 branch February 13, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] api-server 기본 세팅 및 샘플 API 구현
2 participants