[커뮤니티 #15] 클린코드 리팩토링 (Admin API) / 도메인 메서드를 사용하는 이유

이제 어느정도 1차 클린코드 리팩토링 작업이 끝이나고 있습니다.

오늘은 Admin API 리팩토링을 진행하였습니다.

​

이 API에는 유저 관리와, 게시글 관리, 정지 관리를 담당합니다.

​

기존 리팩토링 전 Service 코드를 확인해보겠습니다.

 

@RequiredArgsConstructor
@Service
public class AdminService {
    private final UserRepository userRepository;
    private final BoardRepository boardRepository;
    private final UserReportRepository userReportRepository;
    private final BoardReportRepository boardReportRepository;

    @Transactional(readOnly = true)
    public List<UserEditRequestDto> manageReportedUser() {
        List<User> users = userRepository.findByReportedIsTrue();
        List<UserEditRequestDto> usersDto = new ArrayList<>();
        users.stream().forEach(i -> usersDto.add(new UserEditRequestDto().toDto(i)));
        return usersDto;
    }

    @Transactional
    public UserEditRequestDto unlockUser(int id) {
        User user = userRepository.findById(id).orElseThrow(MemberNotEqualsException::new);
        if(!user.isReported()) {
            throw new NotReportedException();
        }
        user.setReported(false);
        userReportRepository.deleteAllByReportedUserId(id);
        return UserEditRequestDto.toDto(user);
    }


    @Transactional(readOnly = true)
    public List<BoardSimpleDto> manageReportedBoards() {
        List<Board> boards = boardRepository.findByReportedIsTrue();
        List<BoardSimpleDto> boardsDto = new ArrayList<>();
        boards.stream().forEach(i -> boardsDto.add(new BoardSimpleDto().toDto(i)));
        return boardsDto;
    }

    @Transactional(readOnly = true)
    public BoardSimpleDto unlockBoard(int id) {
        Board board = boardRepository.findById(id).orElseThrow(BoardNotFoundException::new);
        if(!board.isReported()) {
            throw new NotReportedException();
        }
        board.setReported(false);
        boardReportRepository.deleteAllByReportedBoardId(id);
        return new BoardSimpleDto().toDto(board);
    }
}

기존에는 도메인에 Set을 사용한 직접적인 데이터 변경을 했고, forEach를 통해서 데이터를 Dto로 담아줬네요.

​

이런 방법은 좋지 않습니다.

​

먼저 setter를 이용할 경우 객체가 독립적이지 않게 됩니다. 즉 메서드가 많아질 수록 의존성이 높아지는 문제가 생기게됩니다. setter 자체 또한 값 접근이 가능해져서 객체의 일관성과 안정성을 보장받기 어려워집니다.

​

쉽게 이해할 수 있도록 한 가지 예를 들어서 setter의 일관성 문제에 대해 설명해보겠습니다.

​

출석체크를 한 유저에게 10포인트씩 주는 로직을 setter로 처리했다고 가정해봅시다.

그러면 서비스에서 user.setPoint(user.getPoint() + 10) 이런식으로 코드를 짜겠죠?

​

이렇게 된다면, 포인트를 건드는 모든 부분에 대해서 user.setPoint(user.getPoint() + 10) 이 코드를 반복적으로 사용해야하는 문제가 생기는데, 만약 출석 시 5포인트로 착각하고 5포인트로 개발자가 설정해버리면 일관성이 떨어지게 됩니다.

​

따라서 차라리 user 도메인 안에 도메인 메서드를 아래와 같이 하나 만들어서 처리를하면 일관성과 안정성을 보장받을 수 있게 됩니다.

 

 

@RequiredArgsConstructor
@Service
public class AdminService {
    private final UserRepository userRepository;
    private final BoardRepository boardRepository;
    private final UserReportRepository userReportRepository;
    private final BoardReportRepository boardReportRepository;

    @Transactional(readOnly = true)
    public List<UserEditRequestDto> findReportedUsers() {
        List<User> users = userRepository.findByReportedIsTrue();
        List<UserEditRequestDto> usersDto = users.stream()
                .map(user -> new UserEditRequestDto().toDto(user))
                .collect(Collectors.toList());
        return usersDto;
    }

    @Transactional
    public UserEditRequestDto processUnlockUser(int id) {
        User user = userRepository.findById(id).orElseThrow(MemberNotEqualsException::new);
        validateUnlockUser(user);
        deleteUnlockUser(user, id);
        return UserEditRequestDto.toDto(user);
    }

    private void validateUnlockUser(User user) {
        if (!user.isReported()) {
            throw new NotReportedException();
        }
    }

    private void deleteUnlockUser(User user, int id) {
        user.unlockReport();
        userReportRepository.deleteAllByReportedUserId(id);
    }


    @Transactional(readOnly = true)
    public List<BoardSimpleDto> findReportedBoards() {
        List<Board> boards = boardRepository.findByReportedIsTrue();
        List<BoardSimpleDto> boardsDto = boards.stream()
                .map(board -> new BoardSimpleDto().toDto(board))
                .collect(Collectors.toList());
        return boardsDto;
    }

    @Transactional(readOnly = true)
    public BoardSimpleDto processUnlockBoard(int id) {
        Board board = boardRepository.findById(id).orElseThrow(BoardNotFoundException::new);
        validateUnlockBoard(board);
        deleteUnlockBoard(board, id);
        return new BoardSimpleDto().toDto(board);
    }

    private void deleteUnlockBoard(Board board, int id) {
        board.unReportedBoard();
        boardReportRepository.deleteAllByReportedBoardId(id);
    }

    private void validateUnlockBoard(Board board) {
        if (!board.isReported()) {
            throw new NotReportedException();
        }
    }

위에 코드와 같이 기존 코드에서 네이밍을 일부 변경해주고, 도메인 메서드 사용하게끔 리팩토링을 진행해주었습니다.

​

또한 기존 코드에서는 forEach()를 사용했지만 map()을 사용하면서 오버헤드로 인한 CPU 리소스 낭비를 줄였습니다.

​

For loop와 Stream의 차이는 아래 게시글을 통해서 확인하실 수 있습니다~

https://blog.naver.com/sosow0212/222877044944

[Spring & Java] 스트림(Stream) 사용 이유 및 for loop와의 차이점 / 리팩토링 해보기 실전 예제

 

리팩토링함에 따라 기존 테스트 코드도 리팩토링을 진행해주었습니다.

 

사실 한 가지 걸리는 점이 있는데, 기존에는 도메인 메서드의 중요함을 몰라서 전부 서비스 레이어에 로직을 구현하고 테스트 또한 컨트롤러와 서비스 코드에만 작성하였습니다.

​

하지만, 도메인의 중요함을 이제는 알아서 도메인 테스트 코드도 작성해야할듯 합니다.

​

그래서 일단 아직도 리팩토링 할 것들이 많지만, 1차 클린코드 리팩토링은 이정도로 마무리하고

도메인 테스트 코드부터 작성할 예정입니다.

 

 

---

코드는 아래 깃허브에서 확인하실 수 있습니다.

질문 및 피드백은 댓글로 남겨주세요 :)

https://github.com/sosow0212/community

GitHub - sosow0212/community: Community API Server (with JUnit5, Redis ...)