[220218] 페어 협업미션 리팩토링 피드백 (레벨1 - 자동차 경주 미션)

우아한테크코스에선, 같은 미션이어도 1단계-구현, 2단계-리팩토링으로 나누어진다.

지난 포스팅에서 1단계-구현 피드백 포스팅을 다뤄보았으니, 이번엔 2단계-리팩토링 피드백 포스팅을 다뤄보려한다.

 

1단계-구현 피드백은 여기서 볼 수 있다.

https://kth990303.tistory.com/262

[220211][JAVA] 페어 협업미션 리뷰어의 피드백_문자열 덧셈 계산기

 

2단계 - 자동차 경주 리팩터링 PR은 https://github.com/woowacourse/java-racingcar/pull/348 여기서 확인 가능하다.

이번 포스팅엔 중요한 피드백을 요약할 것이다.

---

정적 팩토리 메소드를 사용하기 적합한 상황일까?

우선 지난 1단계 merge하면서 남겨주신 미르의 피드백을 보자.

생성자가 public 형이고, 

생성자 내에 특정 name을 검사하는 검증 로직이 없기 때문에, 다른 곳에서 잘못된 name을 가진 Car 객체가 생성될 가능성이 존재했다.

 

여기서 나는 두가지 고민을 했다.

 

1. 생성자 내에 검증 로직을 추가한다.

2. 무분별한 생성자 사용을 막기 위해 private형으로 바꾸고, 정적 팩토리 메소드를 이용한다.

 

결론부터 말하자면, 나는 생성자 내에 검증 로직을 추가하는 방법을 택했다.

그 이유는 아래와 같다.

• 검증 로직은 상태를 변화시키는 로직이 아니기 때문에, 생성자 내에 검증 로직이 추가된다고 해서 생성자의 역할이 과하게 늘어나진 않는다.
• 정적 팩토리 메소드는, 생산성, 가독성을 해칠 수 있다. 일반적인 생성자로 생성되지 않기 때문에 다른 사람들이 Car 객체를 생성하려 할 때, Car 클래스의 코드를 일일이 확인하면서 생성 방법을 확인해야 한다.

물론, 정적 팩토리 메소드가 나쁘다는 건 절대 아니다.

정적 팩토리 메소드를 유용하게 사용할 수 있는 상황도 굉장히 많다.

하지만 여기선, 생성자 내에 검증 로직을 추가하는 걸로 충분히 대체할 수 있다고 판단하여 아래와 같이 수정하였다.

 public Car(final String name) {
    CarNameValidator.validate(name);
    this.name = name;
    this.position = 0;
}

---

Controller의 필요성

원래 내 코드 상황은 utils에서 domain에 종속돼있는 상황이었다.

input을 받으면, 그 input을 separator로 분리하는 것까지가 utils 역할인데,

나는 거기에 추가로, Car 객체를 생성하고 리턴하는 작업까지 거친 것이다.

 

이뿐만이 아니다.

매 라운드가 끝날 때마다, 개행문자를 출력해줘야 했는데,

물론 직접적인 ui 로직 이용이 아닌, ui 클래스의 메소드를 호출하여 간접적으로 이용했다고 하긴 하나,

비즈니스 로직에만 집중해야 하는 service에서 ui 메소드를 이용한 것은 분명 옳지 못한 코드이다.

ui 로직 수정을 위해 비즈니스 로직까지 수정해야 하는 상황이 발생할 수 있기 때문이다.

 

게다가, 내 코드는 원래 service만 존재했기 때문에,

service의 로직과 controller의 로직이 섞여있었다.

직접적인 비즈니스 로직은 service가 맞지만, service를 이용하여 매 라운드를 총괄하는 로직들은 controller로 분리하면 하나의 클래스에 안기는 책임을 덜어줄 수 있다.

 

 

따라서 View와 Service와 소통 가능한 Controller를 만드는 것이 필수적이라 생각하여

controller, service, utils, ui, domain 총 5개의 패키지로 나누게 되었다.

---

Stream.forEach()는 오버헤드가 발생한다.

stream()을 이용한 forEach()를 사용하는 것보다,

오히려 단순한 for-loop 문이 오버헤드는 훨씬 적다.

 

이펙티브 자바에서도 stream().forEach()문은 stream의 의도에 가장 많이 벗어난 연산이라고 한다.

애초에 forEach문에 로직을 넣는다는 것 자체가, forEach를 종료 연산이 아닌, 중간 연산의 역할로 수행하겠다는 의미이기 때문에 stream().forEach()를 사용한다면, 간단한 출력문 외에는 거의 사용하지 않는 것을 추천한다고 한다.

 

특히, primitive 타입에선 stream().forEach()보다 for-loop를 쓰는 것이 효과가 상대적으로 매우 좋다고 한다 :)

 

아무튼 난 그래서 그냥 for-loop 문으로 바꿨다.

 

참고 글: https://tecoble.techcourse.co.kr/post/2020-05-14-foreach-vs-forloop/

참고 글: https://homoefficio.github.io/2016/06/26/for-loop-%EB%A5%BC-Stream-forEach-%EB%A1%9C-%EB%B0%94%EA%BE%B8%EC%A7%80-%EB%A7%90%EC%95%84%EC%95%BC-%ED%95%A0-3%EA%B0%80%EC%A7%80-%EC%9D%B4%EC%9C%A0/

---

코드를 짜면 짤수록 공부해야 할 것들이 많다고 느낀다.

다른 분들은 인터페이스를 사용하거나 Comparator를 이용하는 등 다양한 기법들을 많이 사용하시던데, 나도 빨리 공부해서 더 많은 문법들을 익숙하게 활용할 수 있도록 해야겠다.

 

그리고 확실히 여러번 읽는 것보다, 직접 코딩하면서 피드백을 받는 것이 더 빠르고 효율적으로 성장하는 것 같다 :)