[BE] issue 329: 링크 모음 패지키 이동 리팩토링 #350

verus-j · 2022-09-14T13:19:40Z

요약

reference room 패키지 study room으로 이동

세부사항

close #329

jaejae-yoo

베루스 안녕하세요! 코드 반영해 주신 내용 확인했어요!
이전보다 중복이 많이 줄었들었네요 👍. 고생하셨습니다.

그런데 전반적으로 코드의 변화도 많고, 제네릭이 많이 쓰이고 있어서 코드를 이해하는 데 어려운 것 같아요..
우선 리뷰 남겼는데, 답변 남겨주신 내용 바탕으로 조금 더 이해한 후에 다시 리뷰하겠습니다!

추가적으로 부탁드릴 부분이 있는데요.
혹시 이번 리팩터링으로 변경된 구조에 대해 PR 내용에 조금 더 설명을 부탁드려도 될까요? 구조에 대한 그림이나 설명이 있으면 조금 더 이해하는 데 수월할 것 같습니다!

또 개인적으로 Article과 Content가 서로를 참조하고 있는 구조인데, 서로 타입 간의 관계가 있다 보니 조금 복잡하게 느껴지는 것 같아요!
둘을 합칠 수 있으면 복잡도가 조금 내려갈 것 같은데, 어떻게 생각하시는지 궁금하네요!

jaejae-yoo · 2022-09-17T16:34:55Z

backend/src/main/java/com/woowacourse/moamoa/studyroom/domain/article/CommunityContent.java

+            throw new UneditableArticleException(studyRoom.getId(), accessor, CommunityArticle.class);
+        }
+
+        return new CommunityArticle(studyRoom, accessor.getMemberId(), this);


Article이 Content를 필드로 가지고 있는데, Content에서 Article 객체를 생성하고 있는 게 조금 어색한 것 같아요.

이건 만나서 얘기하시죠...
이거 말고는 방법이 없네요 ㅠㅠ

jaejae-yoo · 2022-09-17T16:45:25Z

backend/src/main/java/com/woowacourse/moamoa/studyroom/domain/article/Article.java

+
+@MappedSuperclass
+@NoArgsConstructor(access = AccessLevel.PROTECTED)
+public abstract class Article<T extends Content<? extends Article<T>>> extends BaseEntity {


Content와 Article의 관계에 대해서 설명해 주실 수 있을까요??

CommunityContent implements Content<CommunityArticle>

CommunityArticle extends Article<CommunityContent>
서로를 양방향으로 구현한 것 같이 보이는 데, 해당 구조에 대해서 설명해 주실 수 있나요?

금욜날 얘기해요~

jaejae-yoo · 2022-09-17T16:53:44Z

backend/src/main/java/com/woowacourse/moamoa/studyroom/service/AbstractArticleService.java

+        final StudyRoom studyRoom = studyRoomRepository.findByStudyId(studyId)
+                .orElseThrow(StudyNotFoundException::new);
+        final C content = articleRequest.createContent();
+        final A article = content.createArticle(studyRoom, new Accessor(memberId, studyId));


Content와 Article로 객체의 역할을 나누신 이유도 궁금해요!
둘을 합칠 수는 없을까요?

둘을 합치기가 전 힘들었습니다...
만나서 얘기하시죠

jaejae-yoo · 2022-09-17T17:00:42Z

backend/src/main/java/com/woowacourse/moamoa/studyroom/service/AbstractArticleService.java

+import org.springframework.transaction.annotation.Transactional;
+
+@Transactional
+public abstract class AbstractArticleService<A extends Article<C>, C extends Content<A>> {


community, notice, link 글들을 한 번에 처리할 수 있는 서비스를 만드셨군요! 👍
개인적으로 셋을 합침으로써 중복은 정말 많이 줄었들었지만, 제네릭이 많이 쓰이다 보니 코드가 조금 복잡하게 느껴지는 것 같아요..
고민이 되는 부분이네요... ㅠ

저도 고민입니다.... ㅠㅠㅠ
만나서 얘기하시죠

tco0427

안녕하세요 베루스! 많은양 리팩터링하시느라 고생하셨습니다!!

Generic을 활용하여 중복되는 CUD 로직을 공통화한 부분과 권한 체크하는 부분을 인터페이스로 풀어내는 부분도 인상깊었습니다!!

몇가지 코멘트 남겼습니다~_~ 확인 부탁드려요!!

backend/.gitignore

backend/src/docs/asciidoc/index.adoc

...n/java/com/woowacourse/moamoa/referenceroom/controller/SearchingReferenceRoomController.java

.../main/java/com/woowacourse/moamoa/study/domain/exception/NotParticipatedMemberException.java

...nd/src/main/java/com/woowacourse/moamoa/studyroom/controller/CommunityArticleController.java

backend/src/test/java/com/woowacourse/moamoa/studyroom/service/CommunityArticleServiceTest.java

backend/src/test/java/com/woowacourse/moamoa/studyroom/service/LinkArticleServiceTest.java

backend/src/test/java/com/woowacourse/moamoa/studyroom/service/NoticeArticleServiceTest.java

tco0427

리뷰 반영해주신 부분 확인하였습니다!!
다만, 피드백 반영 부분 확인하면서 코드를 다시 보니 생성부분에서 특히 article이 content에 의존하고 있다는 느낌을 받았는데요, 당연히 현재 content 내용에 의해서 article의 종류(?)가 달라지기 때문에 의존하는게 맞을 수 있다는 생각도 들면서 정말 여기에 의존하는게 맞을까? 하는 생각도 들었어요...ㅠ.ㅠ
한 번 다시 고민해볼 포인트라고 생각되어 코멘트로 남겨봅니다!

고생하셨습니다!!

tco0427

루스루스베! 빠르게 회의 내용 반영해서 구현해주셨군요..!!
고생하셨습니다!!👍👍

수정해야할 부분은 보이지 않았고, 궁금한 점이나 의견 몇가지 제시해보았습니다! Approve 할게요!

그런데 제가 잘 기억나지 않아서 그러는데..
당시 회의에서 나왔던 내용이 크게 Generic 을 제거하고 type 을 통해서 공지사항과 게시글을 구분한다. 이외에도 추가적으로 나왔던 내용이 있었을까요?? (테스트에서 private 메소드 이런 부분 제외하고 구현 상에서..기록 해놓지 않으니...기억이 가물가물..ㅠ.ㅠ)

tco0427 · 2022-10-04T22:02:18Z

backend/src/main/java/com/woowacourse/moamoa/studyroom/controller/ArticleController.java

-        final Article article = articleService.createArticle(id, studyId, request, type);
-        final URI location = URI.create("/api/studies/" + studyId + "/" + type.lowerName() + "/articles/" + article.getId());
+        final ArticleType type = ArticleType.valueOf(typeName.toUpperCase());
+        final Article article = articleService.createArticle(id, studyId, request.createContent(), type);


reqeust.createContent() 를 통해서 Content 도메인을 만들어 넘기고 있군요..!!

service로 reqeust만 넘기고 Response를 받는 것은 어떨까요??
풀어쓰면 다음과 같은 것 같아서..

final Content content = request.createContent(); final Article article = articleService.createArticle(id, studyId, content, type);

추가적으로 Article을 반환하는 것이 아닌 DTO 를 반환하는것은 어떤가요??

tco0427 · 2022-10-04T22:03:28Z

backend/src/main/java/com/woowacourse/moamoa/studyroom/service/LinkArticleService.java

+    }
+
+    @Transactional
+    public LinkArticle createArticle(final Long memberId, final Long studyId, final LinkContent content) {


어 혹시 create 시에는 도메인을 반환하도록 하는 특별한 이유가 있을까요?? 궁금합니다..!!

tco0427 · 2022-10-04T22:05:43Z

backend/src/main/java/com/woowacourse/moamoa/studyroom/domain/link/LinkArticle.java

+@Entity
+@Table(name = "link")
+@NoArgsConstructor(access = AccessLevel.PROTECTED)
+@Where(clause = "deleted = false")


JPA 로 조회시에 항상 where 절에 delete = false 가 추가된다고 이해하면 될까요??

tco0427 · 2022-10-04T22:06:21Z

backend/src/main/java/com/woowacourse/moamoa/studyroom/domain/article/Article.java

+    @GeneratedValue(strategy = GenerationType.IDENTITY)
+    private Long id;
+
+    @ManyToOne(fetch = FetchType.EAGER)


StudyRoom에 대해서 EAGER 로 해주신 이유를 공유해주시면 좋을 것 같습니다!

articleRepository.findById() 시에 article, study, study_member테이블에 대해서 각각 쿼리를 날리는 것이 아닌 한 번에 가져오기 위함이라고 이해하면 될까요??

tco0427 · 2022-10-04T22:08:50Z

backend/src/main/java/com/woowacourse/moamoa/studyroom/query/ArticleDao.java

-        final String sql = "SELECT {}.id as article_id, {}.title as article_title, {}.content as article_content, "
-                + "{}.created_date as article_created_date, {}.last_modified_date as article_last_modified_date, "
+    public Optional<ArticleData> getById(final Long articleId, final ArticleType type) {
+        final String sql = "SELECT article.id as article_id, article.title as article_title, article.content as article_content, "
+                + "article.created_date as article_created_date, article.last_modified_date as article_last_modified_date, "


쿼리를 변경하면서 {} 를 제거해주셨군요..!! 쿼리 가독성이 훨씬 좋아진 것 같아요!👍

tco0427 · 2022-10-04T22:22:53Z

backend/src/test/java/com/woowacourse/moamoa/studyroom/service/ArticleServiceTest.java

+    @ParameterizedTest
+    @EnumSource(ArticleType.class)


오..이렇게 Enum 상수를 parameterized 로 테스트할 수 있군요! 하나 배워갑니다~_~

tco0427 · 2022-10-04T22:24:10Z

backend/src/test/resources/schema.sql

-    deleted boolean NOT NULL,
+    id                 BIGINT PRIMARY KEY AUTO_INCREMENT,
+    study_id           BIGINT     NOT NULL,
+    author_id          BIGINT     NOT NULL,


tco0427 · 2022-10-04T22:24:29Z

backend/src/test/resources/schema.sql

+    type               VARCHAR(255) NOT NULL,
+    deleted            boolean      NOT NULL,


tco0427 · 2022-10-04T22:26:59Z

backend/src/main/java/com/woowacourse/moamoa/studyroom/controller/ArticleController.java

-@RequestMapping("api/studies/{study-id}/{article-type}/articles")
+@RequestMapping("api/studies/{study-id}/{type}/articles")


API가 길어지면서 articles 는 제거해도 괜찮지 않을까 의견 제시해봅니다!!
api/studies/1/community/4, api/studies/1/notice/5

tco0427 · 2022-10-04T22:27:43Z

backend/src/main/java/com/woowacourse/moamoa/studyroom/controller/LinkArticleController.java

 @RequestMapping("/api/studies/{study-id}/reference-room/links")
-@RequiredArgsConstructor
-public class ReferenceRoomController {
+public class LinkArticleController {


Review의 경우에도 /api/studies/4/links/5 와 같이 reference-room 을 생략해도 자원을 명시하는데 문제가 없어 보여 어떤지 의견 제시해봅니다!

jaejae-yoo · 2022-10-04T23:26:23Z

backend/src/test/resources/schema.sql

@@ -120,6 +107,8 @@ CREATE TABLE notice
    study_id           BIGINT,
    created_date       DATETIME     not null,
    last_modified_date DATETIME     not null,
+    type               VARCHAR(255) NOT NULL,


article 테이블로 통합 및 type 추가 👍

jaejae-yoo · 2022-10-04T23:27:51Z

backend/src/main/java/com/woowacourse/moamoa/studyroom/domain/article/Article.java

+    private Content content;
+
+    @Enumerated(EnumType.STRING)
+    private ArticleType type;


jaejae-yoo · 2022-10-04T23:28:40Z

backend/src/main/java/com/woowacourse/moamoa/studyroom/domain/article/Article.java

+    }
+
+    public void update(final Accessor accessor, final Content content) {
+        if (type.isUneditableAccessor(studyRoom, authorId, accessor)) {


해당 방법으로 푸니 로직이 훨씬 간결해졌네요 👍

jaejae-yoo · 2022-10-05T00:36:31Z

backend/src/main/java/com/woowacourse/moamoa/studyroom/domain/article/ArticleType.java

+
+        @Override
+        boolean isUneditableAccessor(final StudyRoom studyRoom, final Long authorId, final Accessor accessor) {
+            return !(studyRoom.isPermittedAccessor(accessor) && authorId.equals(accessor.getMemberId()));


Accessor 객체의 역할이 궁금해요 ~

authorId.equals(accessor.getMemberId()를 Accessor에 메시지를 보내도 좋을 것 같아용

jaejae-yoo · 2022-10-05T01:26:24Z

backend/src/test/java/com/woowacourse/moamoa/studyroom/domain/link/LinkArticleTest.java

+        final StudyRoom studyRoom = createStudyRoom(owner);
+        final LinkArticle sut = createLinkArticle(owner, studyRoom);
+
+        sut.delete(new Accessor(1L, 1L));


new Accessor(OWNER_ID, STUDY_ID);
new Accessor(owner.getId(), studyRoom.getId());
new Accessor(1L, 1L);

통일하면 좋을 것 같아요.
여러 곳에서 같은 값이 쓰이면 필드로 빼는 게 관리하기 더 좋을 것 같아요.

2022-moamoa · 2022-10-06T05:48:00Z

Analysis Details

15 Issues

0 Bugs
0 Vulnerabilities
15 Code Smells

Coverage and Duplications

No coverage information (68.80% Estimated after merge)
No duplication information (0.40% Estimated after merge)

Project ID: woowacourse-teams_2022-moamoa_AYKvd_z4VbW_bWBvgn13

View in SonarQube

verus-j added 14 commits September 13, 2022 14:59

refactor: referenceroom 패키지 studyroom 패키지로 이동

f90aa58

refactor: LinkArticle 클래스 이름 수정 및 필드 타입 변경

b0a3ec3

refactor: LinkArticle 컨트롤러, 서비스 클래스명 변경 및 EditingLinkRequest 제거

a72a3c1

refactor: LinkArticle update 메서드 변경

56bffea

refactor: LinkArticle delete 메서드 정리

5de05b1

refactor: LinkNotFoundException 제거

8477653

refactor: 링크 게시글 조회 관련 서비스 제거

66a45f5

test: LinkArticleControllerTest setUp 로직 제거

970fcb8

refactor: 링크 게시글 목록 조회 시 Authentication URI에서 제거

2d360cc

refactor: Community와 Notice 컨트롤러, 서비스 분리

4d154ca

refactor: ArticleRepositoryFactory 제거

96c6595

refactor: Article 및 조회 시 인가 기능 제거

89c0efa

refactor: NoticeArticle 소프트 delete 적용

6347b1e

refactor: CommunityArticle 소프트 delete 적용

9fa1459

verus-j self-assigned this Sep 14, 2022

verus-j added 🖥 backend New backend feature 🛠 refactoring Refactor code labels Sep 14, 2022

This comment has been minimized.

Sign in to view

nan-noo linked an issue Sep 15, 2022 that may be closed by this pull request

[BE] 링크모음 리팩토링 #329

Closed

verus-j added 2 commits September 15, 2022 17:06

chore: 충돌 해결

cc91eeb

refactor: 사용하지 않는 메서드 정리 및 예외 패키지 이동

b24da71

This comment has been minimized.

Sign in to view

refactor: 사용하지 않는 코드 제거

7b40834

This comment has been minimized.

Sign in to view

verus-j requested review from jaejae-yoo, tco0427 and sc0116 September 15, 2022 08:38

verus-j added 2 commits September 15, 2022 17:41

refactor: import 문 정리 및 테스트 클래스 default 접근자 설정

8231c45

chore: index.html gitignore 설정

689fbf6

This comment has been minimized.

Sign in to view

test: 테스트 코드 정리

907c2e3

This comment has been minimized.

Sign in to view

jaejae-yoo requested changes Sep 17, 2022

View reviewed changes

tco0427 requested changes Sep 20, 2022

View reviewed changes

verus-j added 5 commits September 29, 2022 21:20

refactor: 커뮤니티 게시글 삭제 URL 수정 및 Article 생성자 접근 제어자 수정

c450b64

refactor: LinkArticle 관련 테스트 가독성 정리

bba8852

test: CommunityArticle 테스트 가독성 정리

8a16f87

test: NoticeArticle 테스트 가독성 정리

5e205cf

chore: develop 브랜치 충돌 해결

45afd56

This comment has been minimized.

Sign in to view

refactor: Article 구조 수정

aedff89

This comment has been minimized.

Sign in to view

tco0427 approved these changes Sep 30, 2022

View reviewed changes

verus-j added 6 commits October 1, 2022 23:37

refactor: LinkArticle 상속 구조 제거

8d94025

refactor: NoticeArtice 상속 구조 제거

870c641

refactor: CommunityArticle 상속 구조 제거

0e4a4de

refactor: ArticleType을 사용한 구분

547adcd

refactor: ArticleType Enum 정리

3f4e9f5

refactor: 도메인 패키지 정리

ee2e850

verus-j requested review from jaejae-yoo and tco0427 October 4, 2022 10:11

tco0427 approved these changes Oct 4, 2022

View reviewed changes

jaejae-yoo approved these changes Oct 5, 2022

View reviewed changes

verus-j added 2 commits October 6, 2022 13:10

refactor: 피드백 반영

81aa170

chore: 충돌 해결

445f0b3

This comment has been minimized.

Sign in to view

refactor: 패키지 정리

6bb7d06

verus-j merged commit 90cd179 into develop Oct 6, 2022

tco0427 deleted the refactor/329-reference-room branch October 6, 2022 10:42

		@RequestMapping("api/studies/{study-id}/{article-type}/articles")
		@RequestMapping("api/studies/{study-id}/{type}/articles")

[BE] issue 329: 링크 모음 패지키 이동 리팩토링 #350

[BE] issue 329: 링크 모음 패지키 이동 리팩토링 #350

Conversation

verus-j commented Sep 14, 2022 • edited Loading

요약

세부사항

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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

jaejae-yoo Sep 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tco0427 left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

tco0427 left a comment

Choose a reason for hiding this comment

tco0427 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

This comment has been minimized.

2022-moamoa bot commented Oct 6, 2022

Analysis Details

15 Issues

Coverage and Duplications

verus-j commented Sep 14, 2022 •

edited

Loading

jaejae-yoo Sep 17, 2022 •

edited

Loading