sprigboot 프로젝트 진행중 리펙토링 적용

개발을 하는데 있어서 가장 중요하고 자주하는 것이 리펙토링이라 들었다. 지금 작성한 코드가 다른 사람이 볼때 충분히 가독성이 있을지, 객체지향적인 방식을 잘 활용하고 있는지를 확실치 못하고 있었다. 면접을 위한 cs전공지식노트(주홍철)에서 디자인 패턴 중 펙토리 패턴과 전략 패턴에 대해서 적용해보고자 리펙토링을 하게 되었다.

위 패턴들을 적용하려는 이유는 현재 진행중인 프로젝트 중 findArticles가 하나의 메서드로 되있지만 public, private, grouped에 따라 구분을 해야한다. 하나의 메서드로 3개를 경우에 따라 구분을 하는 것보다는 3개의 메서드로 가독성을 향상시키는 것이 좋다고 생각하였기 때문이다.

하지만 이 패턴들을 findArticleService에 막연히 적용하기에는 상당히 어려웠다. 이를 인터넷에 검색하면서 리펙토링에 대해서 알아보았고 우아한 테크세미나 중 TDD 리펙토링 by 자바지기 박재성님의 강연을 유튜브를 통해 어떻게 진행할 지를 알게되었다.

여기서 알게된 것은 리펙토링은 굳이 거창하게 적용하는 것이 아니고 하나하나 적용하면서 프로젝트가 끝나서도 지속적으로 수행하는 것이 리펙토링이라는 것이다.

그렇기에 거창하게 디자인 패턴을 적용해보자는 생각보다는 하나하나 리펙토링을 해나가자는 생각으로 진행을 하였다.

아래는 수정 전과 후의 service 코드이다.

findArticleRepository에 findArticles는 동적쿼리를 사용하여 userId가 null이냐 null이 아니냐에 따라 publicArticle을 리턴하는지, privateArticle을 리턴하는지 구분한다.

수정 전

@Service
@RequiredArgsConstructor
public class FindArticleService {
    private final FindArticleRepository findArticleRepository;

    @Transactional(readOnly = true)
    public List<ArticleMapResponse> findMapArticles(Long userId,
            Double latitude, Double latitudeRange, Double longitude, Double longitudeRange) {
        LocationRange locationRange = new LocationRange(latitude, latitudeRange, longitude,
                longitudeRange);
        List<Article> articles = findArticleRepository
                .findArticles(userId,
                        locationRange.getUpperLatitude(), locationRange.getLowerLatitude(),
                        locationRange.getUpperLongitude(), locationRange.getLowerLongitude());
        return toResponses(articles);
    }

    private List<ArticleMapResponse> toResponses(List<Article> articles) {
        if (Objects.isNull(articles)) {
            return Collections.emptyList();
        }
        return articles.stream()
                .map(article -> new ArticleMapResponse(
                        article.getId(),
                        article.getLatitude(),
                        article.getLongitude(),
                        article.getTitle()))
                .collect(Collectors.toUnmodifiableList());
    }
}

수정 후

@Service
@RequiredArgsConstructor
public class FindArticleService {
    private final FindArticleRepository findArticleRepository;

    @Transactional(readOnly = true)
    public List<ArticleMapResponse> findPublicMapArticles(
            LocationRange locationRange) {
        Long userId = null;
        return ArticleMapResponse.toResponses(findArticles(userId, locationRange));
    }
    @Transactional(readOnly = true)
    public List<ArticleMapResponse> findPrivateMapArticles(
            Long userId, LocationRange locationRange) {
        return ArticleMapResponse.toResponses(findArticles(userId, locationRange));
    }

    private List<Article> findArticles(Long userId, LocationRange locationRange) {
        return findArticleRepository.findArticles(userId,
                locationRange.getUpperLatitude(), locationRange.getLowerLatitude(),
                locationRange.getUpperLongitude(), locationRange.getLowerLongitude());
    }
}

비교해보면 수정한 것이 훨씬 가독성이 있다.

여러 글들을 찾으며 정리한 리펙토링 기법을 아래에 간략하게 적어보겠다.

우선 리펙토링의 가장 큰 목적은 가독성, 확장성, 재사용성에 있다.

1. 객체의 get, set 함수를 쓰지 말아라.

우선 set을 사용하지 말아야하는 이유는 무분별한 변수의 변화를 막기위해서이고 계속 set을 쓰다보면 값들이 어제 어디서 변해야 하는지 코드상으로 명확하게 구분할 수가 없어, 차후 변경 시 복잡해질 것이다.

또 하나의 이유는 가독성 문제이다. 단순히 set만 사용한다면 복잡한 의도에 대해서는 한번에 알아보기가 힘든 경우가 있다. 그렇기에 set를 사용하기 보다는 update 같은 이름으로 메서드를 만들어서 외부 로직에서 호출하여 사용하는 것이 좋을 것이다. 이는 https://khdscor.tistory.com/15 에 따로 정리를 해두 었다.

하지만 get을 사용하지 말라는 것은 잘 몰랐었다.

이는 상태 데이터를 가지는 객체에서 데이터를 꺼내려(get)하지 말고 객체에 메시지를 보내라는 것이다.

아래는 우아한테크세미나에서 봤었던 예시 코드이다.

private boolean isMaxPosition(Car car) {
	return car.getPosition(0 == maxDistance;
}

위 코드를 아래와 같이 수정을 하는 것이다.

private boolean isMaxPosition(Car car) {
  return car.isMaxPosition(maxDistance);
}

확실히 가독성이 좋아졌다. 이런식으로 get을 쓰기보다는 객체에 메시지를 보내는 방식으로 수정을 해야 겠다.

2. 하나의 메서드는 하나의 기능을 가져라.

하나의 메서드에 여러개를 꾸겨넣지 말라는 말이라. 메서드 내부는 최소한의 라인으로 구성하여 indent depth(들여쓰기)도 최소한으로 해야 한다. if문 안에 for문안에 if문안에…같이 막 우겨넣지 말고 for문을 혹은 if문을 하나의 메서드로서 따로 구현하라는 것이다. 메서드는 많이 분리하여라. 이렇게 하면 코드의 가독성 뿐만 아니라 재사용성이 증가할 것이다.

3. else를 쓰지 말아라.

아래의 예시를 보자.

private boolean test(Test test){
  int isNull
;
  if(test == null) {
    isNull = 1;
		//추가코드 실행
		}
  else {
    isNull = 0;
		//추가코드 실행
		}
  return isNull;
}

위의 코드를 아래와 같이 바꿔보자.

private boolean test(Test test){
  int isNull;
  if(test == null) {
    isNull = 1;
		//추가코드 실행
		return isNull;
		}
  isNull = 0;
	//추가코드 실행
  return isNull;
}

if문에서 수행을 하고 바로 리턴을 해버리면 else를 쓸 필요가 없고 훨씬 깔끔해 보인다. 그리고 쓸데없는 들여쓰기를 하지 않아도 되는 것이다.

이외에도

1. 자주사용되는 값, 상수는 추출하여 변수화
2. 불필요한 공백라인 제거
3. 변수, 메서드, 클래스 등 이름 재정의
4. java api 적극 사용(이미 있는 api를 힘들게 구현하지 말자)
5. 일부 코드를 메서드 혹은 클래스로 분리하여 가독성, 재사용성, 확장성 향상
6. 메서드 매개변수의 최소화

등이 있다.

 

참고

면접을 위한 cs전공지식노트 - 주홍철

[우아한테크세미나] 190425 TDD 리팩토링 by 자바지기 박재성님(https://www.youtube.com/watch?v=bIeqAlmNRrA&t=1996s)