Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
#39 헌혈인증 내역 조회 API 개발 #40
base: develop
Are you sure you want to change the base?
#39 헌혈인증 내역 조회 API 개발 #40
Changes from 8 commits
7ef5ba7
30ae33d
9532e69
7381a5d
43afeef
435eb00
e2109df
1a2670e
2af9916
10b7864
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
JUnit 5부터 접근제한자 public을 생략해도 되는 것으로 알고 있습니다.🙂
+ 메소드명을 통해 어떤 상황에서 무슨 결과를 검증하는 테스트인지 조금이나마 짐작할 수 있으면 좋을 것 같아요 ㅠ_ㅠ
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.
혹시 list를 map으로 변환할 때, key가 중복되는 경우도 허용하려는 의도로 위와 같이 구현하신 걸까요??
mapFrom, filter 메소드에서는 Collectors.toList() 메소드를 사용하셨는데, associateFrom 메소드에서는 Collectors.toMap() 메소드를 사용하지 않으신 이유도 궁금합니다..!
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.
제가 원하는 결과는 Value 가 있으면 (Key, Value) 가 나오고 없으면 (Key, null) 이 나오도록 하길 원했는데, toMap 같은 경우에 Key 에 대한 Value 가 없을 경우 NPE 가 발생하더라구요. 그래서 직접 HashMap 을 만들도록 구현했습니다 :)
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.
Ref) https://stackoverflow.com/questions/24630963/nullpointerexception-in-collectors-tomap-with-null-entry-values
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.
테스트 코드 작성 편의 목적으로
@AllArgsConstructor
을 추가하신 것으로 생각되는데 맞을까요..?개인적으로 Dto의 경우
@AllArgsConstructor
나@Setter
가 있어도 큰 문제가 없다고 생각하지만,Entity의 경우 Id 주입 시점이 DB에 row를 저장한 시점(
IDENTITY
)만 가능하기 때문에,@AllArgsConstructor
을 열어두는 것은 좋은 코드라는 생각이 들지 않네요 ㅠ_ㅠ@AllArgsConstructor
을 열어두는 방법 대신, 테스트 코드에서ReflectionTestUtils
을 이용하여 Id를 주입해 주는 방식은 어떠실까요..?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.
말씀해주신 내용에 대해서는 충분히 이해가 가는 내용입니다! 엄격하게 따졌을 때 굉장히 중요한 내용인것 같고, 잘못 개발 되었을 때 발생할 수 있는 문제를 잘 잡아주신것 같아요!
개인적으로는 이러한 부분을 엄격하게 챙기는 것과 어느정도 개발 편의성을 가져가는 것에 트레이드 오프가 필요하다고 생각하는데요,, JPA 에 대한 이해도가 있는 상태라면 도메인 Entity 객체를 생성할 때 ID 를 넣지는 않도록 관리가 되어질수는 있을것 같다는 생각입니다!
실제로 코틀린에서는 일반적으로 data class 를 이용해 Entity 클래스를 만드는데, data class 에는 기본적으로 AllArgsArgument 생성자를 가집니다..! 그럼에도 data class 를 이용해 Entity 클래스를 만드는 이유는 이로인해 따라오는 개발 편의성이 매우 크기 때문입니다. 추가로 Reflection 은 개인적으로 별로 선호하지 않는 방법입니다!!
이 부분에 대해서는 약간의 타협점이 필요해보이네요! 🤔
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.
음.. 저희끼리만(혹은 JPA를 잘 아는 사람들끼리) 개발 및 유지보수 한다는 가정 하에는 개발 편의성을 선택하는 것도 방법이겠군요..
그럼 이 부분은 제가 작성한 코드에도 적용하도록 하겠습니다!
그리고 혹시 Reflection을 선호하지 않으시는 이유도 여쭤봐도 될까요?
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.
굳이 맞출필요는 없는 부분이고 선필님이 작업하시는 범위는 하시는대로 진행해주셔도 될것 같아요 :) 크게 충돌이 날만한 부분은 아닌것 같아서요!
제가 Reflection 을 선호하지 않는 이유는 컴파일시점에 타입에 대한 체크가 어려워서입니다! 물론 id 같은 경우엔 Long 타입이 거의 고정시되어있어서 크게 의미가 있는 부분은 아닌것 같지만요. 특히 테스트코드가 아니라 비즈니스 로직에서는 더 안좋은 영향을 많이 주는것 같아요.
This file was deleted.
This file was deleted.
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.
위와 동일한 내용입니다!
추가)
fetch join을 이용하는 경우 메소드 네이밍에 차별점을 주는 건 어떨까요?
fetch join 없이 조회하는 메소드와 공존하는 경우도 고려하면 좋을 것 같습니다!
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.
Fetch Join 이 발생한다는게 해당 메소드를 호출하는쪽에서 관심 가질만한 부분인지 잘 모르겠습니다.. 🤔 사용하는쪽의 입장을 생각하고 메소드이름을 짓는편이 비즈니스 로직의 가독성이 더 좋아진다고 생각해서.. 고민스러운 부분이네요!
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.
fetch join이 필요 없는 곳에서도 해당 메소드를 사용하면 아무래도 불필요한 데이터까지 db에서 fetch 하는게 비효율적이라.. 저는 구분하는 걸 선호하는편입니다 ㅠ_ㅠ
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.
음....🤔 이 부분은 말씀해주신대로
findAllFetchPetBy(Long petId)
정도로 정리해볼게요~