[리팩터링] 테스트 가능한 코드로 리팩터링하기

nimoh·2023년 3월 1일
리팩터링
0

최근 프로젝트를 수정하면서 테스트가 가능한 코드에 대해 고민했다. 갑자기 고민한 것은 아니고 내가 작성한 코드가 테스트하기 좋지않다는 것을 느꼈기 때문이다.

기존 코드의 문제점

프로젝트에 jsoup라이브러리를 사용하여 웹 사이트를 크롤링하는 기능이 있었다.

첫 번째 문제

Jsoup 라이브러리를 사용하려면 Jsoup 클래스의 connect 스태틱 메서드를 사용해서 jsoup Connection을 연결했어야했다. 이를 아래와 같이 작성했다.

	// JobSearchServiceImpl.java
	...
    String url = "크롤링할 URL";
    Connection connection = Jsoup.connect(url);
	Document documenet = connection.get();
    ...

Document는 url에서 가져온 html 문서가 저장되는 객체이다. 이 코드는 @Service 빈에 해당하는 객체에 작성되었으며 Service를 테스트하기 위해서는 라이브러리 역시 테스트해야했다.

Jsoup은 내가 정의한 객체가 아니기때문에 매 번 실제로 크롤링해오는 API에 테스트의 성공여부를 맡길 순 없었다. 테스트를 할 때마다 실제로 크롤링해와야 하므로 테스트 성능 저하도 발생했다.

두 번째 문제

복잡한 로직을 메서드로 추출하려다보니 private 메서드가 많이 생겼다. 

    public List<JobCrawlerDto> getSearchList(Map<String, String> searchOption, String crawlerName) throws IOException {
    	...
        method1();
        method2();
        method3();
        ...
    }
    private String method1(){
    }
    private String method2(){
    }
    private List<JobCrawler> method3(){
    	method4();
    }
    private void method4();

실제 코드는 아니며, 이런 식으로 여러 개의 private 메서드가 객체를 이루고 있었다는 것이다. private 접근제어자는 클래스 외부에서는 접근할 수 없다. 테스트 파일 역시 클래스 외부이다. 정상적인 방법으로 private 메서드를 테스트할 수 없었다.

세 번째 문제

내가 작성한 JobSearchService 인터페이스는 다음과 같은 구현체를 가지고 있었다.

  • Service 인터페이스
	public interface JobSearchService {
    	List<JobCrawlerDto> getSearchList();
    }
  • 구직사이트 별 Service 구현체(크롤러)

👉 사람인

	@Service
	public class SaraminCrawler implements JobSearchService {
    	@Override
        List<JobCrawlerDto> getSearchList() {J}
        
        private String method1(){A}
        private String method2(){B}
        private String method3(){C}
    }

👉 인크루트

	@Service
	public class IncruitCrawler implements JobSearchService {
    	@Override
        ist<JobCrawlerDto> getSearchList() {K}
        
        private String method1(){D}
        private String method2(){E}
        private String method3(){F}
    }

👉 잡코리아

	@Service
	public class JobKoreaCrawler implements JobSearchService {
    	@Override
        List<JobCrawlerDto> getSearchList() {L}
        
        private String method1(){G}
        private String method2(){H}
        private String method3(){I}
    }

메서드 내를 알파벳으로 표현한 것은 모두 다른 코드형식이라는 의미

세 개의 서비스 구현체는 똑같은 메서드 시그니처를 가졌지만 그 안에 로직은 달랐다. 나름 전략패턴으로 생각하여 하나의 인터페이스에 서비스 컴포넌트를 세 개 두었다. 여기까진 그럴 수 있다고 생각한다. 하지만 오버라이딩 하는 메서드도 아닌데 모두 동일한 시그니처의 메서드? 뭔가 중복된 것 같고 좋은 코드가 아니라는 생각이 들었다.

리팩터링

첫 번째 문제

실제로 Jsoup 라이브러리를 사용하지 않고 테스트하기 위해 Jsoup 객체를 Mocking할 필요가 있었다. 올바른 Mocking을 위해 Jsoup 라이브러리를 인터페이스로 감싸서 의존성을 주입해주었다.

	public interface JsoupConnection {
    	Document get(String url) throws IOException;
	}
	@Component
	public class JsoupConnectionImpl implements JsoupConnection {
 	   private Connection connect(String url) {
    	    return Jsoup.connect(url);
	    }

    	@Override
	    public Document get(String url) throws IOException {
    	    Connection connect = connect(url);
        	return connect.get();
    }
}

두 번째 문제와 세 번째 문제

private 메서드를 테스트하는 것은 쉬운 일이 아니었다. public 메서드를 테스트하면서 거기에 포함된 private 메서드를 간접적으로 테스트하는 것이 최선이었다. private method에 별 다른 추가 기능이 없는 순수 자바코드였다면 아마 이 방법으로도 잘 해결되었을 것이다. 하지만 내 코드는 Jsoup 라이브러리에서 제공하는 객체와 메서드가 난무했고, 메서드의 결과로 html문서가 넘어오는 경우도 있었다. 테스트하자고 html 코드를 넣어 테스트하는 것은 그리 좋은 방법같진 않았다.

또한, reflection을 사용하면 접근제어자와 관계없이 메서드에 접근할 수 있지만 캡슐화를 저해하는 명백한 안티패턴이라 생각했다.

어떻게 하면 중복된 private 메서드들을 테스트하기 좋은 코드로 리팩터링하면 좋을까 고민했다. 내가 내린 결론은 '책임을 나누자' 였다.

JobSearchService의 막중한 책임

기존 코드에서 JobSearchServiceImpl 객체는 서비스의 역할과 크롤링까지 해야하는 막중한 책임을 떠안았다. 그러다보니 본인 본연의 행위도 아닌 private 메서드가 난무했고, 그 결과 테스트할 수가 없었다.
따라서 JobSearchServiceImpl로부터 크롤러를 떼어낼 필요가 있었다.

크롤러 기능을 서비스에서 떼어내자 private으로 난무하던 메서드들은 당당하게 한 객체의 기능으로서 public의 역할을 해내게되었다.

  • 크롤러 인터페이스
public interface Crawler {
    String getBaseUrl();

	String makeSearchListUrl(Map<String, String> searchOption);
    
    boolean checkSearchOption(Map<String, String> searchOption);
    
    boolean checkValidSortOption(String recruitSort);

    List<JobCrawlerDto> parseHTML(Document document);

    List<JobCrawlerDto> extracted(Elements itemRecruit, int resultCount);
}
  • 크롤러 구현체 중 사람인크롤러
@Component
public class SaraminCrawler implements Crawler {

    final private String baseUrl = "www.saramin.co.kr";

    @Override
    public String getBaseUrl() {
        return baseUrl;
    }

    @Override
    public boolean checkSearchOption(Map<String, String> searchOption) {
        return searchOption.get("searchWord") == null || searchOption.get("recruitPage") == null || searchOption.get("recruitSort") == null;
    }

    @Override
    public boolean checkValidSortOption(String recruitSort){
        SaraminRecruitSort[] saraminRecruitSorts = SaraminRecruitSort.values();
        return Arrays.stream(saraminRecruitSorts).noneMatch(sort -> sort.getResultSort().equals(recruitSort));
    }

    @Override
    public String makeSearchListUrl(Map<String, String> searchOption){
        if(searchOption.isEmpty()) return "empty";
        return "https://" + baseUrl +"/zf_user/search/recruit?search_done=y&search_optional_item=n&company_cd=0%2C1%2C2%2C3%2C4%2C5%2C6%2C7%2C9%2C10&show_applied=&quick_apply=&except_read=&ai_head_hunting=&mainSearch=n&loc_mcd=101000&inner_com_type=&recruitPageCount=20"
                + "&searchword=" +searchOption.get("searchWord")
                + "&recruitPage=" + searchOption.get("recruitPage")
                + "&recruitSort=" + searchOption.get("recruitSort");
    }
	
    @Override
    public List<JobCrawlerDto> parseHTML(Document document) {
        Elements itemRecruit = document.select(".item_recruit");
        try {
            int resultCount = Integer.parseInt(document.select(".cnt_result").text().replaceAll("[^0-9]",""));
            return extracted(itemRecruit, resultCount);
        }catch (NumberFormatException ne){
            throw new CrawlerException(CrawlerErrorResult.RESULT_NOT_FOUND);
        }
    }

	@Override
    public List<JobCrawlerDto> extracted(Elements itemRecruit, int resultCount) {
       // 크롤링 로직이 너무 길어서 생략합니다.
    }
}

위의 코드가 그 동안 꽁꽁 감춰왔던 private 골칫덩이들이다. 딱 봐도 테스트하기 어렵게 생겼지 않는가..
이렇게 서비스 객체에서 크롤러를 분리하니 서비스 객체는 단 하나만 존재할 수 있었으며 코드 또한 매우 간단해졌다.

@Service
public class JobSearchServiceImpl implements JobSearchService {
    private final JsoupConnection jsoupConnection;
    private final Map<String,Crawler> crawlerMap;

    @Autowired
    public JobSearchServiceImpl(JsoupConnection jsoupConnection, Map<String, Crawler> crawlerMap) {
        this.jsoupConnection = jsoupConnection;
        this.crawlerMap = crawlerMap;
    }

    @Override
    public List<JobCrawlerDto> getSearchList(Map<String, String> searchOption, String crawlerName) throws IOException {
        Crawler crawler = crawlerMap.get(crawlerName);
        if (crawler.checkSearchOption(searchOption)){
            throw new CrawlerException(CrawlerErrorResult.OPTION_NULL_EXCEPTION);
        }

        final String recruitSort = searchOption.get("recruitSort");

        if (crawler.checkValidSortOption(recruitSort)){
            throw new CrawlerException(CrawlerErrorResult.OPTION_BAD_REQUEST);
        }

        String searchList = crawler.makeSearchListUrl(searchOption);

        try {
            Document document = jsoupConnection.get(searchList);
            return crawler.parseHTML(document);
        } catch (IOException e) {
            throw new IOException("JsoupConnect Error");
        }
    }
}

테스트하기 좋은 코드란 무엇일까..

이번 경험을 통해 테스트하기 좋은 코드에 대해 많이 고민했다.
고민 끝에 도출한 좋은 코드는

SOLID 원칙을 지키려고 노력하는 코드이다.

SOLID 원칙에 완전 적합한 코드가 무엇인 지 모르기때문에 노력이라는 키워드를 사용했다.

이 코드는 SOLID 원칙을 지키면서 리팩터링한 것은 아니다. 테스트하기 좋은 코드로 리팩터링하고 보니 SOLID 원칙에 근접했다.

서비스 객체의 책임을 분할하고 (SRP),
크롤러를 계속 추가할 수 있게 확장에 열려있으며 (OCP),
하위 타입(다양한 구직사이트 크롤러)은 언제나 상위 타입(Crawler)과 교체할 수 있고 (LSP),
서비스 인터페이스를 크롤러와 분할하고 (ISP),
구현체(Jsoup)가 아닌 인터페이스(JsoupConnection)에 의존하는 (DIP)

그런 코드가 되어 있었다.

이번 경험을 통해 SOLID원칙이 어떻게 빛을 내는 지 알게되었다.
매 순간 SOLID를 생각하며 개발하긴 어렵지만 좋은 객체지향 코드, 테스트하기 좋은 코드를 작성하기위해 원칙을 되새기며 작성해야겠다.

profile
nimoh
부족함을 인정하는 순간이 성장의 시작점이다.
이전 포스트

[JAVA] 스트림(Stream) - 2

다음 포스트

[Spring] JDBC란?

0개의 댓글