Skip to content
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

[BE] feat: club 도메인 추가 #119

Closed
wants to merge 34 commits into from
Closed

Conversation

jimi567
Copy link
Member

@jimi567 jimi567 commented Jul 18, 2024

이슈

개발 사항

  • club 도메인
  • 도메인 관련 VO 검증 로직

리뷰 요청 사항 (없으면 삭제해 주세요)

  • 꼼꼼하게 봐주시면 감사하겠습니당.

전달 사항 (없으면 삭제해 주세요)

  • 필터링 값 제외하여 구현
  • 어노테이션 선언 순서 논의해보면 좋을 것 같아요.
  • VO 내부 필드명 논의해보면 좋을 것 같아요.

- 필터링 값 제외하여 구현
Copy link

github-actions bot commented Jul 18, 2024

Unit Test Results

29 tests   29 ✔️  3s ⏱️
11 suites    0 💤
11 files      0

Results for commit 711f0d4.

♻️ This comment has been updated with latest results.

@ehtjsv2 ehtjsv2 self-requested a review July 18, 2024 10:42
Comment on lines 24 to 27
private void validate(int value) {
if (value < MIN_CAPACITY_SIZE || value > MAX_CAPACITY_SIZE) {
throw new FriendoglyException("모집 인원은 1명 이상 5명 이하 입니다.");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예외 메시지에도 MIN_CAPACITY_SIZE, MAX_CAPACITY_SIZE를 사용할 수 있겠네요.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메세지 자체가 변하는게 아니라면, 관리 포인트를 줄일 수 있겠네요 .

Comment on lines 3 to 8
public enum Status {

RECRUITMENT,
TERMINATE;

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public enum Status {
RECRUITMENT,
TERMINATE;
}
public enum Status {
OPEN,
CLOSED;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

직관적인 네이밍도 좋을 것 같아요. OPEN, CLOSED 어떤가요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

모집 중, 모집 종료 라는 의미로 해당 워딩을 사용했는데,
저희 입장에서는 OPEN , CLOSED도 이해하는데 큰 불편함이 없을 수 있겠네용

Comment on lines 53 to 56

@Column(nullable = false)
private String imageUrl;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 이미지를 등록하지 않은 경우 null이 될 수도 있을 것 같아요.
  • URL도 원시값 포장을 진행하면 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

url의 경우 S3에 이미지 url을 저장하기 전까지 래핑 하지 않는 것으로 얘기가 나와서 이대로 냅두었어요

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

헉 저도 유의해서 개발할게요 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

트레 의견에 동의합니다. 이미지 URL은 nullable로 두는게 맞을 것 같아요.

Comment on lines 57 to 59
@Column(nullable = false)
@Temporal(TemporalType.TIMESTAMP)
private LocalDateTime createdAt;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@Column(nullable = false)
@Temporal(TemporalType.TIMESTAMP)
private LocalDateTime createdAt;
@Column(nullable = false)
private LocalDateTime createdAt;

LocalDateTime을 사용하면 생략 가능하다고 하네요!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저희 배포 환경이 Mysql이라 , TIMESTAMP 타입으로 저장하려고 저렇게 명시해주긴 했어요.
제가 알기론 생략하면 Mysql에서 DATETIME으로 저장하는걸로 알고 있어서요
근데 좀 찾아보니깐 LocalDateTIme은 @colume으로 지정해줘야된다고하네여 수정해야될 것 같아요

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좀 더 고민해보니깐 지금 단계에서 DATETIME으로 저장하나 타임스템프쓰나 별 차이 없는 것 같아서 지우도록 하겠습니당

Comment on lines 46 to 51
@OneToMany
private List<Member> participantMembers = new ArrayList<>();

@OneToMany
private List<Pet> participantPets = new ArrayList<>();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ArrayList 생성은 생성자에서 하는 것도 좋을 것 같아요

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 필드 초기화가 안전하다는 말도 있네요.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 생성자 초기화에 한표


private void validate(String content) {
if (StringUtils.isBlank(content) || content.length() > MAX_CONTENT_LENGTH) {
throw new FriendoglyException("내용은 1글자 이상 1000글자 이하로 작성해주세요.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new FriendoglyException("내용은 1글자 이상 1000글자 이하로 작성해주세요.");
throw new FriendoglyException(String.format("내용은 %d글자 이상 %d글자 이하로 작성해주세요.", MIN_CONTENT_LENGTH, MAX_CONTENT_LENGTH));

Comment on lines 29 to 31
}

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}
}
}

Comment on lines +24 to +27
void create_Fail(int memberCapacityInput) {
assertThatThrownBy(() -> new MemberCapacity(memberCapacityInput))
.isInstanceOf(FriendoglyException.class);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예외 메시지도 함께 검증해 봐도 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분도 다같이 얘기해보면 좋을 것 같아요.

예외 메세지는 기능적으로 중요한 부분은 아니라서 해당 메세지로 인해서 테스트가 깨지는건 불필요하다. vs 어떤 경우로 해당 예외가 나왔는데 검증해야된다 갈리는 것 같아요.

Copy link
Contributor

@ehtjsv2 ehtjsv2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굿굿

private Member owner;

@OneToMany
private List<Member> participantMembers = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

양방향 의존관계를 2레벨에 공부했었는데, 안쓰고 해보는 것은 어떨까요
JPA 양방향 연관관계 관련하여 질문 드립니다. <- 답변이 맛있더라구요
양방향 의존관계-2
위브가 양방향 의존관계를 써야했던 이유도 궁금하네요~!
남의 도메인을 참조해야하는 것이 걸려서인가요?

Copy link
Member Author

@jimi567 jimi567 Jul 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

양방향 의존 관계 생각하다가 떠오른 부분인데
Club : Member(pets) 관계가 ManyToMany 였어요.
수정할게요. 리뷰 덕분에 리마인드 됐네요.

}

private void validate(String title) {
if (StringUtils.isBlank(title) || title.length() > MAX_TITLE_LENGTH) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringUtils.isBlank() 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 좋네요

Copy link
Contributor

@J-I-H-O J-I-H-O left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생했어요 위브~ 같이 얘기 나눠봐요

Comment on lines 53 to 56

@Column(nullable = false)
private String imageUrl;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

트레 의견에 동의합니다. 이미지 URL은 nullable로 두는게 맞을 것 같아요.

Comment on lines +25 to +26
String contentInput = "a".repeat(contentLength);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😊👍

takoyakimchi
takoyakimchi previously approved these changes Jul 18, 2024
Copy link
Contributor

@takoyakimchi takoyakimchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생 많았습니다 👍

@jimi567 jimi567 added 🖥 backend backend ✨ feat 기능 개발 labels Jul 19, 2024
Copy link

Test Results

3 tests  ±0   3 ✅ ±0   0s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 711f0d4. ± Comparison against base commit ffcb405.

takoyakimchi
takoyakimchi previously approved these changes Jul 20, 2024
Copy link
Contributor

@takoyakimchi takoyakimchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사소한 코멘트 몇 개 남겨 보았습니다
고생했어요

Comment on lines +6 to +8
CLOSED;

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CLOSED;
}
CLOSED;
}

Comment on lines +13 to +20

@DisplayName("생성 테스트")
@Test
void create() {
assertThatCode(() -> new Content("이번주 주말에 공원에서 산책하실 분~?"))
.doesNotThrowAnyException();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

경계값 테스트 해도 좋을 것 같네요.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예외 터지는 부분에서 경계 테스트해서 충분하다고 생각했는데 가능한 영역 내부에서 테스트하면 더 맛있을 것 같아요, 굿

Comment on lines +13 to +20

@DisplayName("생성 테스트")
@Test
void create() {
assertThatCode(() -> new Title("잠실동 견주 모임 모집해요!!"))
.doesNotThrowAnyException();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도 ParameterizedTest로 경계값 테스트 하면 좋겠어요

@jimi567 jimi567 dismissed takoyakimchi’s stale review July 20, 2024 02:41

The merge-base changed after approval.

takoyakimchi
takoyakimchi previously approved these changes Jul 20, 2024
@jimi567 jimi567 dismissed takoyakimchi’s stale review July 20, 2024 02:50

The merge-base changed after approval.

ehtjsv2
ehtjsv2 previously approved these changes Jul 20, 2024
Copy link
Contributor

@ehtjsv2 ehtjsv2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

백엔드-리뷰방 알람만 켜져있어서 늦었네요,,잘봤습니다~!


private static final int MAX_CONTENT_LENGTH = 1000;

@Lob
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저장된 데이터의 크기가 크지않더라도 조회할때 현저히 속도가 떨어지는 문제가 생기는 사람도 있다는데, 혹시라도 우리도 이러한 문제 접하게 되면 해결책을 찾아봅시다~!
모임 목록 조회에는 content가 필요하지않으니 lazy해도 좋겠네요!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분 저도 고민인데 따로 테이블 뺀게 없어서 lazy 비슷한 기능을 활용할 수 있는지 모르겠네요.ㅠㅠ

@jimi567 jimi567 dismissed ehtjsv2’s stale review July 20, 2024 04:53

The merge-base changed after approval.

@jimi567 jimi567 closed this Jul 20, 2024
@jimi567 jimi567 deleted the feature/club-entity branch July 20, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🖥 backend backend ✨ feat 기능 개발
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants