[fix] 연관 관계를 수정 #37
[fix] 연관 관계를 수정 #37
Conversation
|
PR 제목 포맷, 앞서 통합된 PR과 일관되게 맞춰주세요. PR을 github project에 통합하여 관리하기 위해 assignees, labels 등의 속성 설정해주세요 |
| List<Long> githubRepoId = notificationAgreements.stream() | ||
| .map(NotificationAgreement::getGithubRepoId) | ||
| .toList(); |
There was a problem hiding this comment.
| List<Long> githubRepoId = notificationAgreements.stream() | |
| .map(NotificationAgreement::getGithubRepoId) | |
| .toList(); | |
| List<Long> githubRepoIds = notificationAgreements.stream() | |
| .map(NotificationAgreement::getGithubRepoId) | |
| .toList(); |
리스트 타입의 변수가 단수형으로 지어질 경우 의미가 불명확해진다고 생각합니다. 수정 부탁드립니다.
There was a problem hiding this comment.
리스트 타입의 변수가 단수형으로 지어질 경우 의미가 불명확해진다고 생각합니다. 수정 부탁드립니다.
넵!
| for (NotificationAgreement agreement : agreements) { | ||
| this.notificationAgreements.add(agreement); | ||
| agreement.setGitHubRepo(this); | ||
| agreement.setGitHubRepoId(this.getId()); | ||
| } |
There was a problem hiding this comment.
특별한 목적이 있는 것이 아니라면 Stream API를 활용해보는 건 어떨까요? 또한, 값을 변경하는 메서드를 단순히 setter로 작명할 경우 정확히 어떤 작업을 수행하는 지 의미가 모호할 수 있다고 생각합니다. updateGithubRepoId와 같은 메서드명은 어떨까요?
There was a problem hiding this comment.
특별한 목적이 있는 것이 아니라면 Stream API를 활용해보는 건 어떨까요? 또한, 값을 변경하는 메서드를 단순히 setter로 작명할 경우 정확히 어떤 작업을 수행하는 지 의미가 모호할 수 있다고 생각합니다.
updateGithubRepoId와 같은 메서드명은 어떨까요?
넵 그런식으로 수정하겠습니다.
| default GithubRepoCreateResponse toGithubRepoCreateResponse(String owner, | ||
| String reponame) { | ||
|
|
||
| return new GithubRepoCreateResponse( | ||
| owner, | ||
| reponame |
There was a problem hiding this comment.
해당 클래스의 매핑 메서드들에 default 가시성을 설정한 이유에 대해 설명 부탁드립니다.
There was a problem hiding this comment.
해당 클래스의 매핑 메서드들에
default가시성을 설정한 이유에 대해 설명 부탁드립니다.
owner와 reponame 두 개의 파라미터를 받아 Response를 생성하는 방식은 MapStruct가 처리할 수 없기 때문에 default를 사용했습니다.
There was a problem hiding this comment.
좀 더 상세히 설명주시겠어요? Mapstruct가 default 가시성을 설정해주어야 처리를 위해 접근하다는 의미로 이해하는 것이 맞는 지 부연 설명 부탁드립니다.
| @Column(name = "github_repo_id", nullable = false) | ||
| private Long githubRepoId; |
There was a problem hiding this comment.
| @Column(name = "github_repo_id", nullable = false) | |
| private Long githubRepoId; | |
| @Column(name = "github_repo_id") | |
| private long githubRepoId; |
기본 타입으로 설정할 경우 굳이 래퍼 타입을 쓰고 nullable=false를 설정할 필요가 없습니다. 굳이 래퍼 타입을 쓰신 이유가 있다면 설명 부탁드립니다.
There was a problem hiding this comment.
기본 타입으로 설정할 경우 굳이 래퍼 타입을 쓰고
nullable=false를 설정할 필요가 없습니다. 굳이 래퍼 타입을 쓰신 이유가 있다면 설명 부탁드립니다.
몰랐습니다! 수정하겠습니다
| List<NotificationAgreement> findAllByGithubRepoIdAndNotificationType(Long githubRepoId, | ||
| NotificationType notificationType); | ||
|
|
||
| List<NotificationAgreement> findAllByNotificationType(NotificationType notificationType); | ||
|
|
||
| void deleteByGithubRepo(GithubRepo repo); | ||
| void deleteByGithubRepoId(Long githubRepoId); |
There was a problem hiding this comment.
githubRepoId가 not null이라면 파라미터를 기본 타입으로 다뤄도 괜찮지 않을까요? 타입의 적절한 사용에 대해 고민해보시면 좋을 것 같습니다.
There was a problem hiding this comment.
githubRepoId가 not null이라면 파라미터를 기본 타입으로 다뤄도 괜찮지 않을까요? 타입의 적절한 사용에 대해 고민해보시면 좋을 것 같습니다.
넵! 수정하겠습다
📌 작업 개요
✨ 기타 참고 사항
🔗 관련 이슈