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
feat: 수강생 명단 페이지 대시보드 조회 API 추가 #796
feat: 수강생 명단 페이지 대시보드 조회 API 추가 #796
Changes from 10 commits
8243f47
24e4dca
7f1be83
6747b93
526b60b
27c983e
1ee2e40
7c4f1a7
5fa5e05
223286d
b60e067
4dbd45f
7841254
991d43b
d9cab13
961dea1
3f40fb3
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.
getSubmittedAssignment
메서드 개선 제안getSubmittedAssignment
메서드의 구조는 좋지만, null 체크가 부족하여NullPointerException
이 발생할 수 있습니다.다음과 같이 null 체크를 추가하고 최적화하는 것을 제안합니다:
이렇게 하면
NullPointerException
을 방지하고 메서드의 안정성을 높일 수 있습니다.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.
memberIds
에 대한null
또는 빈 리스트 처리 필요현재 메서드
findByStudyIdAndMemberIds
에서memberIds
가null
이거나 빈 리스트일 경우, 쿼리에서 예기치 않은 결과가 발생할 수 있습니다.memberIds
에 대한null
및 빈 리스트 체크를 추가하여 예외를 방지하고 적절한 결과를 반환하도록 수정하는 것이 좋습니다.다음과 같이 수정할 수 있습니다:
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.
그냥 한 이슈에서 처리해도 되는데 public 일부러 붙여두신건가 싶어서 별도 이슈로 분리했어요.
한 이슈에서 하면 pr에서 묻힐수도 있을 것 같아서
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.
사소하긴 한데 studyId가 where절 앞에 오는 편이 더 효율적일듯 합니다
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.
쿼리 메서드가 효율적으로 구현되었습니다만, 개선의 여지가 있습니다.
findByStudyIdAndMemberIds
메서드가 QueryDSL을 사용하여 효율적으로 구현되었습니다.in
절을 사용하여 여러 회원 ID를 한 번에 처리하는 것이 좋습니다.하지만
memberIds
파라미터에 대한 null 체크가 없습니다. 빈 리스트가 전달될 경우 문제가 발생할 수 있습니다.다음과 같이
memberIds
에 대한 null 체크를 추가하는 것이 좋겠습니다:📝 Committable suggestion
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.
별도 이슈긴 한데 CANCELLED, CANCELED 혼용하고 있어서 추후 CANCELED로 통일해야 할듯 합니다
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.
근데 SUCCESS와 CANCELED가 기존에 없었다면 해당 enum 사용하는 API는 기존에 어떻게 동작했나요?