-
Notifications
You must be signed in to change notification settings - Fork 1
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/#04 create project #26
Conversation
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.
정말 고생 많으셨습니다...! 💯
가장 중요한 도메인이기에 할 일이 너무 많으셨을 것 같네요..! 👍
개인적으로 코드를 보면서 든 생각인데, 패키지 이름에 이미 project가 있기 때문에 클래스 이름에 Project는 없어도 되지 않나 생각이 듭니다..!
- ProjectFile -> File
- ProjectMember -> Member
ProjectSkill 같은 경우는 UserSkill에도 사용되니까 이러한 경우만 그대로 두어도 괜찮지 않을까 싶은데, 다른 분들의 의견이 궁금합니다..!
다시 한 번 정말 고생 많으셨습니다 ❤️🔥
@Column(name = "start_date", columnDefinition = "TIMESTAMP") | ||
private LocalDate startDate; | ||
|
||
@Column(name = "end_date", columnDefinition = "TIMESTAMP") | ||
private LocalDate endDate; |
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.
크게 상관이 없기는 한데, deleted_at과 함께 통일성을 맞추기 위해 LocalDateTime
은 어떠신가요? 혹시 LocalDate로 설정하신 이유가 있으신가요?
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.
요것은 저희가 2024-03 이런 형식으로 저장하기로 했던 것 같아서 혹시 몰라 필요한 정보만 담는 것으로 구현 했습니다! 여쭤보려 했는데 까먹고 있었네요!
처음에는 통일성을 고려하지 못해서 이렇게 구현했는데 통일성을 고려한 것이라면 Time까지 넣는 것이 좋은 것 같습니당!
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.
(세희님은 아니지만..) 저희가 프로젝트 시작, 종료 날짜를 년/월까지만 받기로 해서 그런 것 같습니당! 그래서 저는 그대로 LocalDate
로 해도 상관없을 것 같은데, 편하신대로 하면 될 것 같습니당!
@@ -0,0 +1,6 @@ | |||
package sixgaezzang.sidepeek.projects.domain; | |||
|
|||
public enum ProjectAuthority { |
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.
생각해보니까 ProjectAuthority
보다는 AuthorityType
이 enum에 더 적합하고 좋은 것 같은데 다들 어떻게 생각하시나요?
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.
저도 enum 클래스 같은 경우에는 특별한 이유가 있지 않으면 Type를 붙이는 편이긴 합니다! 네이밍 좋은 것 같습니당!!
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.
저도 Enum에는 XXXType이라는 네이밍이 더 좋습니당!
근데 또 막상 AuthorityType
이라는 이름을 생각해보면 일반적으로 auth에서 쓰는 type(ex. ADMIN, NOMAL)과 헷갈릴 것 같기도 하니욤.. 근데 저희 프로젝트에는 auth에 따로 AuthorityType이 없고 저희끼리만 이해하고 있으면 될 것 같아서 AuthorityType
으로 해도 될 것 같습니다!
src/main/java/sixgaezzang/sidepeek/projects/domain/ProjectFile.java
Outdated
Show resolved
Hide resolved
src/main/java/sixgaezzang/sidepeek/projects/domain/ProjectMember.java
Outdated
Show resolved
Hide resolved
꼼꼼한 리뷰 감사합니다ㅠㅠ😭 |
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.
LGTM~ 엔티티 엄청 많네요.. 정말 넘넘 고생하셨습니다!! 최고🥹👍🏻
앗 그리고 전 ProjectSkill
과 UserSkill
은 어쩔 수 없는 중복이라고 생각합니다..! (한 테이블에 Project, User 모두에 대한 FK를 넣는 방식도 생각해 봤는데, 테이블명도 모호해지고 더 복잡성만 올라갈 것 같더라구요! 결론은 지금이 좋습니당!!)
@Column(name = "start_date", columnDefinition = "TIMESTAMP") | ||
private LocalDate startDate; | ||
|
||
@Column(name = "end_date", columnDefinition = "TIMESTAMP") | ||
private LocalDate endDate; |
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.
(세희님은 아니지만..) 저희가 프로젝트 시작, 종료 날짜를 년/월까지만 받기로 해서 그런 것 같습니당! 그래서 저는 그대로 LocalDate
로 해도 상관없을 것 같은데, 편하신대로 하면 될 것 같습니당!
src/main/java/sixgaezzang/sidepeek/projects/domain/Project.java
Outdated
Show resolved
Hide resolved
src/main/java/sixgaezzang/sidepeek/projects/domain/Project.java
Outdated
Show resolved
Hide resolved
src/main/java/sixgaezzang/sidepeek/projects/domain/Project.java
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,6 @@ | |||
package sixgaezzang.sidepeek.projects.domain; | |||
|
|||
public enum ProjectAuthority { |
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.
저도 Enum에는 XXXType이라는 네이밍이 더 좋습니당!
근데 또 막상 AuthorityType
이라는 이름을 생각해보면 일반적으로 auth에서 쓰는 type(ex. ADMIN, NOMAL)과 헷갈릴 것 같기도 하니욤.. 근데 저희 프로젝트에는 auth에 따로 AuthorityType이 없고 저희끼리만 이해하고 있으면 될 것 같아서 AuthorityType
으로 해도 될 것 같습니다!
🎫 관련 이슈
Resolves #25
✅ 구현 내용
💬 코멘트
UserSkill
,ProjectSkill
클래스는 뭔가 중복이 보여서 찝찝하지만 더 좋은 방법이 생각나지 않아 우선 단순한 구조, 단순한 방법으로 구현하는 방법으로 진행했습니다!