[우아한테크코스 4기] Level 1 7일차 회고

Jihoon Oh·2022년 2월 14일
level1우아한테크코스회고
0

우아한테크코스 4기

목록 보기
4/43

우테코 첫 주말을 보내고, 벌써 월요일이 되었다. 짧은 주말은 꿀맛같은 휴식을 주면서도, 코드리뷰와 개인 공부로 헛되이 보낼 수는 없던 주말이었다.

코드리뷰

토요일에 이번 미션의 내 담당 리뷰어이신 제이로부터 자동차 경주 미션의 첫 번째 피드백을 받았다. Pull Requests를 하면서 페어 프로그래밍동안 개인적으로 궁금했던 사항들을 PR 내용으로 적어서 여쭤봤는데, 친절하게 답변해주셨다.

그냥 딱 봐서는 뭐야! 해결책은 하나도 안줬잖아! 라고 생각할 수도 있는데 내가 어떤 방법들을 고민해보고, 어떤 의도로 그러한 방법을 생각했는지에 대한 언급을 충분히 하지 않았던 것 같다. 그래서 이 부분은 다시금 정리해서 PR을 수정할 때 작성했다.

지적받은 부분들

제이가 의견을 제시해준 모든 부분에 대해서 작성하고 넘어가기에는 적을 내용이 많을 것 같아서, 핵심적이고 생각이 드는 부분만 짚고 넘어가기로 하자.

사실 이 부분은 자동차 경주 미션은 아니고, 그 전에 TDD 연습으로 내준 문자열 계산기 미션 부분이어서 잘 생각하지 않고 넘어갔다. 피드백을 받고 생각해보니 이렇게 assertThat()을 여러 번 사용하는 것이 아니라 다른 테스트 방법을 적용하는 것이 훨씬 간단하게 테스트를 할 수 있었다. @ParameterizedTest를 이용한 방법으로 수정.

확실히 전에는 그냥 @Test 어노테이션 쓰고 assertThat이나 assertThatThrownBy를 무한정 남발해서 썼었는데, 테스트하는 가짓수가 적을 때는 모르겠지만 많아지면 정말 보기 어려운 코드가 된다. 이번에 TDD 공부하면서 @ParameterizedTest를 배웠는데, 중복되는 테스트 코드 제거도 쉽고 훨씬 직관적인 테스트를 짤 수 있다는 장접이 있다.

왜 이 부분을 아무 생각 없이 넘어갔을까? 아마 어차피 랜덤 값의 최대 최소 boundary로 주는 값이니까 MIN, MAX로 해도 되겠지 라는 생각이었던 것 같다. 하지만 다시 생각해보니 그냥 봐서는 이 값이 랜덤 값의 최대 최소인지, 아니면 다른 어떤 값의 최대 최소인지를 전혀 알 수 없을 것 같다.

게다가 제이의 피드백대로, 이 상수들이 Cars에서 관리되는 것도 적절하지 않은 것 같다. 생각해보면 0~9 사이의 랜덤값을 생성하는 규칙은 게임 전체에서 관리되어야 하지 Car의 일급 컬렉션인 Cars가 가지기에는 조금 부적절한 것 같아서 Controller로 옮겨주었다. 그리고 상수 이름도 더 직관적이도록 바꿔주었다.

기존에 랜덤 값을 생성하는 util 클래스로 RandomGenerator 라는 클래스를 만들어서 사용했었는데, 상수 값을 조정하는 김에 랜덤 값을 만드는 로직도 리팩토링 하기로 했다. 역시 제이의 피드백과 관련된 내용도 있었다.

그렇다 난 바보다... 물론 내가 짠 프로덕션 코드에서는 min값이 max 값보다 커서 예외가 발생하는 일은 없겠지만, 코드가 더 커지거나 다른 사람들과의 협업 과정에서 잘못된 값이 들어갈 가능성을 배제할 수 없다. 그래서 자잘한 수정을 한 컨트롤러 부분과는 다르게 랜덤 값 클래스는 뜯어고쳤다.

public class RandomNumberGenerator {
    private final Random random = new Random();

    private final int lowerBound;
    private final int upperBound;

    private RandomNumberGenerator(int lowerBound, int upperBound) {
        this.lowerBound = lowerBound;
        this.upperBound = upperBound;
    }

    public static RandomNumberGenerator fromBounds(int lowerBound, int upperBound) {
        if (lowerBound > upperBound) {
            return new RandomNumberGenerator(upperBound, lowerBound);
        }
        return new RandomNumberGenerator(lowerBound, upperBound);
    }

    public int generate() {
        if (lowerBound == upperBound) {
            return lowerBound;
        }
        return lowerBound + random.nextInt(upperBound - lowerBound);
    }
}

기존의 인자값인 min, max를 lowerBound, upperBound로 바꿔주었고, 해당 값을 generateNumber() 메소드 호출시에 인자로 받던 기존의 코드와는 다르게 필드로 만들어 주었다. 이 값들이 필드로 있으니 클래스 이름 자체도 RandomNumberGenerator로 바꾸어주었다.

그리고 정적 팩토리 메소드 패턴을 적용했다. 여기서는 확실히 정적 팩토리 메소드를 쓰는 이득이 더 크다고 생각했는데, RandomNumberGenerator 생성시에 인자로 들어가는 값이 어떤 값인지 한눈에 알아보기 위해서 생성하는 메소드의 이름을 fromBounds()로 해주기 위함이다. 정적 팩토리 메소드를 쓰는 김에 lowerBound > upperBound 일 때 둘을 바꿔주는 로직도 추가했다.

public class RacingController {
    private static final int RANDOM_LOWER_BOUND = 0;
    private static final int RANDOM_UPPER_BOUND = 10;
    ...
    
    private void race() {
        int currentTryCount = 0;
        OutputView.printStartMessage();
        while (tryCount.isNotSame(currentTryCount++)) {
            cars.moveAll(RandomNumberGenerator.fromBounds(RANDOM_LOWER_BOUND, RANDOM_UPPER_BOUND));
            OutputView.printStatus(cars.getCarsStatus());
        }
    }
    ...
}

그리고 컨트롤러에서 관리하는 게임 규칙인 랜덤 값 범위 상수를 가지고 RandomNumberGenerator를 만들어준 뒤 Cars.moveAll() 메소드에 인자로 전달해주었다. 이렇게 되면 Cars 에서는

public class Cars {
    ...
    
    public void moveAll(RandomNumberGenerator random) {
        for (Car car : cars) {
            car.goOrStop(random.generate());
        }
    }
}

와 같이 인자로 받은 RandomNumberGenerator에서 generate()만 호출해서 사용할 수 있다.

나와 연로그는 getter의 사용을 최대한 지양하기 위해 어차피 정보를 넘겨주는 목적이 콘솔 출력을 위함이므로 toString()을 오버라이딩해서 아예 출력을 위한 문자열로 바꾸는 작업을 해주었다. 그런데 제이의 "출력해주는 view가 console이 아니면?" 이라는 질문에 말문이 턱 막혔다. 사실 toString() 재정의로 view에 넘겨주면서도 이게 약간 편법 아닐까? 라는 생각을 하기는 했는데, 피드백을 보고 나니 확실히 콘솔 출력이 아닐 때(예를 들면 JSON으로 넘겨준다든가)를 위한 정리가 필요해보였다.

결국 Car와 Cars에서 데이터를 view로 보내주고, view에서 getter로 꺼내서 출력 형태를 만들어야 할 것 같았다. 하지만 애초에 getter를 지양했던 가장 큰 이유가 setter 뿐 아니라 getter로도 데이터의 불변성이 보장되지 않는다는 점 때문이었기 때문에 고민이 많았다.

public class Foo() {
    private final List<String> bar;
    
    public List<String> getBar() {
        return bar;
    }
}

이런 코드가 있을 때 bar가 final로 선언됐으니까 불변 객체가 아니냐고 생각할 수 있겠지만, 레퍼런스 참조를 하기 때문에 다른 클래스에서 getter로 꺼내온 뒤에 리스트에 값을 추가하거나 수정할 수 있다.

List<String> barReference = foo.getBar();
barReference.add("새로운 String"); // 에러 없이 잘 삽입된다

그래서 Car를 getter로 꺼내서 넘겨줘도 데이터의 불변성을 보장할 수 있도록 하는 방법을 생각하던 도중 Spring으로 프로젝트를 진행할 때 Entity를 다른 계층으로 전달할 때 Entity 자체를 넘겨주지 않고 DTO로 만들어서 넘겨주던 것이 생각났다.

어찌됐던 Car 객체의 정보가 오염되지 않도록 하면 되는 부분이기 때문에, 새롭게 DTO 클래스인 CarStatus 클래스를 만들어서 넘겨주기로 했다. 이 때, Name과 Position 모두 객체 검증 등을 할 필요가 없이 DTO를 넘겨받은 view에서 원시 값만 필요하므로 원시값으로 만들어주었다.

public class CarStatus {
    private final String name;
    private final int position;

    public CarStatus(Car car) {
        this.name = car.getName();
        this.position = car.getPosition();
    }

    public String getName() {
        return name;
    }

    public int getPosition() {
        return position;
    }
}

여기서 봉착한 또 하나의 문제는, 컨트롤러에서 view로 자동차들의 정보를 넘겨줄 때, DTO로 사용하지만 List로 묶어서 주기 때문에 final로 선언해주더라도 역시 불변이 아니라는 점이 있다. 그래서 찾아보던 도중 새로운 원소의 삽입, 기존 원소의 수정, 삭제를 방지하는 Collections.unmodifiableList 자료구조에 대해 알게 되어 해당 자료구조를 적용하였다.

public class Car {
    ...
    
    public List<CarStatus> getCarsStatus() {
        return cars.stream()
                .map(CarStatus::new)
                .collect(Collectors.toUnmodifiableList());
    }
    
    ...
}

이렇게 해서 view에서 CarStatus의 리스트를 순회하면서 getter로 DTO를 꺼내서 출력 가능한 형태로 만들어주었다. 두 번째 코드 리뷰에서 제이가 어떤 의견을 낼 지는 모르겠지만, 개인적으로는 불변성도 지키면서 코드도 깔끔하게 짠 것 같아서 만족스러웠다.

기타 몇가지 자잘한 수정을 남기고 오후 1시 반 경 데일리 미팅이 끝나고 두 번째 코드 리뷰 요청을 했다.

팀 브라운의 코딩 토론회??

데일리 미팅에서 조원들끼리 첫 미션을 하면서 잘 몰랐거나 궁금했던 부분에 대해 의견을 나누어보는 시간을 가졌으면 좋겠다는 이야기가 나왔다. 그래서 오후 4시! 시간이 맞는 팀원들이 다시 게더타운에 모여서 이런저런 이야기를 나누는 시간을 가졌다.


(게더타운에서 이야기를 나누고 있는 팀원들)

후니, 후디, 써머, 엘리, 로마, 어썸오, 루키가 참여해주었고, 코치님인 브라운도 잠깐 들러서 이런저런 이야기를 해 주었다. 브라운이 강조하신 부분은 이렇게 모여서 이야기하면서 메타인지능력을 기르는 것이 매우 중요하다는 것!

첫 번째 얘기 주제는 후디가 다른 사람들의 의견을 듣기 위해 제기한 Random 값 테스트를 어떻게 했느냐는 주제였다. 이번 자동차 경주 미션에서 랜덤 값을 가지고 자동차의 전진 / 정지를 결정하는 부분에 대한 단위 테스트를 이야기했다. 조원들의 주된 의견을 정리하자면

  • 랜덤 값을 테스트하는 것은 사실상 불가능하다.
  • Car에서 랜덤 값이 필요한 부분은 전진 / 정지를 결정하는 부분이다.
  • 따라서 해당 메소드가 (랜덤이든 아니든) 값을 인자로 받도록 하고 그 값에 따라 전진 / 정지를 결정하는 것이 맞다.
  • 전진 / 정지 메소드가 단위 테스트에 고정 값을 넣어서 통과하면 상관없다.

그 외에도 내가 사용했던 RandomNumberGenerator와 같은 객체를 인터페이스 구현 방식으로 만들어서 실제 프로덕션 코드에서는 랜덤 값을 만드는 구현체를 사용하고 테스트 코드에서는 고정 값을 만드는 구현체를 사용하는 방법도 나왔다. 이 방법은 후니가 제시해주었는데, 상속과 의존성 주입을 통해서 랜덤 값이 들어가는 로직을 테스트하고 결과적으로 단위 테스트 뿐 아니라 전체 테스트도 구현할 수 있다는 장점이 있었다. (내가 코드를 짠 방식은 로직 안에 이미 랜덤이 들어가 있으므로 어플리케이션 테스트는 돌아가기 힘들었다.)

이 부분은 같은 팀원인 후니가 블로그에 잘 정리한 글이 있어서 첨부를 한다.
(후니... 사람들이 보라고 쓴거니까 여기다도 올려도 되겠죠...?)
https://steadyjay.tistory.com/6

확실히 이 부분에서 느낀게, 다양한 방법을 찾아보고 익히는 크루들이 많다는 점이었다. 학교 프로젝트를 할때는 그래도 나름 깔끔하게 코드를 짜는 편이고 이런 저런 디자인 패턴도 잘 쓰는 편이었던 내가 우테코 세계에서는 모코코...?

그 다음으로 나온 주제가 테스트를 위한 오버로딩은 허용되어야 하는가? 라는 주제였다.

오버로딩(overloading): 같은 이름의 메소드를 중복하여 정의하는 것.
메소드 인자의 타입이나 개수를 다르게 하면 같은 이름의 메소드라도 여러 개 만들 수 있다.

"프로덕션 코드에서 사용하지 않는 메소드를 순전히 테스트 코드를 위해 사용하는 것이 허용되어야 하는가?" 가 핵심이었다.

예를 들어 대부분의 크루들이 Car 객체를 생성할 때 생성자에서 name을 인자로 받아서 초기화하고 position 값은 0으로 초기화해서 생성해주는데, 이 경우에 setter를 지양하다보니 setter로 position 값을 따로 바꿔줄 수 없다. 그래서 테스트를 진행할 때 0이 아닌 position이 값이 필요한 경우 생성자 오버로딩을 통해 position 값도 인자로 받는 생성자를 만들어서 테스트에만 사용하는 방식이다.

이 때 브라운도 게더타운에 함께 있어서 브라운이 한번 팀원들끼리 토론해보자며 정답은 없지만 다음 세 가지 중 본인의 의견을 가지고 토론해보자고 하였다.

  1. 실제 사용되는 코드가 아니라면 메소드 오버로딩도, 생성자 오버로딩도 안된다.
  2. 메소드 오버로딩은 안되지만 예외적으로 생성자 오버로딩은 된다.
  3. 둘 다 된다.

여기서 나는 1번의 의견이었고, 3번을 고른 크루들은 없었지만 2번을 고른 크루들은 몇 명 있었다. 토론 끝에 결론은 1번이 나오긴 했다. 내가 1번의 의견을 앞장서서 말한 편이었기 때문에 내 생각을 말하자면(앞에서 정답은 없다고 했지만)

우리가 TDD(테스트 주도 개발)을 배우고 최대한 그렇게 코드를 짜려고 하고 있지만
어쨌든 코드를 짜는 주 목적은 프로덕션 코드가 돌아가게 하는 것이다.
그런데 실제 프로덕션 코드에서 사용되지도 않는데 순전히 테스트만을 위해서 오버로딩을 하는 것은 주객이 전도된 느낌이다.

또한 테스트 코드에서 사용하기 위해서 오버로딩한 메소드나 생성자는 public으로 열어둘텐데,
이렇게 되면 프로덕션 코드에서 사용하지 않으려고 한 메소드인데도 불구하고
프로덕션 코드에서 의도와 다르게 오버로딩한 해당 메소드나 생성자를 사용할 가능성이 있다.
그렇다고 private로 오버로딩 하자니 테스트 코드에서 사용하려면 reflexion을 사용해야 해서
테스트를 위해 오버로딩하는 의미가 없다.

따라서, 애초에 테스트를 위해서 메소드나 생성자 오버로딩을 할 필요가 없는 코드를 설계하는 것이 맞다고 생각한다.

물론 내 의견이 절대 정답은 아니다. 앞서 랜덤 테스트를 위해 테스트 상황에서만 필요한 구현체를 만들어 준 것 처럼 오버로딩을 통해 테스트를 해 주는 것도 좋은 방법이 될 수 있다. 하지만 개인적인 생각으로는, 오용의 문제도 있고 코드 가독성을 해칠 것 같아서 반대하는 입장이다.

내가 제시한 주제도 있었는데, getter의 사용을 어디까지 허용해야 하는가? 라는 주제였다. 이 부분이 크루들마다 각기 다른 부분이었다. 나처럼 getter의 사용을 극도로 지양하는 사람도 있었던 반면, 어떤 크루는 view에 객체를 전달할때는 어차피 getter를 쓸 수 밖에 없다는 입장도 있었고(나도 리뷰를 받고 나서는 view에 던지는 용도로는 getter를 사용한다.) 따로 제약 없이 필요하다고 생각하면 getter로 꺼내 쓰는 크루도 있었다. 이 역시 정답이 있는 것은 아니고 개개인의 차이인 것 같은데, 역시 그래서 나는 getter를 최대한 안쓰고 싶다.

getter에 대한 이야기와 getter로 꺼낸 final 객체의 불변성을 말하던 와중에 내가 코드리뷰 이후 리팩토링에서 썼던 Collections.unmodifiableList 자료구조에 대한 이야기도 나왔다. 이 구조를 모르는 크루원들도 있었던 것 같은데 유용하게 쓰기를 바란다!

어느 정도 학술적인 얘기가 끝나고 나서부터는 서로의 이야기를 하면서 아이스 브레이킹 타임을 가졌다. 로마가 거의 메인 MC급의 진행 능력을 보여줘서, 팀원들끼리 어색함 없이 이야기를 나눌 수 있었던 것 같다. 어디 사는지부터 해서 서로의 취미, 자취하는 사람들의 요리 얘기, 맥북과 관련된 얘기까지 온갖 얘기가 다 오고갔다. 데일리 미팅의 제일 아쉬웠던 점이 길어야 30분 정도로 짧다는 점이었는데, 이렇게 충분한 시간동안 이야기를 나누니까 훨씬 친해지고 다양한 얘기도 오갈 수 있어서 좋았다.

그리고 무엇보다 좋았던건, 다양한 사람들의 코드 짜는 방식을 보고 좋은 점은 습득하면서 이야기를 나눌 수 있다는 점이었다. 학교 다니는 동안은 경험하기 힘들었던 대화였는데, 확실히 우테코의 최고의 장점이라고 볼 수 있는 것 같다.

이제 내일쯤이면 아마 2차 코드 리뷰가 올 텐데, PR이 merge 될 수도 있고 추가적인 피드백이 올 수도 있다. merge가 되고 나면 step2로 넘어가게 될 텐데, 강의로 들을 내용을 슬쩍 보니까 MVC 패턴 구현에 대한 이야기였다. MVC 패턴을 이미 적용한다고 적용한 코드지만, 패턴에 대해 확실하게 이해하지 못한 만큼, merge 이후 step2 리팩토링 시간이 기대된다.

profile
Jihoon Oh
Backend Developeer
이전 포스트

[우아한테크코스 4기] Level 1 4일차 회고

다음 포스트

[우아한테크코스 4기] Level 1 8일차 회고

0개의 댓글