Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[또링] 자동차 경주 - TDD 미션 리뷰 요청합니다. #81

Merged
merged 51 commits into from
Feb 17, 2020

Conversation

jnsorn
Copy link

@jnsorn jnsorn commented Feb 13, 2020

TDD를 처음 해봐서 많이 미숙한데, 많이 리뷰해주시면 열심히 수정해보겠습니다! 감사합니다~☺

mintjordy and others added 30 commits February 11, 2020 13:53
Docs : add commit message convention
Copy link

@dave915 dave915 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

또링 안녕하세요! 리뷰를 맡게된 데이브입니다.
너무 잘하셔서 하마터면 바로 머지 할 뻔 했네요 👍
몇가지 피드백 남겼으니 적용해 보시고, 궁금한점 있으시면 편하게 DM주세요!

src/main/java/racingcar/domain/Cars.java Outdated Show resolved Hide resolved
src/main/java/racingcar/domain/Cars.java Outdated Show resolved Hide resolved
src/main/java/racingcar/domain/Winner.java Show resolved Hide resolved
src/main/java/racingcar/splitter/NameSplitter.java Outdated Show resolved Hide resolved
src/main/java/calculator/Number.java Outdated Show resolved Hide resolved
src/main/java/racingcar/Application.java Outdated Show resolved Hide resolved
src/main/java/calculator/Calculator.java Outdated Show resolved Hide resolved
@jnsorn
Copy link
Author

jnsorn commented Feb 17, 2020

안녕하세요, Dave! 말씀해주신대로 이동이라는 것을 추상화하여 어떤 이동 전략을 쓰냐에 따라 🚗car의 움직임이 달라지도록 설계해보았습니다.

RandomMovingStrategy 객체는 생성시 자동으로 난수가 생성되고, isMovable()이 호출되었을 때 난수에 따라 true(4-9사이) or false(0-3사이)를 반환합니다.

또한, 테스트를 위해 작성한 ForwardMovingStrategy 객체는 isMovable()호출 시 무조건 전진을 합니다.(true 반환)

현재, ForwardMovingStrategy를 이용하여 car의 run()과 cars의 run()이 제대로 동작하는지 테스트를 한 상태입니다.

💡 저는 추가로 0,3,4,9 숫자 값이 들어왔을 때, RandomMovingStrategy의 isMovable()이 제대로 동작하는지 테스트를 해보고 싶어서 아래와 같은 코드를 추가하였고(현재 원격저장소에는 올라가지 않았습니다)

image

그에 따라 프로덕션 코드도 아래와 같이 수정하였습니다.

image

하지만 수정하고 나니, 제가 생각했던 RandomMovingStrategy 객체의 의도를 조금 벗어났다는 생각을 하였습니다. 제가 생각했던 RandomMovingStrategy 객체의 의도는 랜덤 값에 의하여 4이상 9이하 전진이라는 기준을 가지고 작동하는 것이었습니다. 하지만 매개변수 생성자를 구현하여 임의의 값을 받게 되면, 랜덤으로 생성된 값에 의해 작동하는 객체가 아니기 때문에 이렇게 구현하면 안될 것 같다라고 생각이 들었습니다.... 하지만 저러한 의도를 벗어나지 않고 위와 같은 테스트 코드를 만드는 방법이 생각나지 않아서 질문 드립니다. ㅠ_ㅠ

최대한 저의 생각을 잘 표현하고 싶어서 주절주절쓰다보니 길어진 것 같습니다. 결론은

1. 위의 테스트 코드를 작성할 필요가 있는가
TDD로 작성한 프로그램이기 때문에, 제가 원하는 테스트가 작성되어있어야 한다고 생각하는데, 어쩌다 보니 리팩토링 과정에서 설계가 변경되어 이러한 상황이 벌어진 것 같습니다. 이런 경우, 어차피 run()을 테스트하므로 위의 테스트코드는 작성할 필요가 없는지, 아니면 테스트 코드를 작성하는 것이 맞고, 설계를 다시 해야하는지.. 모르겠습니다. (맞다면 어떻게 프로덕션 코드를 구현해야할지 감이 안잡힙니다.)

2. RandomMovingStrategy 객체를 잘못 생각하고 있는 것인가.
전략패턴을 거의 써보지 않아서 이런식으로 사용하는 것이 맞나 의문이 듭니다..

@dave915
Copy link

dave915 commented Feb 17, 2020

또링! 전략패턴은 정말 잘 구현하셨어요.
어떤 부분이 헷갈려 하시는지 알 것 같아서 의견 남깁니다.

전략패턴에 대해 자동차 이외에 또 다른 예로 화가로 예를 들어보겠습니다.
화가는 Pen을 이용해서 선을 그릴 수 있습니다. 화가 입장에서는 선을 그릴 수 있는 Pen이 있으면 되지 해당 펜이 어떤 색인지는 화가는 고려 하지 않아도 됩니다. 또한 Pen의 구현체들은 색을 제공하는 책임만 다하면 됩니다.
RainbowPen에서 어떤 기준으로 색상을 내려주는지 Painter는 관심이 없기 때문이죠.
다시 돌아가서 Car의 입장에서 보면 Car는 MovingStrategy 가 이동가능여부만 제공해준다면 그 이상 어떤기준으로 이동가능여부가 판단 되는지는 아무런 상관이 없습니다. 그렇기 때문에 현재 MovingStrategy로 추상화한 전략패턴은 잘 사용 한 것으로 보입니다. :)

이제 Car와 별개로 RandomMovingStrategy만 놓고 봤을때 아래 예시대로면 RainbowColor죠.
RainbowColor에서 colors를 shuffle 후 첫번째 숫자를 뽑았을때 원하는 색상이 나오는지 테스트를 해줘야 합니다. 해당 기능은 RainbowPen의 요구사항이기 때문이죠.

다시 RandomMovingStrategy로 돌아가서 보면 해당 객체는 랜덤 값에 의하여 4이상 9이하 전진이라는 기준을 가지고 작동 이라는 요구사항을 보장하기 위해 해당 요구사항을 테스트하는 테스트 코드가 작성 되어야 하는것이 맞다고 생각합니다. 그리고 테스트 하기 위해선 불확실한 랜덤값을 개발자가 직접 지정하여 넘겨주어야 해당 값으로 정상적으로 4 이상 9 이하 일시 전진이라는 결과가 테스트 가능합니다.

질문주신

아니면 테스트 코드를 작성하는 것이 맞고, 설계를 다시 해야하는지.. 모르겠습니다. (맞다면 어떻게 프로덕션 코드를 구현해야할지 감이 안잡힙니다.)

요부분에 대한답은 지금 캡쳐해서 보여주신대로 구현하시면 될 것 같습니다. 👍

public class Main {
    public static void main(String[] args) {
        Painter painter = new Painter(new BluePen());
        painter.draw();

        Painter painter2 = new Painter(new RainbowPen());
        painter2.draw();
    }
}
public class Painter {
    private Pen pen;

    public Painter(Pen pen) {
        this.pen = pen;
    }

    public void draw() {
        System.out.println(String.format("%s 선을 그렸습니다.", pen.color()));
    }
}
public class BluePen implements Pen {

    @Override
    public String color() {
        return "Blue";
    }
}
public class RainbowPen implements Pen {
    private List<Integer> colors;

    public RainbowPen() {
        colors = Arrays.asList(1, 2, 3);
    }

    public RainbowPen(List<Integer> colors) {  
        // 각 숫자에 맞는 색상이 맞는지 테스트 하기 위해 [1] or [2] or [3] 전달
        this.colors = colors;
    }

    @Override
    public String color() {
        Collections.shuffle(colors);
        int color = colors.get(0);

        if (color == 1) {
            return "빨강";
        }

        if (color == 2) {
            return "주황";
        }

        return "노랑";
    }
}

밤에 리뷰를 해서 그런지 너무 주저리 주저리 코멘트를 단 것 같네요 ㅎㅎ
혹시나 더 궁금한 부분이 있다면 편하게 DM주세요 :)

Copy link

@dave915 dave915 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

또링! 피드백 반영 잘 해주셨어요! 💯
질문 주신 부분에 대해 코멘트 남겼습니다. 확인해보세요 😁
해당 요청은 머지 할게요!
수고하셨습니다!

@dave915 dave915 merged commit 3ee4f6b into woowacourse:jnsorn Feb 17, 2020
jnsorn added a commit to jnsorn/java-racingcar that referenced this pull request Feb 17, 2020
@jnsorn
Copy link
Author

jnsorn commented Feb 18, 2020

제가 고민하던 부분들을 정확히 설명해주셨고, 덕분에 명쾌하게 해결되었습니다. 글만 읽어도 이해가 가도록 친절히 설명해주셨는데 예시까지 들어주시니 더욱 와닿는 것 같아요😊 랜덤값으로 동작하는 객체에 직접 값을 지정하는 것이 과연 맞는 것인가라는 고민을 계속 해왔는데, 아무래도 테스트를 위해서 직접 지정해주는 수 밖에 없겠네요! 다시 한 번 정말 감사드립니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants