개요

1주차 미션이 끝난 후 PR 리뷰를 요청하는 글을 올리고 난 뒤,
내 PR에는 무려 158개라는 놀라운 숫자의 리뷰가 오고 갔다.🫢
그 만큼 내 코드에 개선할 점이 많다는 뜻이니까..!
어떤 피드백이 있었고 나는 어떻게 피드백을 반영했는지 정리해보려고 한다!

📬 공통 피드백

우선 1주차 미션 종료 후 우테코로부터 행운의 메일!...이 아니라, 2주차 가이드와 함께 공통 피드백 메일을 받게되었다.😁
거기서 내가 적용해야할 점들을 몇 개 정리해보았다!

1. 요구사항 준수

시험에서도 항상 문제를 잘 읽어봐야하는 것처럼 요구사항을 모두 꼼꼼히 읽고, 중간에도 읽고, 마지막에도 읽어야겠다.

+) 라고 말한지 3시간 만에, 아니나다를까 매우 치명적인 실수를 발견했다.🫠

이전에 적용해놓았던 구글 컨벤션 스타일을 따로 수정하지 않고 사용중이었는데, 구글 컨벤션 스타일은 들여쓰기 지속이 +4였던 것이다.
[IntelliJ] 코드 스타일을 설정해보자 (feat.우테코) 작성자 분이 아니었다면 최종 미션까지 모르고 지나쳤을 부분이다... 정말 압도적 감사🙏🙏🙏

2. 의미있는 커밋 메시지

1주차 내 커밋 메시지는 의미있었는가?

이 기능, 저 기능 함께 구현하다 한꺼번에 커밋을 올리느라 추상적이게 적은 경우가 많은 것 같다는 생각이 든다..!
메시지에서 작업한 내용에 대한 이해가 가능하도록! 작성해보자!

3. git을 통해 관리할 자원인가?

사실 git 초보자로서 .class 파일이나 .idea 폴더 등은 git에 올릴 필요가 없다는 걸 생각하지 못했다!

.gitignore 파일이 그런 역할을 해주고 있을 거라고 어렴풋이 생각만 했었던 나.
'이러겠거니...'하고 넘어가는 것 멈춰✋✋
이 기회에 .gitignore 파일도 좀 살펴보고 git 레포지토리에 올라가있는 파일과 대조도 해봐야겠다!

정리해보았다! : https://velog.io/@dlguswl936/공부한-것-.gitignore을-왜-써야할까

4. 이름을 통해 의도를 드러낸다

개인적으로 1주차를 진행하며 네이밍에 대해서 느낀 부족함은

1. 에러 메시지, 리터럴 등의 상수화 네이밍을 서술형의 문장이더라도 좀 더 직관적으로
2. 객체와 메소드명에서의 중복을 줄여야함을 느꼈다!

5. 공백 라인을 의미 있게 사용한다

테스트 코드에서의 공백 필요에 대한 리뷰를 받아서 이 피드백이 더 실감됐다😂

📌 String.matches는 왜 성능에 안 좋을까?

리뷰를 달던 중 이런 코멘트를 발견했다.

나도 따로 Pattern 객체 없이 String.matches를 사용하는 편이었는데, 한 쪽에서는 궁금증이 있었왔다.

'Pattern 객체로 compile해 사용하면 더 코드가 늘어나는데 같은 기능이라면 굳이 왜 Pattern 객체를 사용하는 거지?'

하지만 위의 코멘트를 보면서 성능 이슈 가 그 이유라는 걸 알 수 있었는데,
구체적으로 어떤 원리로 String.matches가 Pattern 객체로 compile해 사용하는 것보다 성능 면에서 안 좋은 걸까?

이에 대해서 찾아본 내용과 그렇다면 어떤 걸 사용해야하는지 정리해보았다!

[공부한 것] String.matches는 왜 성능에 안 좋을까?

📌 생성자 주입 방식 컨트롤러에 반영

이번주 내 PR의 최고 인기 스타!💃 바로바로 이 부분!
마지막쯤에 고민하다 일단 아래처럼 구현해서 올렸는데,

와우. 가장 코멘트가 많이 달린 문제의(?) 부분이 되어버렸다!😂

저 BaseballGame을 어디서 생성해서 들어와야하나 생각이 많았는데, 생각보다 쉬운 문제였다.
컨트롤러 전반에서 BaseballGame을 많이 쓰는 것도 함께 고려해,
컨트롤러 바깥에서 생성해서 생성자 주입을 해주면 되는 문제였다!

잠깐, 생성자 주입, 의존성 주입이란?

이 코드 리뷰에서 자주 나오는 단어인데, 정확한 개념을 모르고 대충 이런 방식일 거라고 추측하고 있었다.
이거겠거니... 단속반 출동🚫🚫

정리해본 포스팅! - [공부한 것] 의존성 주입? 생성자 주입? 필드 초기화?

📌 View의 역할 줄이기

OutputView에는 게임 결과를 체크하는 부분이 포함되어 있었고,
InputView에는 validate 호출이 여러번에 나눠 들어가있었다.

객체가 해당되는 역할만 수행하는지 점검을 더 면밀하게 해봐야겠다는 다짐!

📌 확장성 vs 오버 엔지니어링

BaseballGame 객체의 상위 객체인 Game 인터페이스를 만드는 것과 관련한 코멘트를 받았다.

이 경우, 상위 객체인 Game 인터페이스를 만들었을 때의 장단점이 있다고 생각이 들었다.
아직 결론을 내리지 못했지만 일단 정리해놓아보려고 한다!

추상화를 했을 때의 장점

1. 이 객체는 컨트롤러가 의존성을 가지는 객체로 구체적인 구현 클래스가 아닌 고수준 모듈인 인터페이스를 만들어서 의존성을 가지면 DIP 의존 역전 원칙에 훨씬 부합하게 된다.
2. 확장성을 고려해 다른 종류의(?) 게임을 만들 수 있다.

추상화를 했을 때의 단점

1. '다른 게임을 만든다'가 현재 프로그램에서는 일어나지 않을 일이기 때문에 오버 엔지니어링일 수 있다.
2. 클래스의 갯수가 늘어나고 복잡성이 높아진다.

📌 간단한 중복 확인 시, 스트림 대신 set!

작은 규모에서는 성능의 차이가 크지 않다고 하나 불필요한 스트림 사용은 경계해야할 필요가 항상 있다고 생각한다!
역시나 그런 코멘트가 있었고, 중복 요소 확인을 스트림에서 set을 이용하는 방향으로 리팩토링 해보았다!

위에서 아래로 바뀜!

📌 컨트롤러가 중개 이상의 역할을?

컨트롤러가 view - model을 중개해주는 역할 이상을 했는가? 🙆‍♀️

다시 보니, 랜덤 숫자를 포장해서 Balls로 반환하는 역할까지 한 것이 '랜덤 숫자 생성'의 책임까지 했다고 볼 수 있을 것 같다! 아래처럼 리팩토링!

+) 다시보니 NumberGenerator가 여기서 생성되면 안될 것 같다... 이것도 반영하기!

📌 에러 메시지, 숫자, 정규식 등을 상수 객체로 빼야할까?

그동안 나는 에러 메시지나 숫자, 정규식 등을 상수화해서 그걸 사용하는 클래스의 내부에 위치하도록 했었다. 해당 상수가 사용되는 곳과 가까워야 가독성이 좋다고 생각했기 때문인데, 여기에 대한 코멘트를 받으면서 다시 생각해보게 되었다.

그러면서 내가 상수화하는 경우들을 적어보고,
각 경우별로 상수를 모은 객체로 분리하는 것과 사용되는 객체 내에 위치 시키는 것 중
어떤 게 적절한지 개인적으로 정리해보았다!

1. int UNDER_RANGE = 1;와 같은 숫자

: 해당 객체에 특수하게 쓰이는 리터럴이라면 남겨야하고,
: 여러 클래스에서 공통으로 쓰이는 리터럴이라면 분리 가능!

2. ERROR 메시지

: 상수명과 코드로 추측할 수 있을 것 같아 분리 가능!

3. 입, 출력시 표출되는 문구

: printf나 String.join 등을 사용해 결합해 쓰는 경우가 있기 때문에 남기는 것이 좋을 것 같다!

4. 검증에 필요한 정규식

: 보통 구분자, 숫자, 문자에 대한 정규식을 많이 쓰는데 이것들이 쓰이는 곳을 생각해보면 각 객체의 validate 부분으로, 공통적으로 쓰이게 된다면 분리하는 것이 좋을 것 같다!

5. "", " "와 같이 출력에 필요한 문자

: 이 문자들은 상수명으로 용도를 추측하기 쉽고 공통적인 의미므로 분리 가능!

참고: 상수 클래스의 private 생성자
상수만 모아 만든 클래스에는 이렇게 private 생성자가 있는 것을 여러 코드에서 발견할 수 있었는데, 이게 왜 필요한건지 궁금했다!

검색해보니 일반적으로 상수만을 모아놓은 클래스에서 사용되는 관용적인 패턴이라고 한다!
이유는,
1. 상수를 정의하고 다른 클래스에서 static으로 불러와 사용하기 위한 것이기 때문에 객체를 만들 필요가 없다.
2. 따라서 이 private 생성자를 통해 이 상수 모음 클래스가 객체로 생성되는 것(인스턴스화)을 막을 수 있다.
3. 더불어, 이 클래스가 상수 정의를 위한 거지, 객체 생성을 위한 게 아니라는 의도가 명시되는 효과도 있다!

+) 무분별한 상수화를 경계하기 위해 참고하면 좋은 포스트
https://pparksean.tistory.com/118

📌 정적 팩토리 메소드를 만들어서 객체를 생성한 이유가 뭐니?

그동안 객체를 생성할 때 from, of 등의 정적 팩토리 메소드를 사용하는 방법을 썼다.
그렇게 한 이유는? 캡슐화하면 좋으니까! 이 생각만 갖고 있었던 것 같다.
하지만 이러한 리뷰를 보고나니, 캡슐화라는 이유만 있어도 되는 걸까? 하는 생각이 들었고 무지성 사용을 막기위해 자세히 알아보았다!

정리해본 포스팅 : [공부한 것] 정적 팩토리 메소드 사용하는 이유가 확실하니?

📌 생성자에서 검증하지 말라고?

그동안 당연하게 생성자에 validate 넣어왔는데 이게 무슨 얘긴가 말인가!😳

왜 생성자에 코드를 넣으면 안되는 거지?

찾아본 결과 정확히는,
생성자는 코드가 없어야하고 오직 할당문만 포함해야 한다. ,생성자는 객체를 인스턴스화하는 작업만 해야한다.
인데 그 이유로는,

1. 생성자에 코드가 있으면, 객체를 생성할 때마다 전달된 인자로 바로 그 코드를 실행해 처리해야한다. 그렇게 하지만고 실제 사용될 시점까지 미뤄라.

의문) 근데 부생성자로 검증 메소드를 옮겨도 마찬가지 아닌가? 부생성자가 객체를 생성하도록 하는 건데, 
거기서 검증을 한다는 건, 객체를 생성할 때마다 검증 코드를 실행하는 건 마찬가지인데?

2. 따라서, 생성자에 코드가 없을 경우 성능 최적화가 더 쉽기 때문에 코드의 실행 속도가 빨라진다.
3. 생성자 안에서 어떤 일을 처리하고 있으면 나중에 리팩토링하기가 어렵다.

그럼 검증을 어디서?

일단 떠오르는 건 부생성자에서 하는 것인데,
한 번 다음주 코드에 적용해보고 이야기를 나눠보고자 한다!

참고한 글 : 1.3 생성자에 코드를 넣지 마세요. #5

📌 스트림으로 다 해결하려고 하지 말자!

아래의 리뷰를 보고 '아차'싶었다. 과도한 스트림 사용을 경계한다고 했으나 부족했나보다..!
한 스트림으로 하려고 하지말고, 기능이 분리되어있다면 스트림을 나와서 처리하자!

📌 boolean, Predicate 메소드 부정어 네이밍 지양!

참고 포스팅에서 발췌해 온 것으로 정리해보려고 한다!

부정적인 메소드 이름을 갖는 문제는 긍정적인 사용 사례에 대해 동일한 메소드를 재사용하려고 할 때 이해하기가 더 어렵다는 것입니다.
(중략) ! 를 사용하여 메소드 이름을 구성하는 것이 항상 더 나은 방법입니다

📌 리스트 레퍼런스를 그대로 반환할 때의 문제점?

view에 전달하는 용도 등으로 List를 getter로 반환하는 경우가 있었는데 이런 리뷰를 받게 되었다!

불변 리스트 등에 대해서도 들어보기만 했는데 이참에 사용 방법을 찾아보고 정리해보려고 한다!

정리해본 포스팅 : [공부한 것] 리스트 레퍼런스를 그대로 반환할 때의 문제점?

📌 결과가 너무 커졌을 때 오버플로우 어떻게 할래?

반복 횟수가 정해져있기 때문에 오버 플로우를 생각하지 않아봤는데 고민해볼 지점 같다!
일단 떠오르는 방법은,
1. long으로 받아서 최댓값 체크 후에 int로 변환하는 방법

    public int getCount(TryResult inpuTryResult) {
    long resultCount = tryResults.stream()
            .filter(tryResult -> tryResult.equals(inpuTryResult))
            .count();

    if (resultCount > Integer.MAX_VALUE) {
        // 처리할 작업을 수행
                throw new ArithmeticException("count가 interger의 범위를 초과했습다.");
    }
    return (int) resultCount;
}

정도가 있는데, 이렇게 되면 이렇게 int로 연산 결과를 받는 모든 곳에 이 부분을 넣어야하는 것인가? 하는 의문이 든다.
interger의 최댓값이 대략 20억, long은 90억 정도라 대부분의 상황에서 발생하지 않겠지만 말이다...!

📌 StringBuilder이 정말 성능이 더 좋을까?

결과에서 문자열을 합쳐야하는 경우에 나는 성능에 좋다고 하니까~ StringBuilder를 주로 썼다.
그런데 과연 정말 그럴까 싶어 찾아보고 정리해보았다!

정리해본 포스팅: [공부한 것] String보다 StringBuilder를 써야할까?

📌 메시지의 동적 작성

나는 보통 입력 문구 등을 작성할 때 요구 사항에서 제시한 문장을 그대로 넣는 편이었다.

그런데 PR 리뷰를 하면서 다른 분의 코드에서 이 부분을 이렇게 작성한 것을 발견했다.

위의 코드는 재시도 입력 문자를 view에 고정 시켜놓는 것이 아니라,
입력 문자의 책임을 완벽하게 재시도 객체로 이관한 방식이다! 이로써,

• 추후에 해당 기능에 대한 기대 입력 문자가 달라지더라도
• 해당 객체에서 수정할 수 있어서 유지 보수가 그 객체 안에서만 일어나게 제한할 수 있다는 장점이 있게된다!

감명을 받아 남긴 코멘트...😆

번외) 예외 메시지도 동적으로?!

"[5,4,3,2]를 입력하셨습니다. 숫자는 3자리 이하로 입력해야합니다" 이런 식으로 입력값을 반영해서 예외메시지를 내는 경우도 보았다. 끝이 없는 동적 구현의 세계!

.formatted()!

formatted 도 printf와 같은 기능같은데 다음에 써봐야겠다! 킵!

👏회고

으아아아 좋은데 고통스러운데 짜릿하다.. 어쩌면 좋을까..

내 답글을 제외하면 90 여개 정도의 코멘트가 달린 것 같고, 어찌어찌 다 정리해보았다...!
리뷰를 받는 것도 중요하지만 그걸통해 더 찾아보고, 내 걸로 만들어 정리하는 것도 중요한 것 같다!
(물론 리뷰를 읽어보고 반영하고 넘어가는 것에 비하면 훠어어어어얼씬 시간이 많이 걸린다...)

그동안 대충 이렇게겠지 하고 무지성으로 사용했던 것들
그리고 모르겠으니까 일단 나쁘지 않아보이는 내 방식으로 고착화했던 것들
이런 것들을 하나 하나 물어보시고 까발려지는(?) 기분이다😂

하지만 평소라면 다음에 알아보지 뭐~ 아니면 문제 생기면 그 때 알아보지~ 했던 것들을
1. 답변을 해야한다(?)라는 압박감이 생겨서,
2. 내 나름의 그렇게 작성한 이유를 정리하고 찾아보면서,
3. 그리고 잘못된 부분은 과감히 제안해주신 방법으로 바꿔끼워 보면서

이런 저런 이유들로 혼자 했다면 나중에나 알게되고 고쳤을,
또는 영영 알지 못하고 넘어갔을 것들에 있던 그 지식의 틈을 강제로(?) 메꾸는 기분이다!