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.

Sign up for GitHub

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

Jump to bottom

[BE] 서비스 중복 로직 제거 및 캡슐화 #724

Merged
merged 5 commits into from
Nov 16, 2023
Merged

[BE] 서비스 중복 로직 제거 및 캡슐화 #724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
560d1c5
refactor: `ContentService` 중복 로직 제거 및 도메인 로직 캡슐화
woosung1223 Nov 7, 2023
0d32863
refactor: 서비스 로직 개선 및 메소드명을 이해하기 쉽게 변경
woosung1223 Nov 8, 2023
d8983f9
refactor: repository 스타일 일관화
woosung1223 Nov 8, 2023
52770a7
chore: `@Transactional` readonly 설정
woosung1223 Nov 12, 2023
d24537a
refactor: 중복 검증 로직 제거
woosung1223 Nov 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 27 additions & 63 deletions backend/src/main/java/harustudy/backend/content/service/ContentService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package harustudy.backend.content.service;

import harustudy.backend.auth.dto.AuthMember;
import harustudy.backend.auth.exception.AuthorizationException;
import harustudy.backend.content.domain.Content;
import harustudy.backend.content.dto.ContentResponse;
import harustudy.backend.content.dto.ContentsResponse;
Expand All @@ -12,18 +11,14 @@
import harustudy.backend.member.domain.Member;
import harustudy.backend.member.repository.MemberRepository;
import harustudy.backend.participant.domain.Participant;
import harustudy.backend.participant.domain.Step;
import harustudy.backend.participant.exception.StudyStepException;
import harustudy.backend.participant.exception.ParticipantNotBelongToStudyException;
import harustudy.backend.participant.repository.ParticipantRepository;
import harustudy.backend.study.domain.Study;
import harustudy.backend.study.repository.StudyRepository;
import jakarta.annotation.Nullable;
import java.util.List;
import java.util.Objects;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
import java.util.List;
import java.util.Objects;

@RequiredArgsConstructor
@Transactional
Expand All @@ -36,33 +31,26 @@ public class ContentService {
private final ContentRepository contentRepository;

@Transactional(readOnly = true)
public ContentsResponse findContentsWithFilter(
AuthMember authMember, Long studyId, Long participantId, @Nullable Integer cycle) {
public ContentsResponse findContentsWithFilter(AuthMember authMember, Long studyId,
Long participantId, Integer cycle) {
Participant participant = participantRepository.findByIdIfExists(participantId);
validateParticipantIsCreatedByMember(participant, authMember.id());
validateParticipantBelongsToStudy(participant, studyId);
Study study = studyRepository.findByIdIfExists(studyId);
Member member = memberRepository.findByIdIfExists(authMember.id());

participant.validateIsCreatedByMember(member);
participant.validateIsBelongsTo(study);

return getContentsResponseByCycleFilter(cycle, participant);
}

private ContentsResponse getContentsResponseByCycleFilter(Integer cycle, Participant participant) {
List<Content> contents = participant.getContents();
if (Objects.isNull(cycle)) {
return getContentsResponseWithoutCycleFilter(contents);
}
return getContentsResponseWithCycleFilter(contents, cycle);
}

private void validateParticipantIsCreatedByMember(Participant participant, Long memberId) {
Member member = memberRepository.findByIdIfExists(memberId);
if (participant.isNotCreatedBy(member)) {
throw new AuthorizationException();
}
}

private void validateParticipantBelongsToStudy(Participant participant, Long studyId) {
Study study = studyRepository.findByIdIfExists(studyId);
if (!participant.isParticipantOf(study)) {
throw new AuthorizationException();
}
}

private ContentsResponse getContentsResponseWithoutCycleFilter(List<Content> contents) {
List<ContentResponse> contentResponses = contents.stream()
.map(ContentResponse::from)
Expand All @@ -81,59 +69,35 @@ private ContentsResponse getContentsResponseWithCycleFilter(List<Content> conten
public void writePlan(AuthMember authMember, Long studyId, WritePlanRequest request) {
Study study = studyRepository.findByIdIfExists(studyId);
Participant participant = participantRepository.findByIdIfExists(request.participantId());
Member member = memberRepository.findByIdIfExists(authMember.id());

validateParticipantBelongsToStudy(study, participant);
validateParticipantIsCreatedByMember(authMember.id(), participant);
validateStudyIsPlanning(study);
participant.validateIsBelongsTo(study);
participant.validateIsCreatedByMember(member);
study.validateIsPlanning();
Copy link
Collaborator

Choose a reason for hiding this comment

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

훨씬 좋네요 굳굳


Content recentContent = findContentWithSameCycle(study, participant);
recentContent.changePlan(request.plan());
}

private void validateParticipantBelongsToStudy(Study study, Participant participant) {
if (!participant.isParticipantOf(study)) {
throw new ParticipantNotBelongToStudyException();
}
}

private void validateStudyIsPlanning(Study study) {
if (!study.isStep(Step.PLANNING)) {
throw new StudyStepException();
}
}

private Content findContentWithSameCycle(Study study, Participant participant) {
List<Content> contents = contentRepository.findByParticipant(
participant);

return contents.stream()
.filter(content -> content.hasSameCycleWith(study))
.findAny()
.orElseThrow(ContentNotFoundException::new);
}

public void writeRetrospect(AuthMember authMember, Long studyId, WriteRetrospectRequest request) {
Study study = studyRepository.findByIdIfExists(studyId);
Participant participant = participantRepository.findByIdIfExists(request.participantId());
Member member = memberRepository.findByIdIfExists(authMember.id());

validateParticipantBelongsToStudy(study, participant);
validateParticipantIsCreatedByMember(authMember.id(), participant);
validateStudyIsRetrospect(study);
participant.validateIsBelongsTo(study);
participant.validateIsCreatedByMember(member);
study.validateIsRetrospect();

Content recentContent = findContentWithSameCycle(study, participant);
recentContent.changeRetrospect(request.retrospect());
}

private void validateParticipantIsCreatedByMember(Long memberId, Participant participant) {
Member member = memberRepository.findByIdIfExists(memberId);
if (participant.isNotCreatedBy(member)) {
throw new AuthorizationException();
}
}
private Content findContentWithSameCycle(Study study, Participant participant) {
List<Content> contents = contentRepository.findByParticipant(participant);

private void validateStudyIsRetrospect(Study study) {
if (!study.isStep(Step.RETROSPECT)) {
throw new StudyStepException();
}
return contents.stream()
.filter(content -> content.hasSameCycleWith(study))
.findAny()
.orElseThrow(ContentNotFoundException::new);
}
}
14 changes: 14 additions & 0 deletions backend/src/main/java/harustudy/backend/participant/domain/Participant.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package harustudy.backend.participant.domain;

import harustudy.backend.auth.exception.AuthorizationException;
import harustudy.backend.common.BaseTimeEntity;
import harustudy.backend.content.domain.Content;
import harustudy.backend.member.domain.Member;
import harustudy.backend.participant.exception.NicknameLengthException;
import harustudy.backend.participant.exception.ParticipantIsNotHostException;
import harustudy.backend.participant.exception.ParticipantNotBelongToStudyException;
import harustudy.backend.study.domain.Study;
import jakarta.persistence.CascadeType;
import jakarta.persistence.Entity;
Expand Down Expand Up @@ -95,4 +97,16 @@ public void validateIsHost() {
throw new ParticipantIsNotHostException();
}
}

public void validateIsBelongsTo(Study study) {
if (!this.study.getId().equals(study.getId())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

(논란 투척)
직접 id 꺼내서 비교하기 보다 this.study.equals(study)로 비교하는건 어떤가요...?
엔티티의 equals를 어떻게 정의하냐가 논쟁이 많은 주제인것 같은데
좋은 글들도 많이 있더라구요
https://blog.yevgnenll.me/posts/jpa-entity-eqauls-and-hashcode-equality
영한님 강의에서는 비즈니스키로 equals를 재정의할것이 좋다고 하셨었는데, study에는 비즈니스키가 딱히 없는것 같기도 해서 pk로 equals 재정의 하고 id가 null이면 false를 반환하면 괜찮다고 생각합니다.
이렇게 했을때 가능하면 엔티티를 비교하거나 컬렉션을 사용할 때 영속상태의 엔티티로 사용해야 함을 인지해야겠죠.

Copy link
Collaborator

Choose a reason for hiding this comment

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

좋은 아티클 추천 감사합니다 마코! 덕분에 잊고 지내던 동등성에 대해서 다시 한 번 생각할 수 있게 되었네요 ㅎㅎ

Study 엔티티에서 id를 제외하면 다른 필드를 가지고 Study 엔티티를 고유하게 판단할 수 있는 기준으로 잡을 수 있는 비즈니스 키를 도출하기 어렵겠다는 데는 같은 생각입니다. 같은 이유로 PK 이외의 필드들로 엔티티의 고유성을 부여하는 것도 어려우니, 선택지가 PK밖에 없을 것 같아요.

사실 Set과 같은 컬렉션을 사용할 때 해당 자료구조의 동작 원리가 깨질 수 있다는 건 비영속화된 엔티티를 영속화된 엔티티와 함께 사용하기 때문이라고 생각해요. 그리고 비영속화된 엔티티를 비즈니스 로직을 처리하는데 사용해야 된다고 한다면 VO로서 사용되어야 한다는 입장입니다. 엔티티와 VO로 나누어 생각한다면 이들을 함께 컬렉션에 담아 사용하지는 않을 것 같아요!

저희가 프로젝트 리팩토링의 방향을 좀 더 객체지향스러운 쪽으로 진행하고자 한다면 마코가 제시해주신 방향대로 구현하는게 맞을 것 같아요. 추후 저희가 컬렉션을 사용해야 하는 상황이 온다면 VO로 변환해서 해당 로직을 수행하도록 하면 될 것이고요.

몇달 전에 했었던 지하철 미션이 생각나네요 ㅎㅎ...
개인적인 생각들을 정리해봤는데 다른 크루들의 의견도 궁금합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오 오랜만에 레벨1의 영광을 다시 느끼는 기분이군요..👍

말씀해주신대로 id 값을 기준으로 동등성을 비교하느냐 내부 필드를 기준으로 동등성을 비교하느냐는 객체의 성격에 따라 나뉠 거 같습니다!

제 기억으로는 코드 곳곳에 이런 동등성 비교가 id로 이루어졌던 것 같아 개편이 필요하긴 한 것 같습니다.

개인적으로는 조만간 도메인 격벽 분리 & 이벤트로 의존성 개선을 해야 할 것 같은데 해당 이슈 미리 파두고 한번에 개선하면 좋을 것 같은데 어떻게 생각하시나요?

만약 이번 PR에서 개선해야 한다면 추가적인 논의가 필요한 것 같은데, 이는 만나서 이야기해봐야 할 것 같네요!

Copy link
Collaborator

@MoonJeWoong MoonJeWoong Nov 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

위에 polling 관련 메서드 네이밍 관련한 이슈도 그렇고 따로 PR을 만들어 거기에서 해결해보는 방향이 좋겠네요!

일단 이슈 먼저 파뒀습니다!

throw new ParticipantNotBelongToStudyException();
}
}

public void validateIsCreatedByMember(Member member) {
if (isNotCreatedBy(member)) {
throw new AuthorizationException();
}
}
}
7 changes: 2 additions & 5 deletions backend/src/main/java/harustudy/backend/participant/repository/ParticipantRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,13 @@
import harustudy.backend.participant.domain.Participant;
import harustudy.backend.participant.exception.ParticipantNotFoundException;
import harustudy.backend.study.domain.Study;
import org.springframework.data.jpa.repository.JpaRepository;
import java.util.List;
import java.util.Optional;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.Query;
import org.springframework.data.repository.query.Param;

public interface ParticipantRepository extends JpaRepository<Participant, Long> {

Optional<Participant> findByStudyAndMember(Study study,
Member member);
Optional<Participant> findByStudyAndMember(Study study, Member member);

List<Participant> findByStudy(Study study);

Expand Down
101 changes: 33 additions & 68 deletions backend/src/main/java/harustudy/backend/participant/service/ParticipantService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,16 @@
import harustudy.backend.member.domain.Member;
import harustudy.backend.member.repository.MemberRepository;
import harustudy.backend.participant.domain.Participant;
import harustudy.backend.participant.domain.Step;
import harustudy.backend.participant.dto.ParticipateStudyRequest;
import harustudy.backend.participant.dto.ParticipantResponse;
import harustudy.backend.participant.dto.ParticipantsResponse;
import harustudy.backend.participant.exception.ParticipantNotBelongToStudyException;
import harustudy.backend.participant.dto.ParticipateStudyRequest;
import harustudy.backend.participant.exception.ParticipantNotFoundException;
import harustudy.backend.participant.repository.ParticipantRepository;
import harustudy.backend.study.domain.Study;
import harustudy.backend.study.exception.StudyAlreadyStartedException;
import harustudy.backend.study.repository.StudyRepository;
import jakarta.annotation.Nullable;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

import java.util.List;
import java.util.Objects;

Expand All @@ -33,32 +28,31 @@ public class ParticipantService {
private final StudyRepository studyRepository;

@Transactional(readOnly = true)
public ParticipantResponse findParticipant(
AuthMember authMember, Long studyId, Long participantId
) {
public ParticipantResponse findParticipant(AuthMember authMember, Long studyId, Long participantId) {
Study study = studyRepository.findByIdIfExists(studyId);
Participant participant = participantRepository.findByIdIfExists(participantId);
validateParticipantIsRelatedWith(participant, authMember, study);
Member member = memberRepository.findByIdIfExists(authMember.id());

participant.validateIsCreatedByMember(member);
participant.validateIsBelongsTo(study);

return ParticipantResponse.from(participant);
}

// TODO: 임시용이므로 이후에 제거
public ParticipantsResponse tempFindParticipantWithFilter(
AuthMember authMember, Long studyId, Long memberId
) {
@Transactional(readOnly = true)
public ParticipantsResponse tempFindParticipantWithFilter(AuthMember authMember, Long studyId, Long memberId) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

요거 temp 메서드 왜 생겼었는지 또 기억이 안나네요 ㅎㅎ...
그건 차치하고 당장 제거하지 않을 거면 아래 findParticipantWithFilter 메서드처럼 읽기 전용 트랜잭션 설정해주는 것은 어떤가욤?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오 여기 readonly가 빠졌었네요!

굿굿 바로 변경했습니다!

Study study = studyRepository.findByIdIfExists(studyId);
if (Objects.isNull(memberId)) {
validateEverParticipated(authMember, study);
return getParticipantsResponseWithoutMemberFilter(study);
}
Member member = memberRepository.findByIdIfExists(memberId);
validateIsSameMemberId(authMember, memberId);
Member member = memberRepository.findByIdIfExists(memberId);
return tempGetParticipantsResponseWithMemberFilter(study, member);
}

// TODO: 임시용이므로 이후에 제거
private ParticipantsResponse tempGetParticipantsResponseWithMemberFilter(
Study study, Member member) {
private ParticipantsResponse tempGetParticipantsResponseWithMemberFilter(Study study, Member member) {
return participantRepository.findByStudyAndMember(study, member)
.map(ParticipantResponse::from)
.map(response -> ParticipantsResponse.from(List.of(response)))
Expand All @@ -67,16 +61,14 @@ private ParticipantsResponse tempGetParticipantsResponseWithMemberFilter(

// TODO: 동적쿼리로 변경(memberId 유무에 따른 분기처리)
@Transactional(readOnly = true)
public ParticipantsResponse findParticipantsWithFilter(
AuthMember authMember, Long studyId, @Nullable Long memberId
) {
public ParticipantsResponse findParticipantsWithFilter(AuthMember authMember, Long studyId, Long memberId) {
Study study = studyRepository.findByIdIfExists(studyId);
if (Objects.isNull(memberId)) {
validateEverParticipated(authMember, study);
return getParticipantsResponseWithoutMemberFilter(study);
}
Member member = memberRepository.findByIdIfExists(memberId);
validateIsSameMemberId(authMember, memberId);
Member member = memberRepository.findByIdIfExists(memberId);
return getParticipantsResponseWithMemberFilter(study, member);
}

Expand All @@ -86,36 +78,32 @@ private void validateEverParticipated(AuthMember authMember, Study study) {
.orElseThrow(AuthorizationException::new);
}

private ParticipantsResponse getParticipantsResponseWithoutMemberFilter(
Study study
) {
List<ParticipantResponse> responses =
participantRepository.findByStudy(study)
.stream()
.map(ParticipantResponse::from)
.toList();
private ParticipantsResponse getParticipantsResponseWithoutMemberFilter(Study study) {
List<ParticipantResponse> responses = participantRepository.findByStudy(study)
.stream()
.map(ParticipantResponse::from)
.toList();
return ParticipantsResponse.from(responses);
}

private ParticipantsResponse getParticipantsResponseWithMemberFilter(
Study study, Member member
) {
ParticipantResponse response =
participantRepository.findByStudyAndMember(study, member)
.map(ParticipantResponse::from)
.orElseThrow(ParticipantNotFoundException::new);
private ParticipantsResponse getParticipantsResponseWithMemberFilter(Study study, Member member) {
ParticipantResponse response = participantRepository.findByStudyAndMember(study, member)
.map(ParticipantResponse::from)
.orElseThrow(ParticipantNotFoundException::new);
return ParticipantsResponse.from(List.of(response));
}

public Long participateStudy(AuthMember authMember, Long studyId, ParticipateStudyRequest request) {
Member member = memberRepository.findByIdIfExists(request.memberId());
validateIsSameMemberId(authMember, request.memberId());
Member member = memberRepository.findByIdIfExists(request.memberId());
Study study = studyRepository.findByIdIfExists(studyId);
validateIsWaiting(study);

study.validateIsWaiting();
Participant participant = Participant.createParticipantOfStudy(study, member, request.nickname());
participant.generateContents(study.getTotalCycle());
Participant saved = participantRepository.save(participant);
return saved.getId();
participantRepository.save(participant);

return participant.getId();
}

private void validateIsSameMemberId(AuthMember authMember, Long memberId) {
Expand All @@ -124,37 +112,14 @@ private void validateIsSameMemberId(AuthMember authMember, Long memberId) {
}
}

private void validateIsWaiting(Study study) {
if (!study.isStep(Step.WAITING)) {
throw new StudyAlreadyStartedException();
}
}

private void validateParticipantIsRelatedWith(
Participant participant, AuthMember authMember, Study study
) {
validateMemberOwns(participant, authMember);
validateParticipantIsIncludedIn(study, participant);
}

private void validateMemberOwns(Participant participant, AuthMember authMember) {
Member member = memberRepository.findByIdIfExists(authMember.id());
if (participant.isNotCreatedBy(member)) {
throw new AuthorizationException();
}
}

private void validateParticipantIsIncludedIn(Study study, Participant participant) {
if (participant.isNotIncludedIn(study)) {
throw new ParticipantNotBelongToStudyException();
}
}

public void deleteParticipant(AuthMember authMember, Long studyId, Long participantId) {
Study study = studyRepository.findByIdIfExists(studyId);
validateEverParticipated(authMember, study);
Member member = memberRepository.findByIdIfExists(authMember.id());
Participant participant = participantRepository.findByIdIfExists(participantId);
validateParticipantIsRelatedWith(participant, authMember, study);

participant.validateIsCreatedByMember(member);
participant.validateIsBelongsTo(study);

participantRepository.delete(participant);
}
}
2 changes: 1 addition & 1 deletion backend/src/main/java/harustudy/backend/polling/controller/PollingController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public class PollingController {
@Operation(summary = "진행 페이지 폴링")
@GetMapping("/api/v2/progress")
public ResponseEntity<ProgressResponse> progressPolling(@RequestParam Long studyId) {
ProgressResponse progressResponse = pollingService.pollProgress(studyId);
ProgressResponse progressResponse = pollingService.pollInProgress(studyId);
return ResponseEntity.ok(progressResponse);
}

Expand Down
Loading