-
Notifications
You must be signed in to change notification settings - Fork 0
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
[SKRB-143] feat: 클럽 엔티티, 서비스 및 리포지토리 생성 #13
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.
🫡
@Builder | ||
public Club(Long id, String name, String image, String info) { | ||
Assert.notNull(id, "클럽 ID는 null 값이 올 수 없습니다"); | ||
Assert.hasText(Long.toString(id), "클럽 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.
id는 Long형인데 String으로 바꿔서 빈 값인지 테스트하는 이유가 있을까요?
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.
Assert.hasText 메소드의 첫번째 파라미터 타입이 String 으로 받게끔 되어있더라고요
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.
Long형에 숫자 타입인데 빈 값을 검사해야 되는 지가 궁금했습니다! Long형에 빈 값 자체가 들어갈 수 없지 않나요?
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.
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.
고생하셨습니다! 화이팅 !! 🥂
public class ClubController { | ||
|
||
private final ClubService service; | ||
|
||
@PostMapping("/club") |
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.
REST API 컨벤션에 따라서 clubs라 변경이 필요해 보여요!
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.
네 기본이 복수를 원칙으로 하는것으로 알고있어요
특정 자원에 접근하는것이 아니라 어떤 자원에 접근하는지를 뜻하니까요
그리고 하나의 클럽에 접근하는 것이면 clubs/{clubId}로 접근하는것이 좋아보여요
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.
그리괘 club을 생성하고난 후 왜 도메인 실제 url을 보여주나요?
location을 통해서 생성된 club에 접근할 수 있는 uri를 보여주는게 낫다고 생각해요
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.
club에 접근할 수 있는 uri 는 어떤 형태로 생긴건가요?
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.
@GeneratedValue | ||
private Long id; | ||
|
||
@Column(length = 12, nullable = false) |
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.
클럽 이름 길이를 12자로 제한하는 것으로 설정하신건가요?
그러면 DB 단에서도 제약조건을 걸어 주었듯 어플리케이션 단에서 생성자에서 객체 생성 자체를 막는 과정도 필요해 보여요
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.
@Builder | ||
public Club(Long id, String name, String image, String info) { | ||
Assert.notNull(id, "클럽 ID는 null 값이 올 수 없습니다"); | ||
Assert.hasText(Long.toString(id), "클럽 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.
hasText에 notnull이 포함되어 있기 때문에 not 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.
id뿐 아니라 name, image또한 강제한다면 hasText로 검증이 필요해보여요
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.
id는 자동생성될 필드라서 검증은 삭제했고 name 검증은 추가 했습니다
17914c3
to
d531727
Compare
고생하셨습니다!! �커멘트 한번 확인해주세요!! 👍 |
d531727
to
98e356e
Compare
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.
👍
33d0af0
to
cb37f54
Compare
💠 Jira 티켓 링크
🖥️ 작업 내용
✅ PR시 확인 사항
📢 리뷰어 전달 사항