Step1 학습 테스트 실습
@Test
@DisplayName("성공 - 중복이 제거된 Set 크기를 확인할 수 있다.")
void success_set_size_test() {
//when
int size = numbers.size();
int expected = 3;
//then
assertThat(size).isEqualTo(expected);
처음 짠 로직은 Set size를 체크시 size() 메소드를 사용해서 추출한 후에 비교 하고 있다.
아래와 같은 피드백을 받았다.
💡 size를 굳이 추출하지 않고, hasSize 로 확인하실 수 있을 것 같아요 ~
그래서 아래와 같이 수정하였다.
assertThat(numbers).hasSize(3);
Step2 문자열 덧셈 계산기
public int sum(String text) {
int sum = sumByCustomDelimiter(text);
if (sum != 0) {
return sum;
}
return sum(text.split(DELIMITER));
}
private int sumByCustomDelimiter(String text) {
Matcher matcher = CUSTOM_DELIMITER_PATTERN.matcher(text);
if (matcher.find()) {
String customDelimiter = matcher.group(1);
return sum(matcher.group(2).split(customDelimiter));
}
return 0;
}
기본 구분자(쉼표, 콜론) 혹은 커스텀 구분자를 지정하여 숫자를 더할 수 있다.
public sum 메소드에서 먼저 커스텀 구분자를 확인 후 없으면 기본 구분자를 이용하여 합계를 더하는 로직이다.
아래와 같은 피드백을 받았다.
💡 sumByCustomDelimiter() 메소드에서 기본 구분자로 계산한 값을 리턴하게 되면 public sum() 메소드에서 분기가 필요 없는거 아닐까요 ?
sumByCustomDelimiter() 메소드 에서 if문을 타지 않는다면 0을 반환하는게 아니라 기본 구분자를 이용한 합계를 구한 답을 반환하면 된다.
sumByCustomDelimiter() 메소드가 불필요하단 생각이 들어 해당 메소드를 지우고 로직을 public sum으로 옮겼다.
public int sum(String text) {
Matcher matcher = CUSTOM_DELIMITER_PATTERN.matcher(text);
if (matcher.find()) {
String customDelimiter = matcher.group(1);
return sum(matcher.group(2).split(customDelimiter));
}
return sum(text.split(DELIMITER));
}
public class Input {
private final String text;
public Input(String text) {
this.text = text;
}
public boolean isEmptyOrNull() {
return this.text == null || this.text.isBlank();
}
public boolean isSingleDigit() {
return this.text.length() == 1 && Character.isDigit(this.text.charAt(0));
}
}
Input 클래스는 주어진 문자열에 대한 검증을 처리하는 로직이다.
기능 요구사항중에 아래와 같은 사항이 있다.
- 숫자 하나를 문자열로 입력할 경우 해당 숫자를 반환한다.(예 : “1”)
public boolean isSingleDigit() {
return this.text.length() == 1 && Character.isDigit(this.text.charAt(0));
}
구현은 위와 같이 했다.
아래와 같은 피드백을 받았다.
💡 11도 숫자 하나 아닐까요 ?
11, 111, 1111도 숫자 하나인데 한자리의 숫자인 케이스만 생각했다. 주어진 예시만 보고 단편적으로 생각했나보다.. 😥
생각해보면 숫자 한자리를 split() 하면 숫자 한자리가 나올뿐이다. 그럼 그걸 그대로 sum 메소드로 던져줘도 똑같은 값이 나오지 않을까?
isSingleDigit() 메소드를 삭제하고 테스트 케이스를 수정했다.
// 수정 전
@Test
public void splitAndSum_숫자하나() throws Exception {
int result = StringAddCalculator.splitAndSum("1");
assertThat(result).isEqualTo(1);
}
// 수정 후
@ParameterizedTest
@CsvSource(value = {"1:1", "11:11", "111:111"}, delimiter = ':')
public void splitAndSum_숫자하나(String text, int number) throws Exception {
int result = StringAddCalculator.splitAndSum(text);
assertThat(result).isEqualTo(number);
}
패키지 구조는 아래와 같이 작성했다.
├── java-racingcar
│ └── src
│ ├── main
│ │ ├── java
│ │ │ └── calculator
│ │ │ └── Calculator
│ │ │ └── Input
│ │ │ └── NegativeNumberException
│ │ │ └── NotNumberException
│ │ │ └── StringAddCalculator
│ │ │ └── Util
Uitl 클래스는 아래와 같다.
public class Util {
public static int strToInt(String str){
try{
return Integer.parseInt(str);
}catch (NumberFormatException ex){
throw new NotNumberException();
}
}
}
String을 int로 변환하는 strToInt() 메소드를 StringAddCalculator와 Calculator에서 필요하여 Util 클래스의 정적 메소드로 생성하였다. 이런 방법이 적절하게 구현한 것인지 의구심이 들어서 질문을 하였다.
아래와 같은 피드백을 받았다.
💡 클래스를 분리하는 건 괜찮은데, 현재 요구사항에선 너무 과도하게 분리 된 느낌이 있네요.
Util의 메서드는 다른 클래스에서 참조하질 않으니 멤버 메서드로도 충분할 것 같구요. Calculator는 여전히 문자열을 받아 파싱하고 있어서 StringAddCalculator와 역할상의 차이를 잘 모르겠습니다.
Input은 입력값에 대한 검증을 책임지고 있어서 괜찮을 것 같긴 한데, 결국 음수에 대한 유효성 검사는 다른데서 하고 있어서 이것도 역할 분리가 완벽하게 되지 않은 것 같아요
역할에 따라 나누는 건 좋다고 생각합니다. 하지만 역할 분리와 추상화 같은 작업은 그게 필요할 때 이루어져야 의미가 있다고 생각해요
여기 까지는 실무 관점에서 봤을 때의 피드백이구요, 지금은 학습을 하는 과정이니 이런 도전을 더 많이 해야 한다고 생각합니다. 좋은 도전이라고 생각해요 :)
위의 피드백을 반영하면서 Util의 strToInt() 메서드는 한군데 밖에 사용하지 않는다. 그래서 멤버 메서드로 이동하는 편이 좋다고 생각이 든다.
Input은 입력값에 대한 검증을 하는 값 객체 역할로 보인다. 리뷰어님 말씀대로 음수에 대한 유효성 검사는 다른 클래스에서 하고 있어 해당 검사도 Input으로 옮겼다.
또한, Input에서 문자열을 split하고 숫자로 파싱하는 메소드를 추가하여 숫자타입의 리스트로 반환하도록 했다.
Calculator은 제거하고 StringAddCalculator에서 합계를 구한다.
라이브 강의를 보다가 기본 분리와 커스텀 분리를 인터페이스를 사용한 것을 보고 추가로 리펙토링을 하였다.
public interface Spliter {
String[] split(String text);
}
import java.util.regex.Matcher;
public class CustomSpliter implements Spliter {
private static final int CUSTOM_DELIMITER_INDEX = 1;
private static final int CUSTOM_DELIMITER_TEXT = 2;
private Matcher matcher;
public CustomSpliter(Matcher matcher) {
this.matcher = matcher;
}
@Override
public String[] split(String text) {
String customDelimiter = matcher.group(CUSTOM_DELIMITER_INDEX);
return matcher.group(CUSTOM_DELIMITER_TEXT).split(customDelimiter);
}
}
public class DefaultSpliter implements Spliter {
private static final String DELIMITER = "[,:]";
@Override
public String[] split(String text) {
return text.split(DELIMITER);
}
}
Input 객체의 역할은 값 검증을 하기 하는 로직만 들어 있는게 책임상 맞다는 생각이 들어서 기존 계산로직을 StringAddCalculator로 옮겼다.
기존 Input
public class Input {
private static final String DELIMITER = "[,:]";
private static final Pattern CUSTOM_DELIMITER_PATTERN = Pattern.compile("//(.)\n(.*)");
private static final int NEGATIVE_NUMBER_STANDARD = 0;
private static final int CUSTOM_DELIMITER_INDEX = 1;
private static final int CUSTOM_DELIMITER_TEXT = 2;
private final String text;
public Input(String text) {
this.text = text;
}
public boolean isEmptyOrNull() {
//..
}
public List<Integer> toNumbers() {
//..
}
private List<Integer> toNumbers(String[] tokens) {
//..
}
public int toIntBy(String str) {
//..
}
private void validateNegativeBy(int number) {
//..
}
}
수정 후 Input
public class Input {
private static final int NEGATIVE_NUMBER_STANDARD = 0;
private final String text;
public Input(String text) {
this.text = text;
}
public boolean isEmptyOrNull() {
//..
}
public void validateNegativeNumbers(int[] numbers) {
//..
}
private void validateNegativeNumber(int number) {
//..
}
}
아래는 다른 리뷰에서 참고한 내용이다.
null과 empty를 함께 제공해주는 어노테이션인 @NullAndEmptySource 사용
//수정 전
@Test
public void splitAndSum_null_또는_빈문자() {
int result = StringAddCalculator.splitAndSum(null);
assertThat(result).isEqualTo(0);
result = StringAddCalculator.splitAndSum("");
assertThat(result).isEqualTo(0);
}
//수정 후
@NullAndEmptySource
@ParameterizedTest(name = "입력값: {0}")
public void splitAndSum_null_또는_빈문자(String input) {
int result = StringAddCalculator.splitAndSum(input);
assertThat(result).isEqualTo(0);
}
매직넘버를 기능 요구사항에 맞춰 의미를 부여한 상수화
if (input.isEmptyOrNull()) {
return 0;
}
private static final String ZERO = 0; // 비추천
private static final String DEFAULT_RETURN_VALUE = 0; // 추천
StringAddCalculator클래스 분리
- 문자열 덧셈 계산기의 기능을 2개로 클래스로 분리할 수 있다.
- 문자열을 구분자로 나눠서 숫자 배열로 변경한다.
- 숫자를 더한다.
README.md 활용
- 필요한 기능 나열(chceckbox)
- commit마다 check 하기
- 성격이 유사한 기능끼리 뭉쳐서 객체 도출
정리한 내용을 바탕으로 리펙토링을 해보았다.
Step3 자동차 경주
public class Cars {
private final List<Car> cars;
public Cars(List<Car> cars) {
this.cars = cars;
}
public void startRacing(int racingCount) {
OutputView outputView = new OutputView();
for (int i = 0; i < racingCount; i++) {
racingCar();
outputView.printRacing(cars);
}
}
//..
자동차 경주 결과를 실시간으로 출력해야 하는 요구사항이 있었고 위와 같이 구현을 했다.
아래와 같은 피드백을 받았다.
💡 만약 View가 바뀐다면, 여기까지 사이드이펙트 범위에 들어오게 될 텐데, 의도하신게 맞을까요 ?
위와 같이 구현하기 전 처음에는 아래와 같은 로직을 생각했다.
public class Cars {
private final List<Car> cars;
public Cars(List<Car> cars) {
this.cars = cars;
}
public Map<Integer, List<Car> cars> startRacing(int racingCount) {
Map<Integer, List<Car> cars> steps = new TreeMap();
for (int i = 0; i < racingCount; i++) {
racingCar();
steps.put(i, cars);
}
return steps;
}
//...
public class RacingGame {
public static void main(String[] args) {
//..
Map<Integer, List<Car> cars> steps = cars.startRacing(racingCount);
new OutputView().printRacing(steps)
}
public class OutputView {
public static final String OUTPUT_CAR_POSITION = "-";
public void printRacing(Map<Integer, List<Car> cars> steps) {
// steps 별로 결과 출력
}
startRacing 메소드의 반환값으로 회차별로 모든 실행결과를 담은 Map을 리턴하여 OutputView에 넘겨주어 출력을 한번에 하려고 했다.
하지만 steps.put(i, cars); 부분서 cars의 객체의 주소값이 같아서 map의 결과값이 스텝별로 아닌 최종 결과 값으로 모두 동일하게 나와서 사용하지 못했다. 그래서 질문을 하였더니 아래와 같은 피드백을 받았다.
💡 모든 key에 같은 값(cars)을 넣어주고 계셔서 결과값은 동일한게 맞습니다. 이런 결과를 원하신다면 매번 새로운 Car들을 만들어서 List로 넘겨줘야 할 것 같아요.
하지만, 굳이 Map으로 리턴을 안 하고 바깥에서 for문을 돌면 그냥 cars를 넘겨주는 것만으로도 의존성을 제거할 수 있지 않을까요?
startRagcing 메소드에서 for문을 돌리지 않고 RacingGame의 main 메소드서 for문을 돌리면 해결되는 문제였다.
public class Car {
private static final int MOVE_CAR_CONDITION = 4;
private int position;
private GenerateNumber generateNumber;
public void moving() {
if (generateNumber.number() >= MOVE_CAR_CONDITION) {
this.position++;
}
}
//..
PR을 날린 후 문득 의문점이 생겼다.
generateNumber 필드가 Car 객체의 인스턴스 필드로 있어도 괜찮은걸까?
의도는 자동차 전진 테스트 코드 작성시 TestNumber를 주입받아서 원하는 동작의 테스트를 구현하고자 하는거였다.
인터페이스 타입이 어색한 느낌이 들어서 moving()의 파라미터로 받을 껄 그랬나 싶은 생각이 들어서 질문을 하였고, 아래와 같은 피드백을 받았다.
💡 둘 다 괜찮은 방법일 것 같습니다. Car 클래스가 이미 움직인다는 책임을 가지고 있는 상태고, 추상화해주신 부분은 어떻게 움직일지에 대한 전략이기 때문에, 멤버변수든 매개변수든 상관 없을 것 같아요!
아마 이름이 GenerateNumber라서 조금 어색해 보이는 감이 있는거 아닐까 싶은데, MoveStrategy 정도로 바꿔보면 어떨까요~?
public Car moving() {
if (generateNumber.number() >= MOVE_CAR_CONDITION) {
return new Car(position++);
}
}
moving에서 상태값 변경 후 불변 객체로 반환하는 코드를 종종 보았다. 그래서 위와 같이 코드를 짜려 했지만 일급 컬렉션의 cars 필드의 car를 바꿔치기 해야해서 로직 작성하는데 번거로웠다. 그래서 그냥 상태값만 변경하고 말았는데 이 부분도 궁금해서 질문을 하였고, 아래와 같은 피드백을 받았다.
💡 네 말씀하신 것 처럼 Car를 불변객체로 활용 할 수도 있겠지만, 현재 Car가 VO처럼 쓰이고있질 않아서 그냥 상태를 가지고 가는게 좀 더 자연스러울 것 같습니다.
Step4 자동차 경주(우승자)
public class Split {
private static final String DEFAULT_DELIMITER = ",";
public List<String> toCarNames(String carName) {
return Arrays.stream(carName.split(DEFAULT_DELIMITER))
.collect(Collectors.toList());
}
}
1대 이상의 자동차 이름을 쉼표를 구분자로 받는 요구사항이 있어 해당 로직을 구현 및 테스트 하기 위해 Split 클래스로 분리하였다.
ex). kwc,kim,yap
아래와 같은 피드백을 받았다.
💡 이거 메인 기능은 결국 String.split밖에 없어보이는데 클래스로 뺐을 때 어떤 이득이 있나요 ?
아래와 같이 대답 하였다.
💡 자동차 이름은 쉼표(,)를 기준으로 구분한다
해당 요구사항을 테스트 하고 싶어 별도 클래스의 public 메소드로 분리했습니다.
조금 과한 클래스 분리 일까요? 조언 부탁드립니다.
💡 말씀하신 테스트는 Cars에서 문자열을 입력받고 여러개의 Car가 원한대로 만들어지는지 정도로 봐도 되지 않을까요 ?엄밀히 따지면 Split 이라는 클래스는 지금 파라미터로 들어오는 carName이 실제 carName으로 쓰일 녀석이 들어온건지, 리턴되는 값이 carNames의 목적에 맞게 쓰일건지는 관심사가 아니라고 생각해요.
단순히 들어온 문자열을 , 를 기준으로 분리하고 리턴할 뿐이라서 되게 범용적인 책임을 가지고 있는걸로 보이거든요
물론 이런식으로 나누셔도 상관은 없지만, 딱히 메리트가 없어보여서 말씀 드렸습니다 ~
약간 주관적이긴 할텐데, 저는 개인적으로 과한 분리라고 생각합니다 ~
private void printCarPosition(Car car, StringBuilder sb) {
sb.append(car.getCarName() + OUTPUT_CAR_COLON);
출력할 때 이게 맞나~ 싶었는데 역시나..
아래와 같은 피드백을 받았다.
💡 Car에 toString을 오버라이드 하면 꽤 간결해지지 않을까요 ?
@Override
public String toString() {
return carName + OUTPUT_CAR_COLON;
}Car 객체에서 toString을 오버라이딩 하면 sp.appendcar) 로만 사용할 수 있다!!!
찝찝한거 해결!!
public class Car {
private static final int MOVE_CAR_CONDITION = 4;
private Name carName;
private int position;
private MoveStrategy moveStrategy;
public Car(Name carName, MoveStrategy moveStrategy) {
this.carName = carName;
this.moveStrategy = moveStrategy;
}
//..
↓↓ 테스트 given 절
// given
Cars cars = new Cars(
List.of(
new Car(new Name("a"), new TestMove(0)),
Car에서부터 Name 객체를 받아서 생성하고 있었다.
아래와 같은 피드백을 받았다.
💡 어차피 Name은 Car에서만 쓰이는 것 같은데, 문자열을 받고 Car 생성자에서 Name으로 만들어주는 건 어떨까요 ?
왜 개발할 때는 이런 생각을 못했을까!! 피드백 주신 내용으로 리팩토링 후 더 깔끔한 느낌이 든다
// then
assertThat(winners).hasSize(2)
.extracting("carName", "position")
.containsExactly(
tuple(new Name("b"), 5),
tuple(new Name("c"), 5)
);
}
객체로 이루어진 리스트를 검증할때 위와 같이 로직을 짠다.
아래와 같은 피드백을 받았다.
💡 개인적으로는 약간 가독성이 떨어지는 느낌인데, list의 이름과 position을 검증하고 싶으신거라면 각각 따로 검증하는건 어떨까요 ?
테스트에서는 반복문, 조건문 등을 지양해야 한다고 배웠다. 그래서 assertJ 문법을 사용해서 메소드 체이닝으로 구현하였다. 사실 저것도 내부적으로는 같은 반복문이길 할테지만 개인적으론 가독성이 나쁘지 않아서 아래와 같이 질문을 하였다다.
💡 💡 💡
말씀하신 사항이 아래와 같은 테스트 로직일까요?
@Test
@DisplayName("성공 - 레이싱이 끝난 후 우승자의 이름을 구한다.")
void success_racing_winner_names() {
// given
Cars cars = new Cars(
List.of(
new Car("a", new TestMove(0)),
new Car("b", new TestMove(5)),
new Car("c", new TestMove(9))
)
);
fiveStepRacing(cars);
// when
List<Car> winners = cars.findWinners();
// then
assertThat(winners).hasSize(2)
.extracting("carName")
.containsOnly(new Name("b"), new Name("c"));
}
@Test
@DisplayName("성공 - 레이싱이 끝난 후 우승자의 위치를 구한다.")
void success_racing_winners() {
// given
Cars cars = new Cars(
List.of(
new Car("a", new TestMove(0)),
new Car("b", new TestMove(5)),
new Car("c", new TestMove(9))
)
);
fiveStepRacing(cars);
// when
List<Car> winners = cars.findWinners();
// then
assertThat(winners).hasSize(2)
.extracting("position")
.containsOnly(5, 5);
}
혹은 extracting(), containsExactly() 의 메소드 체이닝이 가독성이 떨어진다는 말씀이실까요?
혹..은.. 아래와 같이 assertAll을 사용해서 리스트에서 get으로 하나씩 꺼내서 검증하라는 말씀이실까요?
개인적으로는 언급해주신 부분에서 가독성이 떨어진다는 걸 크게 못느껴서요 ㅠㅠ
조언 부탁드립니다..!
// when
List<Car> winners = cars.findWinners();
// then
assertAll(
() -> assertThat(winners.get(0)).isEqualTo("b"),
() -> assertThat(winners.get(1)).isEqualTo("c")
);
💡 오 아뇨 제가 말씀드린 의도대로 한다면 사실
List<Name> winnerNames = winners.stream.map(Car::getName).collect(toList())
List<Position> winnerPositions = winners.stream.map(Car::getPosition).collect(toList())
assertThat(winnerNames).equals("b","c");
assertThat(winnerPositions).equals(5,5);
이런식으로 됐을거에요! 개인차가 있을 수 있어서 굳이 안바꾸셔도 되긴 합니다~
저는 tuple() 로 한번 감싸야되는거랑 뎁스가 한번 더 들어가게 되는게 보기 불편해서 말씀드렸어요 ~
Kotest나 Spock이었다면 좀 더 적극적으로 권해봤을텐데 Junit으로는 자바로 뽑아야 되니까 호불호가 갈릴거같아서 강하게 어필을 못하겠네요 ㅋㅋ
Step5 자동차 경주(리팩토링)
미션완료!! 수강생들의 PR이 슬랙에 올라와서 항상 지켜보고 있다.(피드백 줍줍..)
리뷰어님한테 칭찬도 받고 첫번째 미션은 제일 먼저 끝내서 기분이 좋다 ㅎㅎ
수료하는 날까지 출바알~~!!
참고
'외부활동 > TDD, 클린 코드 with Java 17기' 카테고리의 다른 글
| TDD, 클린 코드 with Java 17기 종료(모든 미션 수료) (0) | 2023.12.13 |
|---|---|
| 수강신청 - 레거시 코드 리팩터링 회고 (0) | 2023.11.25 |
| 사다리타기 - FP, OOP 회고 (0) | 2023.11.25 |
| 로또 - TDD 회고 (0) | 2023.11.11 |
| TDD, 클린 코드 with Java 17기 시작 (0) | 2023.10.30 |
댓글