refactor: 게시글 관련 서비스 구조 리팩터링 #168
Conversation
|
테스트뿐만 아니라 전반적인 서비스 로직의 리팩토링이 진행된 것으로 보이는데, 커밋 메시지를 refactor: ~ 로 수정하는 것이 어떨까요? 테스트는 코드 작성에 따라 자연스럽게 따라 붙는 것이기에 테스트만을 추가하는 커밋이 아니면 test 외에 다른것을 쓰는게 좋을 것 같습니다. |
There was a problem hiding this comment.
PostService가 조회, 생성/수정/삭제, 좋아요 관련 로직을 모두 담당하고 있어서 PostQueryService, PostCommandService, PostLikeService 로 분리하였습니다.
👍👍👍
1/
게시글 관련 서비스 로직들이 다 파라미터로 받는 값들이 많아 given절이 조금 지저분해져있네요..
given 절이 무엇을 담게 할 것인지, 그 기준에 따라서 ‘불필요한 given 절인가?’도 달라진다고 생각합니다.
제 주변 개발자들도 의견이 갈리더라고요.
‘해당 테스트 함수에서 모든게 이해되어야 한다. private 함수나 fixture도 지양하자’ 는 사람도 있고,
‘무엇을 테스트할지에 집중하는게 더 중요하다. 관련 없는 부분을 생략해도 해도 된다’는 사람도 있습니다.
전자의 입장에서는 지금 규혁님의 코드가 전혀 지저분한 given 절이 아닐겁니다.
저는 개인적으로 저는 후자를 선호합니다…🤭
게시글과 관련된 서비스 코드들은, “사용자와 게시판이 DB에 존재하는 상태”를 전제로 한다고 생각해요.
그래서 이와 관련된 부분은 비중을 줄여도 괜찮을 것 같습니다.
예를 들어, given 절에서 아래 코드가 여러번 사용되더라고요.
SiteUser testUser = createSiteUser();
Board testBoard = createBoard(BoardCode.FREE);이를 줄이기 위해 private SiteUser testUser; private Board testFreeBoard; 를 테스트 클래스에 전역변수로 선언하고,
@BeforeEach 에서 생성 함수를 호출해서 초기화한 다음,
테스트 함수들에서는 testUser와 testFreeBoard 전역변수를 사용하게 할 수도 있을 것 같아요.
하지만 이 역시 규혁님이 지저분한 given 절이라고 하셨기에, 그것을 줄일 방법을 제안드린 것이고
지금의 테스트 코드도 충분히 괜찮아보입니다!
선택적으로 적용하시면 될 것 같습니다.
2/
유효하지 않은 코드로 조회/생성/수정/삭제/좋아요 하는 경우의 테스트코드를 작성했다가 지난번 유저예외처리랑 비슷하게 반복되는 거 같아서 그냥 빼버렸습니다. 혹시나 필요하다면 다시 추가하겠습니다.
좋은 판단이었다 생각합니다!!!👍
사실 애초에 컨트롤러에서 code 를 String 으로 받지 않고 BoardCode enum 으로 받았다면 자동으로 검증되었을것 같아요😓
이 부분도 리팩터링 대상이라 생각하는데, 일단 이렇게 구현이 되어있고, 잘 동작은 하니까 enum으로 바꾸는건 다음에 해봐요!
테스트 코드에서 제외한건 잘 판단하셨습니다 ㅎㅎ
3/
게시글 정상 생성 및 수정 시 Post 엔티티의 PostImage 컬렉션이 지연 로딩으로 설정되어 있어, savedPost.getPostImageList()와 같이 연관 관계 데이터를 검증할 때 LazyInitializationException이 발생합니다. 이를 해결하기 위해 @transactional 어노테이션을 추가했습니다.
테스트 코드에 @Transactional 어노테이션을 붙이는 것은 여러가지 이슈가 있을 수 있어 지양하고 싶지만, 참고
규혁님 말씀처럼 이 방식이 가장 간편하게 해결되는 것 같습니다.
그리고 테스트 코드가 안깨지게 하기 위해서 FetchType을 EAGER 로 설정하는 등의 일을 할 순 없는 일이기도 하고요😅
사실 [애초에 왜 저들이 양방향이어야 하는지? / FetchType이 디폴트인 LAZY 여야 했을지? / 엔티티 그래프의 필요성은 없는지?] 🤔
같은 근본적인 것들에 대한 생각이 들긴 하는데… 이건 테스트 PR이니깐요!
일단 이대로 진행해도 좋을 것 같습니다.
4/
기존 isPresent 함수는 키가 없으면 생성까지 해버리기 때문에, 단순히 키가 존재하는지 확인하는 로직이 필요하다고 판단했습니다. 이를 위해 간단히 키 존재 여부를 확인하는 함수를 추가했는데, 이 방식이 적절한지 고민이 됩니다
기존 isPresent 함수가 이름에서 벗어나는 작업을 했네요,
isPresent 면 존재하는지만 반환할 것 같은데 없으면 새로 생성까지 하다니… 🤔
어차피 리팩터링되어야 할 대상이었던 것 같고, isKeyExists 를 새로 만드신 것도 적절하다 생각합니다👍
| request, | ||
| imageFiles | ||
| ); | ||
| Post savedPost = postRepository.findById(response.id()).orElseThrow(); |
There was a problem hiding this comment.
이 부분은 then 절로 옮겨야 할 것 같아요~
There was a problem hiding this comment.
700 줄의 테스트 코드를 적절히 분배해서 없앤게 참 좋네요
|
추가로, 커밋 프리픽스에 대해서는 위백님 의견에 동의합니다! |
Gyuhyeok99
changed the title
|
테스트 코드를 작성하다가 서비스쪽 코드를 건들이게 되어서 그 커밋만 refactor로 진행을 하였는데 혹시 전부 test 대신 refactor를 사용하는 게 좋다고 말씀하신걸까요? |
생각해보니 유저와 게시판의 경우는 굳이 여기서 private 함수로 생성하는 것보단 대학 정보처럼 BaseIntegrationTest에서 미리 생성하는 게 좋을 거 같네요! |
Gyuhyeok99
changed the title
관련 이슈
작업 내용
게시글 관련 통합 테스트 코드 추가하였습니다.
특이 사항
PostService가 조회, 생성/수정/삭제, 좋아요 관련 로직을 모두 담당하고 있어서 PostQueryService, PostCommandService, PostLikeService 로 분리하였습니다. 처음엔 좋아요만 분리하려고 하였으나 추후 조회 관련 로직이 추가될 수도 있다고 생각해서 그냥 3개로 분리하였습니다.
리뷰 요구사항 (선택)
게시글 관련 서비스 로직들이 다 파라미터로 받는 값들이 많아 given절이 조금 지저분해져있네요.. 최대한 신경써보긴 했는데 혹시 너무 읽기 불편하면 제가 다시 개선해보겠습니다.
유효하지 않은 코드로 조회/생성/수정/삭제/좋아요 하는 경우의 테스트코드를 작성했다가 지난번 유저예외처리랑 비슷하게 반복되는 거 같아서 그냥 빼버렸습니다. 혹시나 필요하다면 다시 추가하겠습니다.
게시글 정상 생성 및 수정 시 Post 엔티티의 PostImage 컬렉션이 지연 로딩으로 설정되어 있어, savedPost.getPostImageList()와 같이 연관 관계 데이터를 검증할 때 LazyInitializationException이 발생합니다. 이를 해결하기 위해 @transactional 어노테이션을 추가했습니다.
다른 방식으로는 실제 Repository에서 한 번에 가져오도록 로직을 수정하거나 TestRepository를 추가해줘야할 거 같은데 이 방식이 제일 간단할 거 같아서 이렇게 진행했는데 괜찮을까요?
Redis 로직을 살펴보니, 해당 키의 존재 여부만 확인하는 함수가 없었습니다. 기존 isPresent 함수는 키가 없으면 생성까지 해버리기 때문에, 단순히 키가 존재하는지 확인하는 로직이 필요하다고 판단했습니다. 이를 위해 간단히 키 존재 여부를 확인하는 함수를 추가했는데, 이 방식이 적절한지 고민이 됩니다. 특히, 테스트를 위해 Main 코드에 변경을 가하는 것은 지양하는 것이 좋다고 생각합니다. 하지만, Redis에 값이 있는지 확인하는 로직은 추후 다른 곳에서도 재사용 가능성이 충분히 있을 것 같아 추가했는데 괜찮은지 의견 부탁드립니다..!