-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
- calculateSectorRatios
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!! 리뷰가 늦어서 죄송해요ㅠㅠ 궁금한 점들 코멘트 달아놓았습니다!!
There was a problem hiding this comment.
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 객체 생성에도 용이할 것 같아서요!!
There was a problem hiding this comment.
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
를 오버라이드해주면 되긴 하는데 그렇게 하는게 나을까요?!
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사소하지만 Validaion 체크하는 김에 NotEmpty도 체크하시는건 어떠신가요?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋습니다 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
따로 DomainService 인터페이스를 적용하신 이유가 궁금해요!! 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저희 서비스 특성 상 배당금 관련해서 계산 로직이 많기 때문에 이건 서비스 로직으로 넣을게 아니라 도메인 패키지에 넣어야한다고 생각했어요!
특정 도메인 엔티티 안에 넣을 수 없는 비즈니스 로직의 경우 도메인 서비스로 분리하였습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호 그러네요 Stock하고 Dividend가 대부분 같이 쓰이게 될 것 같아서 좋은 방식인 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getSectorRatio API 구현 수고하셨습니다~!
Issue Number
close: #19
작업 내역
변경사항
PR 특이 사항
BaseEntity
에서 id 와 equals, hashcode를 제거하고 각각 엔티티에서 관리하는게 더 좋을 것 같아요ㅠㅠ 테스트 코드 작성시에 id 값을 직접 넣을 수 없다보니 객체 생성하기가 까다로워지더라구요,, 테스트코드를 위해 코드를 변경하는 것은 좋지 않다고 보지만 꽤 큰 번거로움이라는 생각이 들었어요! 어떻게 생각하시는지 궁금합니다! (일단은 주석처리해놓았어요)