오랜만에 해보는 공개 코드 리뷰?

자바지기 2013-05-07 18:22

지난 주 SLiPP 스터디에서 TDD를 주제로 스터디를 진행했다. 스터디에서 요구사항을 제시하고 이에 대한 구현을 TDD로 진행하는 과정을 가졌다.

요구사항은 http://www.slipp.net/wiki/download/attachments/12189733/TDD+StringCalculator.pdf 와 같다. 힌트도 제공하고 있으니 약간의 시간만 투자하면 풀 수 있다. 스터디에 참여하고 있는 친구가 이 요구사항에 대한 코드를 TDD로 구현했다. 이 소스 코드에 대해 같이 코드 리뷰를 해보면 재미있을 듯하다. 이 친구에게도 도움이 되리라 생각한다. 비난은 사절한다.

먼저 테스트 코드를 살펴보자.

package net.slipp.ncrash;


import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;


import org.junit.Before;
import org.junit.Test;


public class StringCalculatorTest {


	StringCalculator calculator;


	@Before
	public void setUp() throws Exception {
		calculator = new StringCalculator();
	}


	@Test
	public void 빈_문자열을_입력할_경우_0을_반환해야_한다() throws Exception {


		assertThat(calculator.add(""), equalTo(0));
	}


	@Test
	public void 숫자_하나를_문자열로_입력할_경우_해당_숫자를_반환한다() throws Exception {
		assertThat(calculator.add("1"), equalTo(1));
	}


	@Test
	public void 숫자_두개를_컴마_구분자로_입력할_경우_두_숫자의_합을_반환한다() throws Exception {
		assertThat(calculator.add("1,2"), equalTo(3));
	}


	@Test
	public void 세_개_이상의_숫자를_컴마_구분자로_입력할_경우_모든_숫_자의_합을_반환한다() throws Exception {
		assertThat(calculator.add("1,2,3"), equalTo(6));
	}


	@Test
	public void 구분자를_컴마_이외에_New_Line을_사용할_수_있다() throws Exception {
		assertThat(calculator.add("1,2\n3"), equalTo(6));
	}


	@Test
	public void 더블_슬레쉬를_사용해_커스텀_구분자를_지정할_수_있다() throws Exception {
		assertThat(calculator.add("//;\n1;2;3"), equalTo(6));
	}
}

테스트 코드는 워낙 단순해서 리팩토링할 요소가 많이 보이지 않는다. 위 테스트 코드에 대한 구현 코드는 다음과 같다.

package net.slipp.ncrash;


import java.util.regex.Matcher;
import java.util.regex.Pattern;


public class StringCalculator {


	private int result;
	private String[] numbers;


	public int add(String text) {


		if (text.isEmpty()) {
			return 0;
		}


		calculateNumbers(getNumbers(text));


		return result;
	}


	private String[] getNumbers(String text) {
		if (text.startsWith("//")) {
			numbers = getNumbersUsingCustomDelimeter(text);
		} else {
			numbers = getNumbersUsingBasicDelimeter(text);
		}


		return numbers;
	}


	private String[] getNumbersUsingBasicDelimeter(String text) {
		String[] tokens = text.split(",|\n");
		return tokens;
	}


	private String[] getNumbersUsingCustomDelimeter(String text) {
		Matcher m = Pattern.compile("//(.)\n(.*)").matcher(text);
		m.find();
		String customDelimeter = m.group(1);
		String[] customDelimeterResult = m.group(2).split(customDelimeter);
		return customDelimeterResult;
	}


	private void calculateNumbers(String[] tokens) {
		if (tokens != null) {
			for (int i = 0; i < tokens.length; i++) {
				result += Integer.parseInt(tokens[i]);
			}
		}
	}
}

구현 코드도 깔끔하게 잘 구현했다. 위 구현 코드에 대한 공개 코드 리뷰를 한번 해보자. 위 상태로 더 이상 개선할 부분이 없을까? 최선의 선택인 것인가?

위 소스 코드는 https://github.com/ncrash/slipp 에 공개하고 있으니 저장소 fork 받아서 리팩토링을 진행해 봐도 좋겠다.

0개의 의견 from FB

BEST 의견 원본위치로↓

양지훈 2016-01-15 18:47

junit 한글 메서드 실행 오류, 구글링 하다가 같은 이슈로 확인하던 차에 찾은 방법이 있어서 여기에 공유합니다. 혹시나 다른 사람들도 여기에 오면 도움이 될 수 있을 거 같아서요.. ^^ (mac, eclipse 사용하고 있습니다. ) eclips.ini 에 -Dfile.encoding=UTF-8 설정을 하면 됩니다.

참고 : http://daclouds.tumblr.com/post/21133641126/stsspringsource-tool-suite-%EC%9C%BC%EB%A1%9C-junit-%EC%9D%84-%EC%82%AC%EC%9A%A9%ED%95%A0-%EB%95%8C

좋아요 2 싫어요 0

15개의 의견 from SLiPP

eclipse4j 2013-05-08 09:32

뭐, 빈문자열이라는 강한 조건이 있어서 상관은 없는데요, 예외를 적용해 보면

public int add(String text) { ..

시작점에서 text가 null일 경우 isEmpty는 NullPointerException 을 발생시키니까 뭔가 조치는 있어야 할 듯 합니다.

지난번 스터디에서 이부분 때문에 commons의 isEmpty나 isBlank를 사용했던거 같네요.

좋아요 0 싫어요 0

자바지기 2013-05-08 09:53

위 코드에 대해 나라면 다음과 같이 구현할 듯하다.

private int result; private String[] numbers;

위 두 개의 전역 field가 없어도 되지 않을까? 특히 String[] numbers는 인자와 return 값으로 전달하고 있어서 상태 값이 필요 없을 듯하다. 그리고 result도 calculateNumbers() 메소드에서 return하는 방식으로 구현하면 되지 않을까? 그럼 굳이 별도의 field가 필요 없을 듯하다.

  public int add(String text) {


    if (text.isEmpty()) {
      return 0;
    }


    return calculateNumbers(getNumbers(text));
  }


  private int calculateNumbers(String[] tokens) {
    int result = 0;
    if (tokens != null) {
      for (int i = 0; i < tokens.length; i++) {
        result += Integer.parseInt(tokens[i]);
      }
    }
    return result;
  }

또한 indent를 1depth로 유지한다는 요구 조건을 만족하려면 calculateNumbers() 메소드를 다음과 같이 리팩토링할 수 있겠다.

  private int calculateNumbers(String[] tokens) {
    if (tokens == null) {
      return 0;
    }


    int result = 0;
    for (int i = 0; i < tokens.length; i++) {
        result += Integer.parseInt(tokens[i]);
    }
    
    return result;
  }

나는 메소드 구현할 때 예외 상황을 미리 처리한 후에 정상 로직을 구현하는 방식으로 구현한다. 예외 처리를 미리 함으로써 얻는 효과가 많으니 한번씩 도전해 보면 좋겠다.

좋아요 0 싫어요 0

NCrash 2013-05-08 18:08

먼저 개선안에 대해 의견을 주시고 친절하게 설명까지 해주셔서 감사합니다 ^^ 리팩토링 목표에 대한 작업 진행 해본 결과입니다.

Refactoring : null_값_입력할_경우_0을_반환해야_한다 from eclipse4j - https://github.com/ncrash/slipp/commit/eae8e5808d9f03c0c9b88436ef58799e2d351e95
Refactoring : 두개의 전역 field 변수 제거 from 자바지기 - https://github.com/ncrash/slipp/commit/426c195095cfe09e349209f805a84f0cea05c11b
Refactoring : indent를 1depth로 유지한다는 요구 조건을 만족 from 자바지기 - https://github.com/ncrash/slipp/commit/f91864ce8fb8cea61e02aaf79ec9978b4b8ca2ea

링크를 이쁘게 걸고 싶은데 방법을 몰라서 투박하게 적었습니다ㅠㅠ 혹시나 각종 위키(confluence, redmine) LINK 문법을 뒤져서 테스트를 해봤으나 안되네요.. 시간 되시면 방법 좀 알려주세요!

그리고 질문 있습니다. Testcase에서 method 단위로 실행이 가능한데 한글로 된 테스트 메소드를 실행해보니 아래와 같이 오류가 발생합니다. 해결 방법 아시는 분~.. 이건 구글링 해보진 않았습니다.. 저처럼 혹시라도 찾으시는 분이 있을까봐 질문해봅니다.

좋아요 0 싫어요 0

자바지기 2013-05-08 18:55

@daekwon.kang.1018 나도 Mac에서 똑같은 에러가 발생하고 있는데 아직까지 찾지 못했다. 혹시나 찾으면 공유 좀 해주라. 혹시 OS가 윈도우즈나 리눅스라면 잘 되는 것으로 알고 있다. OS 환경이 어떻게 되는지 공유해주라. Mac이라면 같이 찾아보자.

좋아요 0 싫어요 0

NCrash 2013-05-08 19:50

@자바지기 저와 동일한 현상 겪고 계신다니 문제해결 의지가 더 강해지는데요 ^^ 저도 Mac OS입니다 버전은 형하고 동일할듯요~ 이클립스는 STS 3.x.x 입니다 아이폰에서 코멘트 날리는거라 정확히 버전이 생각나질 않네요

해결하면 공유할께요!

좋아요 0 싫어요 0

lark 2013-05-09 03:22

코드 잘보았습니다.

getNumber() 메소드 내에서 또 한번의 메소드 호출이 이루어져서 (method chaining의 depth가 깊어져서) 좀 줄여보면 어떨까 생각해보았습니다.

리팩토링을 해볼까하다가 관점을 바꿔 한번 구현해보았는데요.

그리 복잡한 소스는 아니라 설명은 소스로 대체합니다. 아래 코드도 뭔가 리팩토링이 필요할 것 같네요.^^;

```public class StringCalculator {

private static final String DEFAULT_DELIMETER = ",|\n";
private static final String CUSTOM_DELIMETER_REGEX = "//(.)\n";
private static final Pattern CUSTOM_PATTERN = Pattern.compile(CUSTOM_DELIMETER_REGEX + "(.*)");


public int add(String text) {


    if (text == null || text.isEmpty()) {
       return 0;
    }


    String delimeter = DEFAULT_DELIMETER;

    if (isCumtomDelimeter(text)) {
       delimeter = getCustomDelimeter(text);;
       text = removeCustomDelimeter(text);
    }

    return add(text.split(delimeter));
}


private int add(String[] nums) {
    int sum = 0;

    for (int i=0; i<nums.length; i++) {
       sum += Integer.parseInt(nums[i]);
    }

    return sum;
}


private String removeCustomDelimeter(String text) {
    return text.replaceAll(CUSTOM_DELIMETER_REGEX, "");
}


private boolean isCumtomDelimeter(String text) {
    return CUSTOM_PATTERN.matcher(text).matches();
}


private String getCustomDelimeter(String text) {
    Matcher m = CUSTOM_PATTERN.matcher(text);
    m.find();
    return m.group(1);
}

}```

좋아요 0 싫어요 0

NCrash 2013-05-09 06:55

@lark 제안해주신 코드 잘 봤습니다. 감사합니다^^

두가지 포인트에서 저에게 아주 도움이 되었습니다.

"else를 사용하지 마라."

* 초기 실습 - 리팩토링 단계에서 "else를 사용하지 마라." 목표를 달성하지 못했었는데 제안 해주신 코드 덕에 목표를 달성할 수 있었습니다.

요구사항 변화에 좀 더 유연하게 대처

* 코드 리뷰를 하면서 든 개인적 견해입니다. 추후 Delimeter에 대한 요구사항 늘어나거나 복잡해 졌을때 기존 코드에 비해 좀 더 유연하게 대처 할 수 있을 것 같단 견해 입니다.

소스 diff 내역을 올려봅니다^^ 원문 : https://github.com/ncrash/slipp/commit/b7be031e210c948490d63166f8f32af6342324a9

```import org.apache.commons.lang3.StringUtils;

public class StringCalculator { + private static final String DEFAULT_DELIMETER = ",|\n"; + private static final String CUSTOM_DELIMETER_REGEX = "//(.)\n"; + private static final Pattern CUSTOM_PATTERN = Pattern + .compile(CUSTOM_DELIMETER_REGEX + "(.*)");

public int add(String text) { if (StringUtils.isBlank(text)) { return 0; } -
+ return calculateNumbers(getNumbers(text)); }

private String[] getNumbers(String text) { - if (text.startsWith("//")) { - return getNumbersUsingCustomDelimeter(text); - } else { - return getNumbersUsingBasicDelimeter(text); + String delimeter = DEFAULT_DELIMETER; + + if (isCustomDelimeter(text)) { + delimeter = getCustomDelimeter(text); + text = removeCustomDelimeter(text); } +
+ return text.split(delimeter); }

private String[] getNumbersUsingBasicDelimeter(String text) {
String[] tokens = text.split(",|\n");
return tokens;
private String removeCustomDelimeter(String text) {
return text.replaceAll(CUSTOM_DELIMETER_REGEX, ""); }
private String[] getNumbersUsingCustomDelimeter(String text) {
Matcher m = Pattern.compile("//(.)\n(.*)").matcher(text);
private String getCustomDelimeter(String text) {
Matcher m = CUSTOM_PATTERN.matcher(text); m.find();
String customDelimeter = m.group(1);
String[] customDelimeterResult = m.group(2).split(customDelimeter);
return customDelimeterResult;
return m.group(1);
} +
private boolean isCustomDelimeter(String text) {
return CUSTOM_PATTERN.matcher(text).matches(); }

private int calculateNumbers(String[] tokens) { int result = 0; - if (tokens == null) { - return 0; - } -
+ for (int i = 0; i < tokens.length; i++) { result += Integer.parseInt(tokens[i]); }```

좋아요 0 싫어요 0

NCrash 2013-05-09 10:15

@자바지기 문제 해결책은 찾질 못했고 원인은 파악 됐습니다.

/Users/ncrash/Documents/workspace-sts-3.2.0.RELEASE/.metadata/.plugins/org.eclipse.debug.core/.launches

위와 같이 launch 파일명이 깨지는 것을 확인했습니다.

위에서 언급하신대로 windows에서는 확인해보니 정상적으로 한글명칭에 맞게 나오구요

추측컨데 mac만 non-ascii에 대한 대응코드가 적용되지 않아 발생하는 것 같습니다.

좋아요 0 싫어요 0

Changhwa Oh 2013-05-09 15:15

mac에서 IntelliJ로 한글메소드 사용해서 할때도 많은데요 인텔리J에서는 별문제 없는듯합니다.

좋아요 0 싫어요 0

benghun 2013-05-09 17:50

좀더 디테일한 테스트를 작성해보면 어떨까요?

class ExprParserTest {
    @Test public void 디폴트구분자로_표현식에서_1과2를_찾을수있다() {
        int[] actual = ExprParser.parse("1,2");


        assertEquals(1, actual[0]);
        assertEquals(2, actual[1]);
    }
    @Test public void 커스텀구분자로_표현식에서_1과2를_찾을수있다() {
        int[] actual = CustomExprParser.parse("\\;1;2");


        assertEquals(1, actual[0]);
        assertEquals(2, actual[1]);
    }
}

이제 Calculator는 파싱에서 독립될 수 있을 것 같네요.

class Calc {
    public int add(String expr) {
        int[] nums;
        if (ExprParser.definedCustomDelimeter(expr)) 
            nums = CustomExprParser.parse(expr);
        else 
            nums = ExprParser.parse(expr);
        return sum(nums);
    }
    private int sum(int[] nums) {
        int total = 0;
        for (int num : nums) total += num;
        return total;
    }
}

좋아요 0 싫어요 0

NCrash 2013-05-10 06:54

@Changhwa Oh 역시 IntelliJ는 잘 되는군요 ㅠㅠ

이런 현상 어디다 버그 리포팅 하면 되는지 아시는 분???

좋아요 0 싫어요 0

양지훈 2016-01-15 18:47

참고 : http://daclouds.tumblr.com/post/21133641126/stsspringsource-tool-suite-%EC%9C%BC%EB%A1%9C-junit-%EC%9D%84-%EC%82%AC%EC%9A%A9%ED%95%A0-%EB%95%8C

좋아요 2 싫어요 0

Louis Bae 2016-02-03 14:15

@Changhwa Oh intellij 에서 한글 메소드 사용시 컴파일 오류가 나는데 혹시 해결 방법을 아시나요? vmoption 에 encoding 을 추가해 봐도 안되네요.

--> 해결책을 찾았네요. java compiler additional parameter 에 아래 옵션을 추가하니 잘 되네요.^^; 좀더 깔끔한 방법이 있으면 공유 부탁 드립니다. 자문자답 이었습니다.ㅎ

좋아요 0 싫어요 0

cjunekim 2016-05-03 22:19

가장 큰 문제는 TDD 진행이 너무 homogeneous하다(calculator.add만 테스트)는 점입니다. 이것 때문에 SUT의 설계가 OOP적이지 못하게(그리고 testability가 떨어지고, 확장하기 어렵게) 나왔습니다.

우선 테스트 코드만 보면, 저라면 다음 부분을 고치겠습니다. 그대로 두면 테스트 관리하기가 점점 어려워질 수 있습니다.

assertThat(calculator.add(x), equalTo(N)); 이라는 문장의 중복(duplication).
homogeneous한 테스트들. calculator.add를 테스트하는 과정에서 getNumbers를 테스트하고, 또 getCustomDelimeter를 테스트하고 등등 heterogenous한 테스트로 바꾸기.

예를 들어, http://poj.org/problem?id=2562 이런 간단한 문제의 경우에도, 착실하게 Baby Step을 밟아서 TDD를 한다면, 전혀 이질적인 테스트 케이스(다른 함수를 테스트하는)들이 6가지 종류는 되어야 합니다.

좋아요 0 싫어요 0

자바지기 2016-05-04 15:44

@cjunekim 답변 감사합니다. TDD에 대한 맛보기 용도로 활용하는 예제인데 창준님 주신 답변 참고해서 한 단계 더 깊이 있는 경험할 수 있도록 추가해 봐야겠네요.

좋아요 0 싫어요 0

연관태그

← 목록으로

"else를 사용하지 마라."

요구사항 변화에 좀 더 유연하게 대처

SNS공유

연관태그