[BE] issue122: 스터디 참여 기능 #132

tco0427 · 2022-07-26T08:21:04Z

요약

스터디 참여 기능

세부 설명

도메인에서의 검증이나 스터디원 증가 로직이 중요하다고 판단하여, Bottom Up 으로 도메인 부터해서 올라가는 식으로 구현하였습니다. 오늘 인수 테스트 작성은 완료하지 못하였고, 내일 중으로 완료될 것 같습니다!
누락된 검증이나 피드백 주시면 감사하겠습니다!😃

API 명세 도출
한 사람은 하나의 스터디에 대해 한 번만 참여가 가능하다.
인원이 다 찼을 경우 모집을 종료한다.
CLOSE 상태의 스터디에는 참여가 불가능하다.
모집 기간(enrollmentEndDate)인 동안만 참여가 가능하다.
인수 테스트 작성

close #122

…oamoa into feat/122-study-participation # Conflicts: # backend/src/main/java/com/woowacourse/moamoa/study/domain/Study.java

sc0116

디우 잘 구현하셨네요 허허이~! 몇가지 코멘트 남겨두었어요 확인 부탁드려요!

sc0116 · 2022-07-28T00:35:32Z

backend/src/main/java/com/woowacourse/moamoa/study/domain/Details.java

@@ -34,6 +38,12 @@ public Details(final String title, final String excerpt, final String thumbnail,
        this.description = description;
    }

+    protected void checkStudyStatus() {
+        if (status.equals(CLOSE)) {


status를 enum으로 두면 CLOSE같은 상수를 안써도 될 것 같네용

넵!! 린론 코드와 병합하는 과정에서 열거형 상수를 사용하게 되면 수정하도록 하겠습니다!

sc0116 · 2022-07-28T00:39:51Z

backend/src/main/java/com/woowacourse/moamoa/study/domain/Participants.java

+    }
+
+    private boolean isInvalidMemberSize() {
+        return max == null || max <= size;


max가 null이면 무제한이라는 뜻 아닌가요?? 무제한이면 받을 수 있지 않을까요?

protected void checkParticipating(Long memberId) { if (isInvalidMemberSize() || isAlreadyParticipation(memberId)) { throw new InvalidParticipationStudyException(); } }

넵!! 짱구 말처럼 스터디 최대 인원 이 설정되지 않은 경우에는 가입이 가능해야하는데요, 따라서 max == null 이면 isInvalidMemberSize 가 True가 반환되고 예외가 터지겠네요! max != null 로 수정하여야 겠네요!

오류 발견 굿굿!

sc0116 · 2022-07-28T00:46:33Z

backend/src/main/java/com/woowacourse/moamoa/study/domain/Period.java

+    }
+
+    protected void checkParticipatingPeriod() {
+        if (enrollmentEndDate != null && !enrollmentEndDate.isAfter(LocalDate.now())) {


요건 취향이긴 한데 !와 isAfter면 isBefore가 더 쉽게 읽히지 않을까 합니다!

오..!! 좋은 생각입니다!😃

sc0116 · 2022-07-28T00:47:04Z

backend/src/main/java/com/woowacourse/moamoa/study/domain/Period.java

@@ -50,4 +58,18 @@ public boolean equals(final Object o) {
    public int hashCode() {
        return Objects.hash(enrollmentEndDate, startDate, endDate);
    }
+
+    private void validatePeriod(final LocalDate enrollmentEndDate, final LocalDate startDate, final LocalDate endDate) {
+        if (isImproperStudyDate(startDate, endDate) || isImproperEnrollmentEndDate(enrollmentEndDate, endDate)) {


jaejae-yoo

디우 구현하느라 고생하셨어요 ~ 코멘트 확인 부탁드립니다.

jaejae-yoo · 2022-07-28T01:48:19Z

backend/src/test/java/com/woowacourse/moamoa/study/domain/DetailsTest.java

+    public void checkStudyStatus() {
+        final Details details = new Details("title", "excerpt", "thumbnail", CLOSE, "description");
+
+        assertThatThrownBy(() -> details.checkStudyStatus());


예외 타입은 확인하면 좋을 것 같아요 ~

좋습니다..!! isInstanceOf() 로 체크하도록 하겠습니다.👍

jaejae-yoo · 2022-07-28T01:49:37Z

backend/src/main/java/com/woowacourse/moamoa/study/domain/Participants.java


    public Participants(final Integer size, final Integer max,
-                        final List<Participant> participants, Long ownerId) {
+                        final Set<Participant> participants, Long ownerId) {


중복된 값 X 👍 명확해서 좋아요!

jaejae-yoo · 2022-07-28T01:50:42Z

backend/src/main/java/com/woowacourse/moamoa/study/domain/Participants.java

+
+    private boolean isAlreadyParticipation(final Long memberId) {
+        final Participant participant = new Participant(memberId);
+        return Objects.equals(memberId, ownerId) || participants.contains(participant);


조건을 메서드로 분리하면 owner는 참여자에 포함하지 않기로 한 도메인 규칙이 잘 드러날 것 같아요!

그린론이 말해준 내용이 아래와 같을까요??
우선 아래와 같이 이해한대로 변경해 보았습니다!!

private boolean isAlreadyParticipation(final Long memberId) { final Participant participant = new Participant(memberId); return isOwner(memberId) || isParticipant(participant); } private boolean isOwner(final Long memberId) { return Objects.equals(memberId, ownerId); } private boolean isParticipant(final Participant participant) { return participants.contains(participant); }

verus-j · 2022-07-28T01:44:54Z

backend/src/main/java/com/woowacourse/moamoa/study/domain/Details.java

+    protected void checkStudyStatus() {
+        if (status.equals(CLOSE)) {
+            throw new InvalidParticipationStudyException();
+        }
+    }
+


checkStudyStatus보다 좀 더 의미 있는 이름은 없을까요???
현재는 스터디가 종료됐는지 확인하는데 checkStudyStatus라는 메서드 이름은 이 상황에 맞지 않은거 같아요

저도 isCloseStatus() 등등..이 부분에서 고민이 많았는데요..🤔
모집 종료인지 확인한다라는 의미에서 checkClosingRecruitment는 어떤가요??
우선 위의 메소드명으로 수정해보겠습니다!

verus-j · 2022-07-28T01:45:35Z

backend/src/main/java/com/woowacourse/moamoa/study/domain/Participants.java

+    public static Participants createByMaxSizeAndOwnerId(final Integer maxSize, Long ownerId) {
+        return new Participants(1, maxSize, new HashSet<>(List.of(new Participant(ownerId))), ownerId);
+    }


참여자에 방장은 넣지 않기로 하지 않았나요???

헉...수정하겠습ㄴ디ㅏ!

verus-j · 2022-07-28T01:47:29Z

backend/src/main/java/com/woowacourse/moamoa/study/domain/Participants.java

+    protected void checkParticipating(Long memberId) {
+        if (isInvalidMemberSize() || isAlreadyParticipation(memberId)) {
+            throw new InvalidParticipationStudyException();
+        }
+    }
+
+    private boolean isInvalidMemberSize() {
+        return max != null && max <= size;
+    }
+
+    private boolean isAlreadyParticipation(final Long memberId) {
+        final Participant participant = new Participant(memberId);
+        return Objects.equals(memberId, ownerId) || participants.contains(participant);


이부분을 participate 메서드 안에 넣어서 캡슐화하면 더 좋을 것 같아요...
현재는 Study에서 checkParticipating을 호출하는 것 처럼 보이는데 스터디에 참여하기 위해 체크하는 부분이 외부로 드러나서 캡슐화가 필요한 것 같습니다.

오 좋습니다!! 외부에 드러낼 필요가 없네요! 수정하였습니다!

verus-j · 2022-07-28T01:50:51Z

backend/src/main/java/com/woowacourse/moamoa/study/domain/Study.java

+        details.checkStudyStatus();
+        period.checkParticipatingPeriod();
+        participants.checkParticipating(memberId);


각각의 Embeddable 객체에서 예외를 던지는게 아니라 상태만 확인하고 Study에서 예외를 던지는건 어떨까요??
check라는 이름이 어떤 상황일 때 예외를 던지는지에 대한 내용을 전혀 담고 있지 않기 때문에 조금 더 드러내는게 좋을 것 같습니다.
Study 클래스 하나만 보고 어떤 상황에서 사용자가 참여가능한지 드러나는게 가독성면에서 좋을 것 같아요

수정해보았습니다!! 확인 부탁드려요~_~

verus-j · 2022-07-28T01:52:23Z

backend/src/main/java/com/woowacourse/moamoa/study/service/StudyService.java

+    private Study findStudyBy(final Long studyId) {
+        return studyRepository.findById(studyId)
+                .orElseThrow(StudyNotFoundException::new);
+    }
+
+    private Member findMemberBy(final Long githubId) {
+        return memberRepository.findByGithubId(githubId)
+                .orElseThrow(() -> new UnauthorizedException(String.format("%d의 githubId를 가진 사용자는 없습니다.", githubId)));
+    }


이 부분은 굳이 메서드 추출하지 않아도 될 것 같습니다. 차라리 각 메서드에 드러나는게 우선은 좋아보여요...
중복된 로직이 많은 경우에 다시 한번 생각해보는게 좋아 보입니다.

현재는 중복되는 부분이 2곳 뿐이니 좋습니다!
그런데 개인적으로 3곳이상 중복 발생하면 이부분은 메소드로 추출해내도 괜찮은 것 같습니다.
findStudyBy(final Long studyId) 라는 네이밍에서 우리가 알 수 있어서..

verus-j · 2022-07-28T01:57:42Z

backend/src/test/java/com/woowacourse/acceptance/study/ParticipationStudyAcceptanceTest.java

+import org.springframework.http.HttpStatus;
+import org.springframework.jdbc.core.JdbcTemplate;
+
+public class ParticipationStudyAcceptanceTest extends AcceptanceTest {


현재 방장과 참여자를 따로 관리하고 있기 때문에 방장이 본인이 생성한 스터디에 참여시 400 반환하는 테스트도 추가해야될것 같습니다.

좋습니다! 추가해보도록 하겠습니다~_~

tco0427 · 2022-07-28T05:58:23Z

backend/src/main/java/com/woowacourse/moamoa/study/controller/StudyController.java

+    public ResponseEntity<Void> participateStudy(@AuthenticationPrincipal final Long githubId,
+                                                 @PathVariable("study-id") final Long studyId
+    ) {
+        studyService.participantStudy(githubId, studyId);


participantStudy -> participateStudy

의견 : join, regist

tco0427 · 2022-07-28T06:04:00Z

backend/src/main/java/com/woowacourse/moamoa/study/domain/Participants.java

+        return size;
+    }
+
+    protected void participate(final Participant participant) {


default 접근 제어자가 좋을 것 같습니다.

tco0427 · 2022-07-28T06:05:39Z

backend/src/main/java/com/woowacourse/moamoa/study/domain/Participants.java

+    }
+
+    protected void participate(final Participant participant) {
+        checkParticipating(participant.getMemberId());


해당 메소드명으로 무엇을 check하는지 파악하기 힘들고, 추상화 수준도 맞지 않습니다.

tco0427 · 2022-07-28T06:18:00Z

.../java/com/woowacourse/moamoa/study/service/exception/InvalidParticipationStudyException.java

@@ -0,0 +1,8 @@
+package com.woowacourse.moamoa.study.service.exception;
+
+public class InvalidParticipationStudyException extends RuntimeException {


네이밍 변경

jaejae-yoo · 2022-07-29T02:39:15Z

backend/src/main/java/com/woowacourse/moamoa/study/domain/Participants.java

-        final boolean b = isOwner(memberId) || isParticipant(participant);
-        System.out.println("b = " + b);
-        return b;
+        return isOwner(memberId) || isParticipant(participant);


jaejae-yoo · 2022-07-29T02:43:26Z

backend/src/main/java/com/woowacourse/moamoa/study/domain/Participants.java

-    protected void participate(final Participant participant) {
-        checkParticipating(participant.getMemberId());
-
+    void participate(final Participant participant) {


sc0116

디우 피드백 반영 되게 빠르네요 이만 Approve할게요!

verus-j · 2022-07-29T04:22:19Z

backend/src/main/java/com/woowacourse/moamoa/study/domain/Study.java

+    public void participate(final Member member) {
+        final Long memberId = member.getId();
+        if (details.isCloseStatus() || period.isCloseEnrollment() || participants.isImpossibleParticipation(memberId)) {
+            throw new FailureParticipationException();
+        }
+
+        participants.participate(new Participant(memberId));
+    }


memberId로 파라미터를 받는 것이 좋을 것 같습니다.

tco0427 added 9 commits July 26, 2022 11:00

refactor: 네이밍 변경

8de6562

feat: StudyService 구현 및 테스트

40a88b4

Merge branch 'develop' of https://github.com/woowacourse-teams/2022-m…

593a4d3

…oamoa into feat/122-study-participation # Conflicts: # backend/src/main/java/com/woowacourse/moamoa/study/domain/Study.java

feat: Participants 검증 로직 및 참여자 추가 로직 구현

b0dc328

feat: Details CLOSE 확인 로직 추가

0f79069

feat: Period 모집 기간 확인 로직 추가

bbc410a

feat: Study 참여 로직 추가

9f0a622

feat: StudyService 회원 참여 기능 구현 및 생성 테스트 로직 추가

3df6cba

feat: StudyController 구현 및 테스트

5b6a5c5

tco0427 requested review from verus-j, sc0116 and jaejae-yoo July 26, 2022 08:21

tco0427 self-assigned this Jul 26, 2022

tco0427 added 🚀 feature New feature or request 🖥 backend New backend feature labels Jul 26, 2022

tco0427 added 5 commits July 26, 2022 17:24

fix: 스터디 생성 테스트 오류 수정

40a7434

fix: EnrollmentEndDate 는 null 일수 있으므로 이를 고려하여 검증 로직 수정

47b49dd

feat: 인수 테스트 작성

63a4378

fix: ParticipantsTest 수정

9291dd0

fix: owner는 participants 가 아니므로 제거

0634073

sc0116 reviewed Jul 28, 2022

View reviewed changes

tco0427 added 2 commits July 28, 2022 10:15

fix: isInvalidMemberSize 스터디 최대 인원수 가 없는 경우도 반영하도록 수정

72274e1

refactor: 마감일자 검증 로직 가독성 있게 변경

b1ec72f

jaejae-yoo requested changes Jul 28, 2022

View reviewed changes

verus-j reviewed Jul 28, 2022

View reviewed changes

tco0427 added 4 commits July 28, 2022 11:34

refactor: assertThatThrownBy().isInstnaceOf() 로 예외 타입 체크

90e9ec5

refactor: isAlreadyParticipation 메소드 내부 로직 메소드 분리

e482a95

refactor: 의미가 분명한 네이밍으로 변경

f7f8bd3

fix: 참여자에서 방장은 제외하도록 수정

5822471

tco0427 added 4 commits July 28, 2022 11:44

fix: 참여자수를 확인하는 테스트 코드 수정

78aafb4

refactor: checkParticipating 캡슐화

574b6ad

refactor: 스터디 참여 검증 로직 개선

6d34aee

test: 스터디장은 스터디에 참여할 수 없다.

8786486

tco0427 commented Jul 28, 2022

View reviewed changes

tco0427 added 2 commits July 29, 2022 09:37

refactor: Study 추상화 및 예외 네이밍 변경

d9c0b25

refactor: Boolean 네이밍 변경

eed9d4e

jaejae-yoo approved these changes Jul 29, 2022

View reviewed changes

sc0116 approved these changes Jul 29, 2022

View reviewed changes

tco0427 added 2 commits July 29, 2022 13:17

refactor: 메소드명 동사로 변경

4875818

refactor: participate 메소드 수정

7b78036

verus-j reviewed Jul 29, 2022

View reviewed changes

verus-j approved these changes Jul 29, 2022

View reviewed changes

fix: 깨지는 테스트 수정

4dce039

tco0427 merged commit d460942 into develop Jul 29, 2022

tco0427 deleted the feat/122-study-participation branch July 29, 2022 04:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE] issue122: 스터디 참여 기능 #132

[BE] issue122: 스터디 참여 기능 #132

tco0427 commented Jul 26, 2022 •

edited

Loading

sc0116 left a comment

sc0116 Jul 28, 2022

tco0427 Jul 28, 2022

sc0116 Jul 28, 2022

tco0427 Jul 28, 2022 •

edited

Loading

sc0116 Jul 28, 2022

tco0427 Jul 28, 2022

sc0116 Jul 28, 2022

jaejae-yoo left a comment

jaejae-yoo Jul 28, 2022

tco0427 Jul 28, 2022

jaejae-yoo Jul 28, 2022

jaejae-yoo Jul 28, 2022

tco0427 Jul 28, 2022

verus-j Jul 28, 2022

tco0427 Jul 28, 2022

verus-j Jul 28, 2022

tco0427 Jul 28, 2022

verus-j Jul 28, 2022

tco0427 Jul 28, 2022

verus-j Jul 28, 2022

tco0427 Jul 28, 2022

verus-j Jul 28, 2022

tco0427 Jul 28, 2022

verus-j Jul 28, 2022

tco0427 Jul 28, 2022

tco0427 Jul 28, 2022

tco0427 Jul 28, 2022

tco0427 Jul 28, 2022

tco0427 Jul 28, 2022

jaejae-yoo Jul 29, 2022

jaejae-yoo Jul 29, 2022

sc0116 left a comment

verus-j Jul 29, 2022

		@@ -0,0 +1,8 @@
		package com.woowacourse.moamoa.study.service.exception;

		public class InvalidParticipationStudyException extends RuntimeException {

[BE] issue122: 스터디 참여 기능 #132

[BE] issue122: 스터디 참여 기능 #132

Conversation

tco0427 commented Jul 26, 2022 • edited Loading

요약

세부 설명

sc0116 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tco0427 Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaejae-yoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sc0116 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tco0427 commented Jul 26, 2022 •

edited

Loading

tco0427 Jul 28, 2022 •

edited

Loading