[리팩토링] 레거시 코드 리팩토링 예제

실습 1. Homeguard

기존에 작성된 Legacy Code인 홈가드의 메소드 중 하나인 getSensorMessage를 외부 동작을 바꾸지 말고 리팩토링 해라 (Depency Breaking)

getSensorMessage 메소드는 CentralUnit클래스에 있는 것이 맞을까?

public String getSensorMessage(Sensor sensor) { String message = "default"; if (!sensor.isTripped()) { if (sensor.getType().equals(Sensor.DOOR)) return sensor.getLocation() + " is closed"; else if (sensor.getType().equals(Sensor.WINDOW)) return sensor.getLocation() + " is sealed"; else if (sensor.getType().equals(Sensor.MOTION)) return sensor.getLocation() + " is motionless"; else if (sensor.getType().equals(Sensor.FIRE)) return sensor.getLocation() + " temperature is normal"; } else { if (sensor.getType().equals(Sensor.DOOR)) return sensor.getLocation() + " is open"; else if (sensor.getType().equals(Sensor.WINDOW)) return sensor.getLocation() + " is ajar"; else if (sensor.getType().equals(Sensor.MOTION)) return "Motion detected in " + sensor.getLocation(); else if (sensor.getType().equals(Sensor.FIRE)) return sensor.getLocation() + " is on FIRE!"; } return message; }

getSensorMessage메소드 내부는 대부분 static으로 사용된다. 그말은 CentralUnit의 변수나 속성들을 대부분 사용하지 않는다는 말이다.

오히려 sensor의 메소드를 대부분 사용하고 있다. 

객체지향에서 메소드는 자신의 클래스와 관련 있는 멤버를 사용해야한다.

따라서 Sensor 메소드로 가야한다.

하지만 가기전에, LegacyCode의 외부 동작을 바꾸면 안되므로 메소드를 추출해서 옮겨줘야 한다. (Dependency Breaking) 

public String getSensorMessage(Sensor sensor) { return getMessage(sensor); } public String getMessage(Sensor sensor) { String message = "default"; if (!sensor.isTripped()) { if (sensor.getType().equals(Sensor.DOOR)) return sensor.getLocation() + " is closed"; else if (sensor.getType().equals(Sensor.WINDOW)) return sensor.getLocation() + " is sealed"; else if (sensor.getType().equals(Sensor.MOTION)) return sensor.getLocation() + " is motionless"; else if (sensor.getType().equals(Sensor.FIRE)) return sensor.getLocation() + " temperature is normal"; } else { if (sensor.getType().equals(Sensor.DOOR)) return sensor.getLocation() + " is open"; else if (sensor.getType().equals(Sensor.WINDOW)) return sensor.getLocation() + " is ajar"; else if (sensor.getType().equals(Sensor.MOTION)) return "Motion detected in " + sensor.getLocation(); else if (sensor.getType().equals(Sensor.FIRE)) return sensor.getLocation() + " is on FIRE!"; } return message; }

리팩토링의 Move기능(Alt + Shift + V 단축키)을 이용하여 Sensor 메소드로 getMessage를 이동시킨다.

자 그러면 SensorTest 테스트 케이스를 만들고 getMessage를 테스트 해보자.

caller creator방식으로 테스트 하고싶은 메소드 or 클래스를 먼저 쓰고, 후에 선언하는것은 이제 익숙해지자.

Sensor의 소스코드를 다 읽어보지 않고 테스트케이스를 우선 작성 하였다.

일단 아무것도 모르지만, 실행해보자. Sensor의 Constructor 매개변수인 id, place, type이 있다.

뭔지 모르니 우선 아무값이나 임의로 넣어보자.

@Test public void testX() { Sensor sensor = new Sensor("id", "place", Sensor.DOOR); assertEquals("", sensor.getMessage()); }

place is closed라는 값을 요구한다.

이것을 통해서 place에 어떤 값을 넣으면 + is closed가 붙어서 출력됨을 알 수 있다.

테스트케이스는 이처럼 기존의 소스를 일일히 해석하지 않아도 output을 알기에 내부 구조를 일일히 분석해보지 않아도 

테스트를 가능하도록 해주는 장점이 있다. 

그래도 원활한 테스트의 진행을 위해서는 적어도 getMessage의 내부는 볼 필요가 있다.

public String getMessage(Sensor sensor) { String message = "default"; if (!sensor.isTripped()) { if (sensor.getType().equals(Sensor.DOOR)) return sensor.getLocation() + " is closed"; else if (sensor.getType().equals(Sensor.WINDOW)) return sensor.getLocation() + " is sealed"; else if (sensor.getType().equals(Sensor.MOTION)) return sensor.getLocation() + " is motionless"; else if (sensor.getType().equals(Sensor.FIRE)) return sensor.getLocation() + " temperature is normal"; } else { if (sensor.getType().equals(Sensor.DOOR)) return sensor.getLocation() + " is open"; else if (sensor.getType().equals(Sensor.WINDOW)) return sensor.getLocation() + " is ajar"; else if (sensor.getType().equals(Sensor.MOTION)) return "Motion detected in " + sensor.getLocation(); else if (sensor.getType().equals(Sensor.FIRE)) return sensor.getLocation() + " is on FIRE!"; } return message; }

Sensor메소드의 isTripped 여부에 따라서 다른 코드가 실행된다. 

어떤 메소드가 테스트할 가치가 있는 메소드인가? 에 대해서 오이사님께서 어제 잠깐 말씀하셨다.

if문 -> 테스트 가치 100%

for문 -> 테스트 가치가 높음

왜냐하면 이것들은 어떤 값이 나올지 예측하기가 어렵기 때문에 테스트를 거쳐야 하는 것이다.

반대로 getter와 setter들은 어떤 값이 나올지 예측할수 있으므로 굳이 테스트 할 가치가 없다.

따라서, 2갈래로 분기되는 getMessage는 Trip의 여부에 따라 2가지로 나눠서 테스트를 한다.

경보의 종류 4가지, 분기의 경우 2가지, 총 8개의 테스트 케이스가 나올 것이다.

테스트의 목적이 명확해졌으므로, 테스트 케이스의 이름도 새로 지을 수 있다.

먼저 Door 타입에서 Trip과 NonTrip을 구분하여 테스트해본다.

@Test public void doorIsNonTripped() { Sensor sensor = new Sensor("id", "door", Sensor.DOOR); assertEquals("door is closed", sensor.getMessage()); } @Test public void doorIsTripped() { Sensor sensor = new Sensor("id", "door", Sensor.DOOR); sensor.trip(); assertEquals("door is open", sensor.getMessage()); }

Trip일때와 NonTrip일때 요구되는 값이 다름을 알 수 있다.

Fire, Motion, Window도 이러한 방식으로 작동될 것이다.

하지만 여기에 경보의 종류가 추가된다면 어떻게 될까?

어느 부분을 수정해야 할까?

아마 getMessage를 수정해야 할 것이다. 

경보 타입도 추가시켜야 할 것이다.

경보의 종류가 추가되는데 한군데 이상을 수정해야된다.

오이사님께서는 변경을 할때는 딱 한군데만 수정을 해야 좋은 코드라고 말씀하셨다. (프로그램의 유연함)

변경이 잘 되도록 구조를 개선하려면 어떻게 해야 할까?

DoorSensor, FireSensor, MotionSensor, WindowSensor의 센서가 있다.. 여기에 SoundSensor가 추가된다고 해보자.

기존에 있던 4가지의 센서에서 공통점(중복)을 발견할 수 있는가?

그렇다. Sensor의 종류라는 것을 알 수 있다.

이것을 객체지향에서의 상속 개념과 연관지어보면

Sensor라는 부모클래스의 속성을 이어받고, 각 센서마다 다른 기능은 재정의하여 표현할 수 있다.

이 방법을 이용하면 SoundSensor를 새로 만든다고 가정하더라도, SoundSensor 클래스를 만든뒤 Sensor에서 상속받고

경보를 제공하는 부분만 수정하면 되는 것이다.

중요한것은 이것은 모두 SoundSensor에서 수정한다는 것이다. Sensor나 그 외의 클래스를 수정할 필요가 없다.

이것이 오이사님께서 말씀하신 변경시, 한 군데만 수정한다는 내용인 것 같다.

또한 프로덕션 코드의 외부 동작을 수정하지도 않는다.

그러면 기존의 Door테스트 케이스를 수정해보자.

@Test public void doorIsNonTripped() { DoorSensor doorSensor = new DoorSensor("id", "door"); assertEquals("door is closed", doorSensor.getMessage()); } @Test public void doorIsTripped() { DoorSensor doorSensor = new DoorSensor("id", "door"); doorSensor.trip(); assertEquals("door is open", doorSensor.getMessage()); }

그리고 Sensor를 상속받은 DoorSensor는 아래와 같다.

DoorSensor의 생성자에서 Sensor.Door부분을 제거하고 

내부에서 부모클래스의 생성자를 호출하여 매개변수를 Sensor.Door로 전달해주고 있기 때문에 외부 동작의 변경이 없다.

public class DoorSensor extends Sensor { public DoorSensor(String id, String location) { super(id, location, Sensor.DOOR); } public String getMessage() { if (!isTripped()) { return getLocation() + " is closed"; } else { return getLocation() + " is open"; } } }

마찬가지로 나머지 센서들도 상속받는 클래스를 만든다.

WindowSensor

public class WindowSensor extends Sensor { public WindowSensor(String id, String location) { super(id, location, Sensor.WINDOW); } public String getMessage() { if (!isTripped()) { return getLocation() + " is sealed"; } else { return getLocation() + " is ajar"; } } }

FireSensor

public class FireSensor extends Sensor { public FireSensor(String id, String location) { super(id, location, Sensor.FIRE); // TODO Auto-generated constructor stub } public String getMessage() { if (!isTripped()) { return getLocation() + " temperature is normal"; } else { return getLocation() + " is on FIRE!"; } } }

MotionSensor

public class MotionSensor extends Sensor { public MotionSensor(String id, String location) { super(id, location, Sensor.MOTION); } public String getMessage() { if (!isTripped()) { return getLocation() + " is motionless"; } else { return "Motion detected in " + getLocation(); } } }

현재까지의 테스트 코드

@Test public void doorIsNonTripped() { DoorSensor doorSensor = new DoorSensor("id", "door"); assertEquals("door is closed", doorSensor.getMessage()); } @Test public void doorIsTripped() { DoorSensor doorSensor = new DoorSensor("id", "door"); doorSensor.trip(); assertEquals("door is open", doorSensor.getMessage()); } @Test public void windowIsNonTripped() { WindowSensor windowSensor = new WindowSensor("id", "window"); assertEquals("window is sealed", windowSensor.getMessage()); } @Test public void windowIsTripped() { WindowSensor windowSensor = new WindowSensor("id", "window"); windowSensor.trip(); assertEquals("window is ajar", windowSensor.getMessage()); } @Test public void fireIsNonTripped() { FireSensor fireSensor = new FireSensor("id", "bathroom"); assertEquals("bathroom temperature is normal", fireSensor.getMessage()); } @Test public void fireIsTripped() { FireSensor fireSensor = new FireSensor("id", "bathroom"); fireSensor.trip(); assertEquals("bathroom is on FIRE!", fireSensor.getMessage()); } @Test public void motionIsNonTripped() { MotionSensor motionSensor = new MotionSensor("id", "staranger"); assertEquals("staranger is motionless", motionSensor.getMessage()); } @Test public void motionIsTripped() { MotionSensor motionSensor = new MotionSensor("id", "staranger"); motionSensor.trip(); assertEquals("Motion detected in staranger", motionSensor.getMessage()); }

Sensor의 getMessage는 오버라이딩의 대상이 되기 때문에 더이상 기능이 필요하지 않다.

public String getMessage() { String message = "default"; return message; }

따라서 getMessage를 추상 메소드로 바꿔준다.

자연스럽게 Sensor는 추상 클래스가 된다.

public abstract class Sensor { public static final String DOOR = "door"; public static final String MOTION = "motion"; public static final String WINDOW = "window"; public static final String FIRE = "fire"; private String id; private String location; private String type; private boolean tripped = false; public Sensor(String id, String location, String type) { this.id = id; this.location = location; this.type = type; } public String getId() { return id; } public String getType() { return type; } public String getLocation() { return location; } public void trip() { tripped = true; } public void reset() { tripped = false; } public boolean isTripped() { return tripped; } abstract public String getMessage(); }

경보 타입을 나타내는 static 변수도 의미가 없어졌으므로 삭제한다. (상속받는 클래스만으로 구분이 가능하기 때문에)

public static final String DOOR = "door"; public static final String MOTION = "motion"; public static final String WINDOW = "window"; public static final String FIRE = "fire";

따라서 Sensor의 Constructor에서 경보 타입이 제거되므로 바뀐다.

(이부분에서 이해가 안되는 것.. 리팩토링은 외부의 동작을 바꾸지 않고 내부를 개선하는 것인데 production코드의 파라미터를 바꾸는 것은 외부 동작을 바꾸는 것이 아닌가?)

public Sensor(String id, String location) { this.id = id; this.location = location; }

여기에 따른 각 서브클래스들의 생성자도 수정해준다.

자..이제 끝일까?

아니다. Sensor에서 상속받은 서브클래스들을 한번 보자

DoorSensor

public class DoorSensor extends Sensor { public DoorSensor(String id, String location) { super(id, location); } public String getMessage() { if (!isTripped()) { return getLocation() + " is closed"; } else { return getLocation() + " is open"; } } }

WindowSensor

public class WindowSensor extends Sensor { public WindowSensor(String id, String location) { super(id, location); } public String getMessage() { if (!isTripped()) { return getLocation() + " is sealed"; } else { return getLocation() + " is ajar"; } } }

리팩토링의 기준 중 하나는 '중복을 무자비하게 줄이는 것' 이다.

무언가 공통점이 있지 않은가?

그렇다. getMessage부분이 완전한 중복은 아니지만 로직이 거의 동일하다. 

그렇다면 getMessage 부분에서 중복을 추려내보자

아래와 같은 getNonTrippedMessage, getTrippedMessage로 구분할수 있다.

(이때 느낀점. 나는 처음에 DoorSensor에서 return getLocation() + " is closed"; 부분을 따로 빼낼때 메소드의 이름을 whenDoorClosed로 지으려 했다. 그러나 이렇게 지으면 이름상 Door에 종속된 메소드가 되버려서 다른 메소드에 중복을 제거하기가 어렵다. (WindowSensor에도 whenDoorClosed라고 해야 중복을 제거할 수 있는 상황) 그래서 중복을 추려내기 위해 만드는 메소드의 이름은 다른 중복 대상까지 포괄할 정도로 추상적이어야 한다는 것을 느꼈다.)

DoorSensor

public class DoorSensor extends Sensor { public DoorSensor(String id, String location) { super(id, location); } public String getMessage() { if (!isTripped()) { return getTrippedMessage(); } else { return getNonTrippedMessage(); } } public String getNonTrippedMessage() { return getLocation() + " is open"; } public String getTrippedMessage() { return getLocation() + " is closed"; } }

WindowSensor

public class WindowSensor extends Sensor { public WindowSensor(String id, String location) { super(id, location); } public String getMessage() { if (!isTripped()) { return getTrippedMessage(); } else { return getNonTrippedMessage(); } } protected String getNonTrippedMessage() { return getLocation() + " is ajar"; } protected String getTrippedMessage() { return getLocation() + " is sealed"; } }

이제 두 클래스의 getMessage가 완전히 동일해졌다.

그러면 두 서브클래스의 중복을 어떻게 제거할까?

답은 아까도 했던 상속을 이용하는 것이다. 

getMessage를 부모클래스인 Sensor 클래스로 올려버린다 (Pull-Up기능 이용, 반대로 부모-> 자식으로 메소드를 내릴때는 Push-Down 기능)

그리고  getNonTrippedMessage, getTrippedMessage 메소드들은 추상 메소드로 만들어서 각 서브클래스마다 다른 기능을 수행하도록 한다.

Sensor클래스

public String getMessage() { if (!isTripped()) { return getTrippedMessage(); } else { return getNonTrippedMessage(); } } abstract protected String getNonTrippedMessage(); abstract protected String getTrippedMessage();

DoorSensor클래스

public class DoorSensor extends Sensor { public DoorSensor(String id, String location) { super(id, location); } public String getNonTrippedMessage() { return getLocation() + " is open"; } public String getTrippedMessage() { return getLocation() + " is closed"; } }

소스 코드가 훨씬 간결해졌음을 알 수 있다.