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

[#16] [FEATURE] 게시글 기능 구현 #39

Merged
merged 8 commits into from
Sep 30, 2024
Merged

[#16] [FEATURE] 게시글 기능 구현 #39

merged 8 commits into from
Sep 30, 2024

Conversation

Anyeon00
Copy link
Collaborator

✨ 구현 기능 명세

  • 게시글 등록
  • 지역 별 게시글 목록 조회
  • 내 게시글 목록 조회
  • 게시글 상세 조회
  • 게시글 수정
  • 게시글 삭제

✅ PR Point

ToDo

User 클래스와 충돌 해결

User 클래스에 연관관계 추가로 인한 충돌이 예상됩니다.

  • 필드 (List<Post>)
  • 연관관계 편의 메서드

@AuthenticationPrincipal로 변경 필요

SpringSecurity 의존성이 없는 관계로, 임시로 RequestParam을 이용하여 구현하였습니다.

  • 게시글 등록 api
  • 내 게시글 조회 api

내 게시글 목록 조회 Api 이동 필요

우선 PostApi에 구현해 놓았습니다. Api 설계에 따라 유저 관련 Api 패키지로 이동이 필요합니다.
(ResponseDTO와 함께 이동이 필요합니다.)

기타사항

PostService가 사용하는 Component에서, User패키지의 UserNotFoundException을 사용했습니다.

😭 어려웠던 점

사전에 이야기했던 코드 컨벤션 가능한 지키며 작성하려고 노력해보았습니다. (2차 프로젝트 메모)
그래도 모두 처음 적용해본거라 고칠 부분이 많지 않을까 싶습니다. 또 동시에 그동안 얼마나 코드를 막 짜고 있었는지 알게 되었습니다... 피드백 주시면 감사드리겠습니다.

중간에 rebase관련 이슈로 인해 작업내용이 초기화되어서...... 다소 지연되었습니다.(정말 죄송합니다ㅜㅜㅜ)

페이징 포함하여 n+1 문제 개선은 마지막에 리팩토링 예정입니다!

@Anyeon00 Anyeon00 requested a review from a team September 29, 2024 10:10
@Anyeon00 Anyeon00 self-assigned this Sep 29, 2024
@s0o0bn
Copy link
Collaborator

s0o0bn commented Sep 29, 2024

고생하셨습니다 !!! 🙌

페이징이나 엔티티들 엮여 있는 게 있어서
은근 까다로운 부분이 많았을 거 같은데 잘하셨네용

그 와중에 작업한 것도 날라갔다니 제가 다 억장이 무너집니다,,,

그 도메인 모듈에서 service나 repository 등 패키지를 분리하신 부분에 관해서
저도 확실하진 않지만 나름의 궁예를 해봤을 때, 저희 아키텍처 특성 상 클래스가 되게 많아지는데
패키지를 구분하게 되면 같은 도메인 내의 클래스들에서 import가 많아질 수밖에 없을 거 같아요

뭔가 그런 부분을 최소화하고자 도메인 모듈에서 별도의 패키지 분리를 안한게 아닌가라는 생각이 듭니당
이게 뭔가 문제가 된다거나 하진 않을 거 같긴 한데, 이 부분은 저희가 통일하는 게 좋지 않을까요?

아무튼 진짜 고생 많으셨습니다 !!

@Anyeon00 Anyeon00 force-pushed the feature/posts branch 2 times, most recently from 3986854 to a3c7a9c Compare September 29, 2024 11:41
import com.nbe2.domain.posts.entity.City;
import com.nbe2.domain.posts.service.dto.LocalPostPageCommand;

public record LocalPostPageRequest(City city, Integer page, Integer size) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

페이징 관련 공통으로 사용되는 파라미터(page, size)는
common 모듈에 Page로 분리해서 사용할 수 있게 추가해 둔 게 있습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

record안에 record를 사용하면 @ModelAttribute로 매핑이 안되는 문제가 있어서 primitive 타입으로 받았습니닷

Copy link
Collaborator

Choose a reason for hiding this comment

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

아 엇 아뇨 레코드 안에 말구

Api에서 파라미터로 City를 받고 그거랑 같이 Page@PageDefault로 받는 건 어떠신가요
getMyPostPage 에서 하신 방식이랑 동일하게요!

제가 코드리뷰를 처음 해봐서 코드를 수정해서 여기다 보이게 하는 법을 모르겠네용...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

정보를 dto객체로 묶어서 받고 싶어서 이렇게 작성했었는데, @PageDefault를 사용해서 받는게 더 좋겠군요! 아래 내용들까지 일괄적으로 수정한 후에 전체코멘트?로 남기겠습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

저희 그러고보니 서비스에 인터페이스를 따로 둘 건지도 논의해서 통일하는 게 좋겠네용

User user =
userRepository
.findByEmail(command.email())
.orElseThrow(() -> UserNotFoundException.EXCEPTION);
Copy link
Collaborator

Choose a reason for hiding this comment

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

회원 조회 목적으로 UserReader에 이미 같은 로직이 있어서
여기서 userRepository 로 조회해서 쓰기 보다는

서비스 클래스에서 userReader로 조회한 뒤에
여기로 User를 넘겨주는 건 어떨까요?

Copy link
Collaborator Author

@Anyeon00 Anyeon00 Sep 29, 2024

Choose a reason for hiding this comment

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

저도 고민했던 부분이었습니다. 같은 레벨의 레이어를 의존하기 보다 하위 레이어를 사용해야 하지 않을까 하는 생각에 이렇게 구현하긴 했는데, 잘 모르겠네요. 구조적인 관점에서 큰 문제가 없을까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

PostSerivce에서 UserReader로 조회하고
UserPostAppender로 전달해주면 괜찮을 거 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아하 그런방법이... 수정해서 반영하도록 하겠습니다!

User user =
userRepository
.findByEmail(command.email())
.orElseThrow(() -> UserNotFoundException.EXCEPTION);
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기두용


// ** 연관관계 편의 메서드 **//
public void addPost(final Post post) {
posts.add(post);
Copy link
Collaborator

Choose a reason for hiding this comment

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

제가 잘못 알고 있을 수도 있는데, 혹시 연관관계 편의 메서드가
한쪽 엔티티(주로 연관관계 주인이 되는 엔티티 ex. Post)에서
양쪽 엔티티의 매핑을 담당하는 거 아닌가용?

Copy link
Collaborator Author

@Anyeon00 Anyeon00 Sep 29, 2024

Choose a reason for hiding this comment

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

아... 그렇네요 양쪽을 설정해줘야 하는데, 조금 이상하게 쓰고 있었네요 다시 공부해서 수정하도록 하겠습니다! 감사합니다ㅎㅎ

Copy link
Collaborator Author

@Anyeon00 Anyeon00 Sep 29, 2024

Choose a reason for hiding this comment

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

수정해서 PR에 반영하였습니다! 혹시 다른 문제 또 있으면 말씀해주세요!


import com.nbe2.domain.posts.service.component.PostAppender;

@SpringBootTest
Copy link
Collaborator

Choose a reason for hiding this comment

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

MockitoExtension을 사용하면 Mock 객체 사용해서 하는
단위 테스트를 작성하는 거라서 아마 @SpringBootTest 같이 쓰시면
의도대로 동작 안 할 거에요..! 아마 오류나지 않을까 싶네요

그런 의미에서 아래 @Transactional도 아마 제가 알기로 필요하진 않을 거에요

Copy link
Collaborator

Choose a reason for hiding this comment

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

아니다 오류는 안 날거 같네요

그래도 제 기억에 SpringBootTest는 통합 테스트용이고,
MockitoExtension은 단위 테스트 용이라 같이 안 쓰는 건 맞을 겁니당

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아하 그렇군요! 테스트는 잘 몰라서 일단 지금까지 하던대로 쓴거랑, 수빈님코드랑 섞어놓고 내버려둔.. 그냥 막 쓴 코드입니다..ㅋㅋ 나중에 Mock과 MockMvc공부해서 한번 작성해보려고 합니다!

@Anyeon00
Copy link
Collaborator Author

그 도메인 모듈에서 service나 repository 등 패키지를 분리하신 부분에 관해서 저도 확실하진 않지만 나름의 궁예를 해봤을 때, 저희 아키텍처 특성 상 클래스가 되게 많아지는데 패키지를 구분하게 되면 같은 도메인 내의 클래스들에서 import가 많아질 수밖에 없을 거 같아요

뭔가 그런 부분을 최소화하고자 도메인 모듈에서 별도의 패키지 분리를 안한게 아닌가라는 생각이 듭니당 이게 뭔가 문제가 된다거나 하진 않을 거 같긴 한데, 이 부분은 저희가 통일하는 게 좋지 않을까요?

넵 일단 지금까지 관성대로 그렇게 만들었는데 나중에 조율하면 좋을 것 같습니다!

@Anyeon00
Copy link
Collaborator Author

Anyeon00 commented Sep 30, 2024

Command 사용

  • postAppend하는데 command를 넘겨준다는 것이 코드 상으로 직관적이지 않음
  • Appender와 Updater 둘 다 같은 정보를 사용하므로, 이를 나타낼 수 있는 dto를 사용하기
  • 혹은 dto를 사용하지 않더라도 상관없음, 파라미터를 여러개 두더라도 코드의 직관성에 더 초점을 두기

service나 component 메서드명에 도메인 이름이 또 사용될 필요 없음

postAppender.appendPost() (x)


까지 수정해서 다시 코멘트 올리겠습니다!

@Anyeon00
Copy link
Collaborator Author

개선 사항

컨트롤러에서 Page를 DTO가 아닌 @PageDefault로 받도록 수정

지역 게시글 페이지 Api의 파라미터를 수정하였습니다.

    @GetMapping
    public Response<PageResult<PostListInfo>> getLocalPostPage(
            //LocalPostPageRequest request <- 아래로 수정
            @RequestParam("city") City city, @PageDefault Page page) {
        PageResult<PostListInfo> postPage = postService.findListPageByCity(page, city);
        return Response.success(postPage);
    }

UserRepository를 사용하지 않고, UserReader를 이용

PostAppender에서 UserRepository를 사용하지 않고, PostService에서 UserReader를 이용하여 user를 찾도록 수정하였습니다.

    @Transactional
    public Long save(final String email, final PostDefaultInfo info) {
        User user = userReader.read(email);
        return postAppender.append(info.toEntity(user));
    }
  • 서비스의 다른 로직에서도, PostReader를 이용해 업데이트등의 로직을 수행하도록 개선하였습니다.
    @Transactional
    public Long update(final Long postsId, final PostDefaultInfo info) {
        Post post = postReader.read(postsId);
        return postUpdater.update(post, info);
    }

Service 인터페이스를 제거

메서드명에 불필요한 도메인이름 중복제거

(Command를 사용하지 않고) 파라미터 등을 활용해 코드 가독성을 개선

PostRegisterCommand와 PostUpdateCommand를 PostDefaultInfo로 통일

PostReader의 readPageList에 오버로딩 적용

postReader.readListPage(PagingUtil.toPageRequest(page), city)
postReader.readListPage(PagingUtil.toPageRequest(page), user)

(수빈님 UserReader사용하면서 보고 바로 따라 만들었습니다ㅎㅋ)

PageRequest로 변환을 컴포넌트에서 하지않고, 서비스에서 변환하여 넘겨주도록 변경

게시글 등록과 수정 Api에서, @ModelAttribute방식에서 @RequestBody방식으로 수정


쓰다보니 자질구레한 것까지 다 적게 되었는데 그냥 코드로 한번 보시는게 편하실 수 있을 것 같습니다.
추가로 필요한 부분 있으면 말씀 부탁드립니다!

@Anyeon00 Anyeon00 force-pushed the feature/posts branch 2 times, most recently from 201620a to 919f1fd Compare September 30, 2024 08:09
@Anyeon00 Anyeon00 merged commit d1e32e5 into develop Sep 30, 2024
1 check passed
@Anyeon00
Copy link
Collaborator Author

Anyeon00 commented Sep 30, 2024

게시글 등록과 내 게시글 조회 컨트롤러에서, String username을 받는 코드를 Long id로 수정하였습니다.

User클래스의 Post연관관계를 삭제했습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] 게시글 기능 구현
2 participants