지가나던 사람의 피드백입니다.

hyunsu15 commented 3 months ago

안녕하세요. 깃허브 추천 알고리즘으로 선생님 프로젝트를 보게 된 사람입니다.

프로젝트를 보면서 좋았던 점과 아쉬웠던 점을 말씀드리고 싶어서 이슈에 남깁니다. 참고하시면 좋은 내용은 링크도 남겨두겠습니다.

아쉬운 점

entity는 도메인이 아닙니다.

사이드 프로젝트이니 편하게 작업을 하신 의도가 같지만, 저는 분리 하는 것이 맞다고 생각합니다. 지금은 mybatis를 쓰신것 같지만 jpaEntity로 변경하게 된다면, domain도 jpa 관련어노테이션으로 지저분 해집니다. entity 관련 어노테이션은 domain과 관련없는 내용입니다.

https://wonit.tistory.com/652

패키지 응집이 아쉽습니다.

예를 들면, 제가 만든다고 한다면, Card와 CardStatus를 한 패키지에 둘 것 같습니다.

Card 코드중에 이런 임포트문이 필요할까 싶습니다.

import org.wedding.domain.CardStatus;

임포트에 관해서 생각해보시면 좋은 패키지 응집이 가능합니다.

패키지를 고민하면서 개발하시다면, visibility로 무조건 public이 아니라, 경우에 따라선 default를 써도 되는 클래스도 구별될겁니다.

https://www.youtube.com/watch?v=RVO02Z1dLF8

도메인이 더 능동적이었으면 좋을 것 같습니다.

  checkDuplicateCardTitle(command.cardTitle());
     Card card = CreateCardCommand.toEntity(command);

CardService에서 카드제목 중복체크를 하고, 저장하는 내용으로 보입니다.

이렇게 만드시면 어떨까요?

boolean isDuplicateCardTitle = checkDuplicateCardTitle(command.cardTitle())
card.checkDuplicateCardTitle(isDuplicateCardTitle);

이렇게 수정한다면, 비지니스 로직이 도메인으로 들어가게되는 장점이있습니다.

비지니스 로직을 테스트할때도 테스트 더블을 만들 필요없이, 도메인테스트를 진행하면 됩니다. 또한, card 관련 로직은 card객체에 집중되기에, 더 좋은 응집성을 경험할 수 있습니다.

https://velog.io/@leejh3224/%EB%B2%88%EC%97%AD-%EB%8F%84%EB%A9%94%EC%9D%B8-%EB%AA%A8%EB%8D%B8-%EC%88%9C%EC%88%98%EC%84%B1-vs-%EB%8F%84%EB%A9%94%EC%9D%B8-%EB%AA%A8%EB%8D%B8-%EC%99%84%EC%A0%84%EC%84%B1

선택사항

is .collect(Collectors.toList()) ToBe .toList()

8버전도 사용해보신것같은데, 프로젝트는 17버전이니, 최신 java api로 편하게 작업해보시면 좋을 것같습니다.

좋았던 점

문서화

혼자서 프로젝트를 하다보면, 문서를 등한시 할 수 있다고 생각합니다. 하지만 프로젝트 문서를 보고, 감탄했습니다.

열정

이러한 문서들을 보니, 열정이 있으시다 라는 느낌을 저절로 느껴졌습니다. 제가 왜 자주 떨어지는지도 살짝 반성하게 되었습니다. 이러한 동료와 한번쯤은 일해보고 싶다는 느낌을 받았습니다.

이력서 관련

선생님의 열정에 감동해서 이력서도 확인했는데, 수정해야 할 부분이나 추가하면 좋을 부분이 있는 것 같아서 말씀드립니다.

컬리 관련 이력 관련 날짜 기입이 이상합니다.

2022.05-2022.10(1년 6개월)

ElastiCache 비용 및 성능 최적화 작업 (23.04 ~ 22.07)

처음에 보고, 당황했었습니다.

하셨던 프로젝트 성능 개선도 공유해주시면 더 좋을 것 같습니다.

일해보신 경험인 ElastiCache 비용 및 성능 최적화 작업으로 연간 2억이상의 금액을 절감시켰다는 내용이 있어서 흥미로웠습니다. 면접때 세부적으로 이야기 할 내용일 수 있습니다.개인적으로는 문서화 하셔서 링크도 남겨두시면, 좋을 것 같습니다. 문서화를 열심히 하시는 장점도 더 어필할 수 있을 것 같아요.

맺음말

선생님 사이드 프로젝트를 보면서, 저도 많을 것을 배웠습니다. 앞으로도 더 잘되셨으면 좋겠습니다.

zeze1004 commented 3 months ago

@hyunsu15 님 안녕하세요. 먼저 정성어린 리뷰 감사드립니다.

아쉬운 점에 대해서는 평소에도 고민이 많았던 부분이었는데 명쾌하게 리뷰해주셔서 감사합니다. 참고하여 리팩토링하고자 합니다. 그리고 이력서 관련하여 제안주신 부분도 반영했습니다! 저도 놓쳤던 부분인데 꼼꼼히 봐주셔서 감사합니다.

부족한 실력이지만 혹시 필요한 리뷰(?)가 있다면 성심성의껏 확인해보도록 하겠습니다. 다시 한 번 감사드립니다.

hyunsu15 commented 3 months ago

저도 부족하지만, 혹시 도움이 필요하거나 이해가 안되는 부분이 있다면 말씀해주세요!!

ilgolf commented 2 months ago

글 잘 읽어봤습니다. 여기서 전 의문이 있는게 "domain entity는 jpa entity와 다르다." 저도 이 말에 동의 합니다. 다만 jpa entity를 domain entity로서 표현하면 jpa entity도 domain entity라고 볼 수 있습니다.

분리하게 될 경우 오히려 aggregate 로 하위 aggregate를 저장하고 완성된 도메인 정보를 반환하는 과정에서 osiv 이점이나 jpa dirty checking 같은 jpa 영속성의 이점을 못누릴 수도 있다고 생각합니다.

결론적으로 소정님이 이는 trade off를 고려하여 적당한 조율을 하시면 좋을 것 같습니다. (전 개인적으로 jpa entity를 domain entity로 표현해보면 좋을 것 같습니다.)

ilgolf commented 2 months ago

추가적으로

val isDuplicateCardTitle: Boolean = checkDuplicateCardTitle(command.cardTitle)
card.checkDuplicateCardTitle(isDuplicateCardTitle);

문제 영역의 불변식 또는 표현하고자 하는 동작이 맞는지 고민이 필요합니다. 도메인 모델을 코드로 표현한게 domain entity인데 문제의 영역에 중복검사라는 맥락이 필요하지 않다면 빼는게 좋습니다.

오히려 UseCase에서도 이와 같은 맥락을 제거하고 infrastructure service로 위임하면 어떨까요? 중복 체크는 어디까지나 unique key 때문이 아닐가 싶습니다.

zeze1004 / wedding-jira-backend