refactor(all): 리팩토링 완료 #1

MoonJeWoong · 2022-12-07T09:59:09Z

다리건너기 게임 리팩토링 완료하였습니다.
이번 리팩토링에서 집중한 부분은 다음과 같습니다.

객체를 객체답게 사용하기 위해 노력한다.
Enum 을 단순한 상수가 아닌 클래스처럼 사용하기 위해 노력한다.
stream을 활용하여 단순 반복문의 사용을 줄인다.
꼬리재귀를 이용한 메서드 호출을 지양하고 반복문을 통해 작업을 수행한다.
view 단에서 판단하는 로직을 최대한 지양한다.

be-student · 2022-12-07T16:49:28Z

src/main/java/bridge/controller/Controller.java

+    public Controller(InputView inputView, OutputView outputView, BridgeNumberGenerator generator){
+        this.inputView = inputView;
+        this.outputView = outputView;
+        this.bridgeMaker = new BridgeMaker(generator);


application 쪽에서 new BridgeMaker(generator) 를 통해 생성하지 않으신 이유가 뭔가요?
그냥 바깥에서 통째로 주입해도 괜찮을 거 같아보여요

bridge와 관련된 것일수록 최대한 구조적으로 내부에 배치를 하고 싶었습니다. 어떤 방식이 더 적합한지는 아직도 잘 모르겠네요... 😭

be-student

잘 읽었습니다!

be-student · 2022-12-07T16:51:39Z

src/main/java/bridge/controller/Controller.java

+    }
+
+    private void playGame(){
+        GameCommand command = null;


null 대신 다른 것으로 초기화 하는 것도 좋을 거 같아요
commznd가 다른 것이 있다면 GameCommand.RETRY 같은 느낌으로요

이 부분은 저도 처음에 GameCommand.Retry로 구현을 했었습니다. 그런데 사용자가 입력하지 않은 초기 값이 Retry라는 것은 뭔가 제 직관에는 어긋나는 것처럼 느껴져서 null로 초기화를 했습니다. 하단에 반복문을 수행할때는 command가 Retry / Quit 두 가지 값만 가지고 수행되도록 했습니다!

null을 만약 쓴다면 Optional 로 감싸는 쪽이 무조건 맞다고 봐요
optional 로 감싸지 않는 null 같은 경우에는 좀 쎄게 얘기하자면 거의 대부분 잘못된 코드라고 생각해요
이 로직에서 문제가 없어도 추후에 문제가 생길 여지가 너무너무 많은 코드니까요

저도 이 의견에 동의합니다.
null로 인한 에러가 나는 경우가 생각보다 많아서 묶어주거나 다른 값으로 초기화해주는 것이 좋을 거 같아요.

be-student · 2022-12-07T16:52:12Z

src/main/java/bridge/controller/Controller.java

+
+    private void playGame(){
+        GameCommand command = null;
+        while(!bridgeGame.isCompleted() && !GameCommand.QUIT.equals(command)){


이 부분을 하나의 메서드로 묶는 것이 좋을 거 같아요

while문 내부에만 집중하다보니 정작 가장 쉽게 depth를 줄일 수 있는 법을 까먹었네요... 감사합니다!

be-student · 2022-12-07T16:54:19Z

src/main/java/bridge/controller/Controller.java

+    }
+
+    private void resetGame(GameCommand command){
+        if(command.equals(GameCommand.RETRY)){


enum equals 대신 ==으로 비교하는 것도 좋아보여요

오 이 부분은 생각하지 못했던 부분인데 확실히 NPE가 발생할 수 있는 코드네요... 감사합니다!

be-student · 2022-12-07T16:57:13Z

src/main/java/bridge/view/InputView.java

+    public GameCommand readGameCommand() {
+        System.out.println("게임을 다시 시도할지 여부를 입력해주세요. (재시도: R, 종료: Q)");
+        try{
+            return GameCommand.of(readLine().trim());


인자가 하나면 from이 좋을 거 같아요
그리고 그냥 of, from 보다는 문맥을 담아주시는 쪽이 좋아보여요

엇 from으로 작성한다고 생각했는데 정신이 딴데 팔려있었나봐요...
문맥을 담는다고 하면 어떤식으로 작성할 수 있을까요? from이라는 단어 자체로 인자로부터 객체를 생성한다는 의미로는 부족한걸까요?

정적 팩토리 메서드를 쓰는 이유가

생성자가 이름을 가질 수 있다

호출시마다 인스턴스를 매번 새로 만들지 않아도 된다

반환 타입의 하위 타입을 반환할 수 있다

이렇게가 커다란 장점이라고 생각하는데요
1번의 장점은 from 을 사용하는 것만으로는 충분히 살릴 수가 없다고 생각합니다
2번의 장점은 어차피 enum이기에 살릴 수 없다고 생각합니다
3번은 어차피 다 똑같은 타입만을 반환하기에 살릴 수 없다고 생각합니다

최소한 장점을 살리려면 fromCommand같은 (command는 안쪽 변수명입니다) 추가적인 설명이 있어야 한다고 생각합니다

개인적으로 가독성 부분에서 일반 생성자보다 정적팩토리 메서드를 사용하는 편이 더 좋다고 생각해서 사용했던 부분이었는데 앞으로는 좀 더 확실한 장점을 갖는 부분에 사용하도록 해봐야겠네요. 감사합니다!

현재 string 값을 받기 때문에 fromCommand 같이 문맥을 담는 것이 좋아 보이는 것은 동의합니다.
하지만 객체를 전달한다고 했을 때는 from만 사용해도 괜찮다고 생각됩니다.

be-student · 2022-12-07T16:57:50Z

src/main/java/bridge/view/InputView.java

+        try {
+            return Integer.parseInt(readLine().trim());
+        } catch (NumberFormatException error) {
+            System.out.println("[ERROR] 다리 길이는 숫자값으로만 입력되어야 합니다. 재입력 해주세요.");


입력이 9999999999999999999999 로 들어왔을 경우에 숫자값인데도 오류가 날 거 같은데 메시지랑 다른 느낌이 들 것 같아요

그렇네요... 숫자값 입력을 받을 때 항상 이런식으로 검증해서 받는 방식을 사용했는데 에러메세지 작성시 참고해야겠습니다. 감사합니다!

be-student · 2022-12-07T17:05:19Z

src/main/java/bridge/domain/Bridge.java

+    private final List<Move> bridge;
+
+    public Bridge(List<String> bridge){
+        validateBridgeSize(bridge);


이 메서드가 가장 먼저 호출되기에 validateBridgeSize 메서드를 가장 위로 옮겨야 될 것 같아요

be-student · 2022-12-07T17:05:39Z

src/main/java/bridge/domain/Bridge.java

+        this.bridge = bridge.stream().map(Move::of).collect(Collectors.toList());
+    }
+
+    public Bridge(){


빈 생성자를 만드신 이유가 있으신가요?
로직상 있으면 문제를 일으킬 것 같아보여서요

Bridge라는 클래스를 systemBridge와 userBridge를 만들때 모두 사용하기 때문입니다.
systemBridge를 생성할 때는 bridgeMaker를 통해 자동으로 생성된 list로 Bridge를 생성해야 하고,
userBridge를 생성할 때는 빈 ArrayList로 생성하여 사용자의 move 입력 시마다 값을 add해야 하기 때문입니다.

어떤 로직 부분에서 문제를 일으킬 수 있다고 생각하셨는지 구체적으로 말씀해주시면 감사하겠습니다!

저는 애시당초 사용자가 움직인 기록인 userBridge와, 생성된 systemBridge를 분리해야된다고 보는 입장입니다
systemBridge는 생성되는 시점에 딱 한 번만 초기화되는 완벽하게 불변인 객체로 만들 수 있다고 생각하는데요
userBridge는 그렇지 않죠

단순하게 코드 중복만을 해결하기 위해서 userBridge와 systemBridge를 통합하게 되다보니 systemBridge의 불변성을 유지할 수 없는 그런 부분을 보더라도 되게 좋지 않은 코드라고 생각합니다

be-student · 2022-12-07T17:07:11Z

src/main/java/bridge/domain/Bridge.java

+        bridge.add(move);
+    }
+
+    public Move getMove(int index){


getter를 두는 것은 객체스럽게 쓰는 느낌은 아닐 것 같아보여요

일급 컬렉션에서 속성변수로 가지는 컬렉션의 요소값들을 하나씩 조회해야 하는 기능에 사용된 메서드입니다.
이런 기능은 어떻게 구현하는 것이 객체답게 사용할 수 있는 방법일 지 잘 모르겠더라구요...
일급 컬렉션 내부의 컬렉션 값 요소들을 외부에서 순회해야하는 경우에는 getter사용이 불가피하다라고 생각이 좀 들었던 것 같습니다.

일단 외부에서 바로 get을 하는 것보다는 index와 item을 받아서 같은지 비교하는 메서드가 코드상에서 볼 수 있었는데요
이 방식으로 사용해도 바로 getter보다는 더 좋다고 생각합니다

be-student · 2022-12-07T17:09:51Z

src/main/java/bridge/domain/BridgeGame.java

+     * 재시작을 위해 필요한 메서드의 반환 타입(return type), 인자(parameter)는 자유롭게 추가하거나 변경할 수 있다.
+     */
+    public void retry() {
+        userBridge = new Bridge();


여기서 빈 생성자를 호출해서 사용하는 느낌이라고 생각되지만,
움직임에 해당하는 기록으로서의 기능과
다리로서의 기능은 차이가 있어보여서 분리하는 쪽이 좋아보여요

위에서 말씀드린 대로 다리의 자동생성 기능과 수동생성 기능을 모두 수행하기 위해서 그런 방식으로 구현했습니다. 말씀을 듣고 나니 정적 팩토리 메서드를 사용해 이름을 다르게 부여해서 명확하게 분리할 수 있으면 더 좋았을 것 같다는 생각이 드네요!

be-student · 2022-12-07T17:10:17Z

src/main/java/bridge/domain/BridgeGame.java

+        while(index < compareResult.size()){
+            bridgeMap.addLog(userBridge.getMove(index), compareResult.get(index));
+            index++;
+        }


이 부분을 bridgeMap 안쪽에서 하면 좋을 것 같아보여요

BridgeMap에 Bridge에 대한 의존성을 제거하려고 바깥에서 주입하려다 보니 이렇게 구현한 것 같네요... 이것 때문에 Bridge 클래스에서 getter 메서드를 사용했고요... 다시 생각해보니 BridgeMap은 Bridge 객체에 의존성을 가져도 되었을 것 같네요. 그러면 불필요한 getter 메서드도 지양하고 해당 작업도 bridgeMap 내부에서 수행할 수 있었을 것 같습니다.

의존성의 기준을 데이터의 변화로 잡아보는 것도 추천드립니다.

엇 영균님 혹시 데이터의 변화를 의존성의 기준으로 잡아보라고 한 것에 대한 좀 더 구체적인 설명이나 레퍼런스를 부탁드려도 될까요? 추상적으로 다가와서 한번에 이해하긴 힘든 것 같습니다...!

be-student

이거 어떻게 없애나요...

be-student

이거 어떻게 없애나요...

be-student

이거 어떻게 없애나요...

be-student

이거 어떻게 없애나요...

iyk2h

리뷰가 조금 늦은 점 죄송합니다.
은우님이 리뷰를 꼼꼼하게 해두셔서 리뷰가 많지는 않습니다...
코드와 리뷰를 보면서 많이 배우는 시간이었습니다.
리뷰 남기러 왔다가 공부하다가 가네요. 감사합니다. :)

iyk2h · 2022-12-08T14:31:15Z

src/main/java/bridge/controller/Controller.java

+    }
+
+    private void playGame(){
+        GameCommand command = null;


저도 이 의견에 동의합니다.
null로 인한 에러가 나는 경우가 생각보다 많아서 묶어주거나 다른 값으로 초기화해주는 것이 좋을 거 같아요.

iyk2h · 2022-12-08T15:10:16Z

src/main/java/bridge/domain/BridgeGame.java

+        while(index < compareResult.size()){
+            bridgeMap.addLog(userBridge.getMove(index), compareResult.get(index));
+            index++;
+        }


의존성의 기준을 데이터의 변화로 잡아보는 것도 추천드립니다.

iyk2h · 2022-12-08T15:12:43Z

src/main/java/bridge/domain/BridgeGame.java

+    }
+
+    public boolean isOver(){
+        return systemBridge.makeCompareResult(userBridge).contains(FAIL_SIGN);


사소한 것이지만, 한 줄에 점 하나만 찍는 게 좀 더 코드 관리하기에 쉽습니다.

iyk2h · 2022-12-08T15:16:00Z

src/main/java/bridge/domain/BridgeMap.java

+    }
+
+    @Override
+    public String toString(){


저도 두 분 덕분에 좋은 기회를 얻었네요. 감사합니다. :)

iyk2h · 2022-12-08T15:18:28Z

src/main/java/bridge/domain/GameCommand.java

+    RETRY("R"),
+    QUIT("Q");
+
+    private static final String GAME_COMMAND_ERROR = "[ERROR] 입력값은 R / Q 중 하나만 입력이 가능합니다. 재입력 해주세요.";


사소한 부분이지만 출력되는 부분에 R, Q 값은 GameCommand.RETRY.getValue() 를 사용해서 관리하는 게 좋아 보입니다.

아 비즈니스 로직 상에서 GameCommand에 대해서 내부 string 값을 사용해야 하는 경우가 생기지 않아서 getter를 삭제하다보니 그랬던 것 같네요. 확실히 출력부분은 이후 수정가능성을 생각했을 때도 고려해야 되니까 영균님 말씀이 맞는 것 같습니다.

iyk2h · 2022-12-08T15:27:17Z

src/main/java/bridge/view/InputView.java

+    public GameCommand readGameCommand() {
+        System.out.println("게임을 다시 시도할지 여부를 입력해주세요. (재시도: R, 종료: Q)");
+        try{
+            return GameCommand.of(readLine().trim());


현재 string 값을 받기 때문에 fromCommand 같이 문맥을 담는 것이 좋아 보이는 것은 동의합니다.
하지만 객체를 전달한다고 했을 때는 from만 사용해도 괜찮다고 생각됩니다.

refactor(all): 리팩토링 완료

77e05f5

be-student reviewed Dec 7, 2022

View reviewed changes

be-student suggested changes Dec 7, 2022

View reviewed changes

be-student reviewed Dec 7, 2022

View reviewed changes

iyk2h reviewed Dec 8, 2022

View reviewed changes

refactor(all): 리팩토링 완료 #1

Are you sure you want to change the base?

refactor(all): 리팩토링 완료 #1

Conversation

MoonJeWoong commented Dec 7, 2022

be-student Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

be-student left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

be-student left a comment • edited Loading

Choose a reason for hiding this comment

be-student left a comment • edited Loading

Choose a reason for hiding this comment

be-student left a comment • edited Loading

Choose a reason for hiding this comment

be-student left a comment • edited Loading

Choose a reason for hiding this comment

iyk2h left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

be-student Dec 7, 2022 •

edited

Loading

be-student left a comment •

edited

Loading

be-student left a comment •

edited

Loading

be-student left a comment •

edited

Loading

be-student left a comment •

edited

Loading