[잔디일기] 코드 리팩토링을 해보자

기능을 구현 하면서 정신 차리고 보니 중복된 코드가 너무 많아서 코드 리팩토링을 진행하기로 했습니다.

 

여러 사람이 작업하면서 PR 날릴 때 코드 리뷰를 진행하지 않은 결과입니다.

리뷰를 진행하지 않으면 어떤 나비효과를 불러오게 되었는지 요즘 엄청 잘 느끼고 있습니다.😅

 

대표 서비스 클래스인 DiaryService만 코드 리팩토링 과정을 나타내 보려고 합니다.

---

미리보는 리팩토링 결과

• 리팩토링 전 341줄로 이루어져있던 코드는 333줄의 코드로 줄었습니다.
• 코드 줄로만 판단했을 때는 많이 줄은 것은 아니나,
  전반적인 중복 로직에 대한 코드를 메서드로 분리하여 코드의 양이 줄었고, 메서드 내 가독성이 향상되었습니다.
• 신경쓰이는 네이밍도 조금씩 손보아서 코드의 가독성과 명확성이 향상되었습니다.
• 메서드가 많아짐에 따라 어떤 순서로 해당 메서드가 진행되는지 한번에 이해하지 못하는 경우가 있을 것 같아 주석으로 어떤 로직인지 설명을 추가하면 더욱 이해하기 쉬울 것 같습니다.

---

리팩토링 진행된 순서

이 게시글의 목차는 다음과 같습니다.

1. 메서드 내 기능 분리
2. 새로 생성한 메서드 중 중복된 부분이 있는지 확인
3. 새로 생성한 메서드라도 기능 분리가 필요한 곳이 있는지 확인
4. 팀원 분의 코드리뷰

---

1. 메서드 내 기능 분리하기

기존 DiaryService 코드

너무 많은 기능이 한 메서드 안에 담겨있어서 읽기도 힘든 것을 볼 수 있습니다.

코드 전체를 가져와서 보여드리는 것보다는 save 기능을하는 메서드를 예시로 가져와 보았습니다.

한 메서드가 8-90줄 가량 되는 것도 있는데 이게 말이 안 되죠...😅

@Service
@RequiredArgsConstructor
public class DiaryService {

    @Transactional
    public DiarySaveResponseDTO save(Long id, DiarySaveRequestDTO requestDto, MultipartFile image) {
        // TODO: 기능별 메서드 분리
        
        // 1. 일기 작성하는 사용자 검색
        Member member = memberDAO.findById(id)
                .orElseThrow(() -> new MemberNotFoundException("해당 사용자가 존재하지 않습니다. id = " + id));

	// 2. 다이어리 저장
        Diary diary = diaryDAO.save(requestDto.toEntity(member));

	// 3. 해시태그가 있다면 해시태그 저장
        if (requestDto.getHashtags() != null) {
            for (String hashtag : requestDto.getHashtags()) {
                TagList tagList = tagListDAO.findByTag(hashtag)
                        .orElseGet(() -> tagListDAO.save(new TagList(hashtag)));
                tagList.incrementCount();
                MemberTags memberTags = memberTagsDAO.findByMemberIdAndTagList(member.getId(), tagList)
                        .orElseGet(() -> memberTagsDAO.save(new MemberTags(member, tagList)));
                memberTags.incrementCount();
                diaryTagDAO.save(new DiaryTag(diary, memberTags));
            }
        }
	
    	// 4. 리워드 포인트 랜덤 생성 및 저장
        long seed = System.currentTimeMillis();
        Random random = new Random(seed);
        int rewardPoint = random.nextInt(10) + 1;

        member.addRandomPoint(rewardPoint);
        
        // 5. 리워드 출처 저장
        RewardHistory rewardHistory = rewardHistoryDAO
                .save(new RewardHistory(member, RewardType.PLUS_DIARY_WRITE, rewardPoint));

        if (rewardHistory.getId() == null) {
            throw new IllegalArgumentException("일기 히스토리 저장 오류");
        }
		
        // 6. 일기 내 이미지가 있다면 이미지 저장 후 이미지 경로 전송, 없다면 String("") 값 전송
        if (requestDto.getHasImage()) {
            String imagePath = diaryImageService.uploadDiaryImage(image, FileFolder.PERSONAL_DIARY, diary);
            return new DiarySaveResponseDTO(diary.getId(), true, imagePath);
        }
        return new DiarySaveResponseDTO(diary.getId(), false, "");
    }
}

1차적으로 수정된 전체 코드

전체적으로 기능별로 메서드를 만들어서 불러오도록 수정 했습니다.

전반적인 가독성이 향상 된 것을 볼 수 있습니다.

@Service
@RequiredArgsConstructor
public class DiaryService {

    @Transactional
    public DiarySaveResponseDTO save(Long id, DiarySaveRequestDTO requestDto, MultipartFile image) {
        Member member = getMemberById(id);
        Diary diary = saveDiary(requestDto, member);

        if (requestDto.getHashtags() != null) {
            saveTags(requestDto, member, diary);
        }

        int rewardPoint = makeRewardPoint();
        saveRewardPointAndHistory(member, rewardPoint);

        if (hasImage(requestDto.getHasImage(), image)) {
            //TODO: 만약 hasImage가 true인데 image가 비었다면 FE에게 에러 메시지 보낼 것
            String imagePath = diaryImageService.uploadDiaryImage(image, FileFolder.PERSONAL_DIARY, diary);
            return new DiarySaveResponseDTO(diary.getId(), true, imagePath);
        }
        return new DiarySaveResponseDTO(diary.getId(), false, "");
    }
    
    private Member getMemberById(Long id) {
        return memberDAO.findById(id)
                .orElseThrow(() -> new MemberNotFoundException("해당 사용자가 존재하지 않습니다. id = " + id));
    }

    private Diary saveDiary(DiarySaveRequestDTO requestDto, Member member) {
        return diaryDAO.save(requestDto.toEntity(member));
    }

    private void saveTags(DiarySaveRequestDTO requestDto, Member member, Diary diary) {
        for (String hashtag : requestDto.getHashtags()) {
            TagList tagList = tagListDAO.findByTag(hashtag)
                    .orElseGet(() -> tagListDAO.save(new TagList(hashtag)));
            tagList.incrementCount();
            MemberTags memberTags = memberTagsDAO.findByMemberIdAndTagList(member.getId(), tagList)
                    .orElseGet(() -> memberTagsDAO.save(new MemberTags(member, tagList)));
            memberTags.incrementCount();
            diaryTagDAO.save(new DiaryTag(diary, memberTags));
        }
    }

    private int makeRewardPoint() {
        long seed = System.currentTimeMillis();
        Random random = new Random(seed);
        return random.nextInt(10) + 1;
    }

    private void saveRewardPointAndHistory(Member member, int rewardPoint) {
        member.addRandomPoint(rewardPoint);
        RewardHistory rewardHistory = rewardHistoryDAO
                .save(new RewardHistory(member, RewardType.PLUS_DIARY_WRITE, rewardPoint));

        if (rewardHistory.getId() == null) {
            throw new IllegalArgumentException("일기 히스토리 저장 오류");
        }
    }
}

---

2. 새로 생성된 메서들 중 중복 제거

Tag 관련 메서드 중복 제거

리팩토링 전

// 새로운 태그 저장
private void saveTags(DiarySaveRequestDTO requestDto, Member member, Diary diary) {
    for (String hashtag : requestDto.getHashtags()) {
        TagList tagList = tagListDAO.findByTag(hashtag)
                .orElseGet(() -> tagListDAO.save(new TagList(hashtag)));
        tagList.incrementCount();
        MemberTags memberTags = memberTagsDAO.findByMemberIdAndTagList(member.getId(), tagList)
                .orElseGet(() -> memberTagsDAO.save(new MemberTags(member, tagList)));
        memberTags.incrementCount();
        diaryTagDAO.save(new DiaryTag(diary, memberTags));
    }
}

// 일기 업데이트시 사용하는 새로운 태그 저장
private void saveNewTags(DiaryUpdateRequestDTO requestDto, Diary diary) {
    for (String hashtag : requestDto.getHashtags()) {
        TagList newTagList = tagListDAO.findByTag(hashtag)
                .orElseGet(() -> tagListDAO.save(new TagList(hashtag)));
        newTagList.incrementCount();
        MemberTags newMemberTags = memberTagsDAO.findByMemberIdAndTagList(diary.getMember().getId(), newTagList)
                .orElseGet(() -> memberTagsDAO.save(new MemberTags(diary.getMember(), newTagList)));
        newMemberTags.incrementCount();
        diaryTagDAO.save(new DiaryTag(diary, newMemberTags));
    }
}

 

알고보니 같은 로직이 들어 가있어서 이 부분을 지우고 본문에 들어있던 tag 관련 로직을 한 곳으로 모아 주었습니다.

리팩토링 후

private void saveTags(List<String> hashtags, Member member, Diary diary) {
    if (hashtags != null) {
        hashtags.forEach(hashtag -> saveTag(hashtag, member, diary));
    }
}

private void saveTag(String hashtag, Member member, Diary diary) {
    TagList tagList = tagListDAO.findByTag(hashtag)
            .orElseGet(() -> tagListDAO.save(new TagList(hashtag)));
    tagList.incrementCount();
    MemberTags memberTags = memberTagsDAO.findByMemberIdAndTagList(member.getId(), tagList)
            .orElseGet(() -> memberTagsDAO.save(new MemberTags(member, tagList)));
    memberTags.incrementCount();
    diaryTagDAO.save(new DiaryTag(diary, memberTags));
}

private void updateTags(Diary diary, List<String> newHashtags) {
    removeExistingTags(diary);
    saveTags(newHashtags, diary.getMember(), diary);
}

@Transactional
public DiarySaveResponseDTO update(Long id, DiaryUpdateRequestDTO requestDto, MultipartFile image) {
    Diary diary = getDiaryById(id);
    validateUpdateDate(diary);
    updateTags(diary, requestDto.getHashtags()); // 변경된 로직
    /* 기존 로직
    removeExistingTags(diary);

    if (requestDto.getHashtags() != null) {
        saveNewTags(requestDto, diary);
    }
    */

    boolean currentHasImage = requestDto.getHasImage();
    String imagePath = updateDiaryImage(currentHasImage, image, diary);

    return updateDiary(requestDto, diary, currentHasImage, imagePath);
}

 

덕분에 코드의 중복된 부분을 줄일 수 있었습니다.

3. 기능 분리가 필요한 부분 재확인

서비스 기본 로직과 이미지 관련 로직을 별도의 메서드로 분리하여 가독성을 향상 시켰습니다.

리팩토링 전

// save 메서드 내 이미지가 있을 때 이미지를 저장하는 부분
if (hasImage(requestDto.getHasImage(), image)) {
    String imagePath = diaryImageService.uploadDiaryImage(image, FileFolder.PERSONAL_DIARY, diary);
    return new DiarySaveResponseDTO(diary.getId(), true, imagePath);
}
return new DiarySaveResponseDTO(diary.getId(), false, "");

//
private String updateDiaryImage(boolean currentHasImage, MultipartFile image, Diary diary) {
    boolean originalHasImage = diary.getHasImage() != null && diary.getHasImage();
    String imagePath = "";
    if (currentHasImage) {
        imagePath = diaryImageService.updateImage(originalHasImage, image, FileFolder.PERSONAL_DIARY, diary);
    }
    if (originalHasImage && !currentHasImage) {
        diaryImageService.deleteImage(diary);
    }
    return imagePath;
}

리팩토링 후

// 변경된 save 메서드 내 이미지가 있을 때 이미지를 저장하는 부분
// 가독성이 향상된 것 뿐만 아니라 이미지 로직이 분리 되었다.
if (hasImage(requestDto.getHasImage(), image)) {
    return sendSaveResponseWithImage(diary, image);
}
return new DiarySaveResponseDTO(diary.getId(), false, "");

//
private DiarySaveResponseDTO sendSaveResponseWithImage(Diary diary, MultipartFile image) {
    String imagePath = diaryImageService.uploadDiaryImage(image, FileFolder.PERSONAL_DIARY, diary);
    return new DiarySaveResponseDTO(diary.getId(), true, imagePath);
}

private String updateDiaryImage(boolean currentHasImage, MultipartFile image, Diary diary) {
    boolean originalHasImage = Boolean.TRUE.equals(diary.getHasImage());

    if (currentHasImage) {
        return diaryImageService.updateImage(originalHasImage, image, FileFolder.PERSONAL_DIARY, diary);
    }
    if (originalHasImage) {
        diaryImageService.deleteImage(diary);
    }

    return "";
}

 

해당 리팩토링 하면서 `Boolean.TRUE.equals(diary.getHasImage())` 이 부분도 알게 되었는데 ...

• Boolean.TRUE.equals(null)은 false를 반환합니다.
• Boolean.TRUE.equals(Boolean.TRUE)는 true를 반환합니다.
• Boolean.TRUE.equals(Boolean.FALSE)는 false를 반환합니다.

Boolean의 해당 메서드는 null 값이 들어와도 false로 반환해주는 편리한 메서드였습니다.
이 부분도 다른 로직에 많이 사용했어서 교체 해주면 좋을 것 같은데 ... 일단은 보류해두고 다음 리팩토링 때 진행 해야겠습니다. 😢

 

처음에는 별거 아니라고 생각을 했었는데 리팩토링 하는 데도 많은 에너지가 쓰이고 생각보다 시간이 많이 걸리는 걸 깨닫게 된 경험이었습니다. 😂

 

---

4. PR 날린 후 팀원 분의 코드리뷰

1. DiaryDTO의 경우 이름을 DiaryResponseDTO로 하는 것은 어떨까요?
   • 클래스의 이름을 명확하게하여 사용하는 곳을 알 수 있도록 하는 것이 좋았습니다.
   • 이번에 이미지 기능이 업데이트 되면서 일기들을 모아 보는 페이지에서는 이미지 정보를 보내주지 않고, 세부 일기를 확인 했을 때만 추가 해주자는 논의가 진행되고 있어(확정되지 않음) 추후 확장될 기능을 위해서 `DiaryDetailDTO`를 건의 드렸습니다.
2. 이미지 기능이 추가되면서 FE에서 이미지가 존재할 때 `hasImage`라는 boolean 값을 사용 중인데, 해당 변수가 꼭 필요할까요?
   • 처음에 저는 이미지 기능은 사용자가 선택 가능한 데다, default가 false라고 생각을 했어서 그 값을 명시해주기 위해 필요하다 생각했습니다. 하지만 해당 리뷰를 읽고 고민을 해보다 `hasImage` 변수 값을 받지 않기로 결정했습니다.
     • hasImage 값이 있다면 물론 명시적으로 표현이 가능하지만 hasImage값이 False였을 때 이미지 데이터가 없었을 경우를 위해 따로 예외처리가 필요합니다.(서로 값이 배치되는 경우를 고려)
     • 기타 예외를 확인 해야할 곳이 많은 것입니다.

       Boolean 값	사용자의 이미지 삽입 유무	FE 전송 값	BE 받은 값
       hasImage(True)	O	O	O	정상
       	O	O	X	예외 발생
       	X	O	O	예외 발생
       hasImage(False)	O	.	.	예외 발생
       ... 기타 예외 발생 가능

     • 하지만 hasImage 변수가 존재하지 않았을 때 image 데이터 자체 값의 유무만 판단하면 되기 때문에 불필요한 로직을 줄일 수 있다고 판단 했습니다.