[kotlin] 레거시(legacy) 코드 리팩토링하기 3편 - 리팩토링 진행하기

1편 : 레거시(legacy) 코드 리팩토링하기 1편 - 테스트 코드를 작성해야하는 이유

2편 : 레거시(legacy) 코드 리팩토링하기 2편 - 리팩토링 진행하기

안녕하세요. 레거시를 리팩토링하는 3번째 시간입니다. 오늘은 이전 포스팅에서 만들어둔 라인커버리지 100%의 테스트코드에 대한 신뢰를 바탕으로 코드가 좋은 가독성을 가지는 방향으로 리펙토링을 실제로 진행해보고자 합니다.

환경

환경은 이전 시간과 마찬가지로 아래와 같습니다. 기본적인 테스트 프레임워크는 JUnit5를 사용하며 mockking을 위해서 Unit 테스트 라이브러리인 mockk를 추가로 사용합니다.

// JUnit5
testImplementation("org.springframework.boot:spring-boot-starter-test") {
    exclude(group = "org.junit.vintage", module = "junit-vintage-engine")
}

// mockk
testImplementation("io.mockk:mockk:1.12.3")

샘플 코드

오늘 리팩토링을 진행할 코드는 아래와 같습니다. 아래의 코드는 이전 포스팅에서 라인커버리지 100%를 달성해보았던 코드와 동일합니다. 로직을 간단히 설명드리면 아래와 같습니다.

1. User의 로그인 정보를 바탕으로 User 정보를 획득
2. 로그인 정보가 없다면 UserNotLoggedInException throw
3. 친구이기 판단하기 위해서 친구리스트를 가져와서 비교
4. 친구가 맞으면 tripList를 가져오고 반환
5. 친구가 아니라면 emptyList를 반환

@Throws(UserNotLoggedInException::class)
fun getTripByUser(user: User): List<Trip> {
    var tripList: List<Trip> = ArrayList()
    val loggedInUser: User = UserSession.getInstance().getLoggedUser()
    var isFriend = false
    return if (loggedUser != null) {
        for (friend in user.getFriends()) {
            if (friend == loggedUser) {
                isFriend = true
                break
            }
        }
        if (isFriend) {
            tripList = tripRepository.findTripsByUser(user)
        }
        tripList
    } else {
        throw UserNotLoggedInException()
    }
}

private fun getLoggedUser(): User? {
    return UserSession.getInstance().getLoggedUser()
}

100% coverage 테스트 코드

또한 100%의 테스트 커버리지를 가지는 코드는 아래와 같습니다. 보통은 테스트 메서드를 분리하여 각 케이스별인 3가지 경우에 대해서 각각 테스트 코드를 작성하겠지만 이번에는 편의상 하나의 테스트 코드로 대체했습니다.

@Test
fun getTripByUser() {
    val user2 = User(emptyList())
    val user = User(listOf(user2))

    every { sampleService["getLoggedUser"]() }.returnsMany(user2, user, null)

    sampleService.getTripByUser(user) // 정상 호출

    sampleService.getTripByUser(user2) // 친구가 없는 경우

    assertThrows(UserNotLoggedInException::class.java) {
        sampleService.getTripByUser(user2) // LoggedUser가 없는 경우
    }
}

테스트 코드에 대한 커버리지는 아래와 같습니다. 아래 처럼 테스트 커버리지를 사용하는 방법은 이전 포스팅에 자세히 기술하여 두었습니다. 궁금하신분들은 참고하세요.

return early pattern을 사용하자

그렇다면 이제 실제로 리팩토링을 진행해보도록 하겠습니다. 가장먼저 진행할 리팩토링은 else 구문을 없애고 validation을 가장 코드의 앞쪽으로 가져오는 일입니다. 함수의 전체적인 흐름을 어떻게 가져갈지는 주관적인 부분이 강한 부분입니다. 저는 개인적으로 else가 있다면 조건문 안에 조건문이 들어갈 수 있고, if / else 라면 if 구문이 생략되고 else 만 실행되면 코드의 중간이 생략되는 등 읽기에 불편함을 준다고 생각합니다.

결과적으로 되도록이면 else는 안쓰고 전체적으로 그리고 함수의 흐름을 return earlry를 이용하여 valdation - process - postProcess 3단계로 구현하는게 좋다고 생각합니다.

자세한 내용은 Return Early Pattern를 참고하시면 좋을것 같습니다.

이를 적용한 변경분은 아래와 같습니다. else로 throw 하던 부분을 ?:(엘비스)를 이용해서 바로 throw 하도록 하였습니다. Java라면 if (loggedUser == null) 일때 throw 구문을 앞에 추가해주면 됩니다.

@Throws(UserNotLoggedInException::class)
fun getTripByUser(user: User): List<Trip> {

    val loggedUser: User = getLoggedUser()
        ?: throw UserNotLoggedInException()

    var tripList: List<Trip> = ArrayList()
    var isFriend = false

    for (friend in user.getFriends()) {
        if (friend == loggedUser) {
            isFriend = true
            break
        }
    }
    if (isFriend) {
        tripList = tripRepository.findTripsByUser(user)
    }

    return tripList
}

작성된 테스트 코드를 실행하여 정상적으로 변경했는지 판단합니다. 테스트를 실행했을 때 아래처럼 정상적으로 모든 코드를 동일하게 커버하며 성공적으로 테스트가 성공한것을 확인할 수 있었으며 이걸로 리팩토링에 실수는 없었다고 판단할 수 있었습니다.

하나의 메서드에서 너무 많은 일을 한다면 private method로 분할하자

리팩토링의 기본적인 내용으로 Extract Method(메서드 추출)이라는 내용이 있습니다. 한 메서드에서 너무 다양한 일을 한다면 그 메서드에서 하는 일을 한 눈에 볼 수 있도록 메서드로 추출해서 분할하는 것입니다. 이렇게 함으로써 메서드가 하는 일을 명확히 할 수 있습니다. 자세한 내용은 Link를 참고해주세요.

우리의 예제에서는 유저의 친구가 맞는지 판단할 때 isFriend 라는 변수를 판단하는데 for 구문을 사용하고 있는데요. 이 부분을 별도의 메서드로 추출해서 사용하도록 하겠습니다. 결과는 아래와 같습니다.

@Throws(UserNotLoggedInException::class)
fun getTripByUser(user: User): List<Trip> {

    val loggedUser: User = getLoggedUser()
        ?: throw UserNotLoggedInException()

    var tripList: List<Trip> = ArrayList()

    if (isFriends(user, loggedUser)) {
        tripList = tripRepository.findTripsByUser(user)
    }

    return tripList
}

private fun isFriends(
    user: User,
    loggedUser: User
): Boolean {
    for (friend in user.getFriends()) {
        if (friend == loggedUser) {
            return true
        }
    }
    return false
}

작성된 테스트 코드를 실행하여 정상적으로 변경했는지 판단합니다. 테스트를 실행했을 때 아래처럼 정상적으로 모든 코드를 동일하게 커버하며 성공적으로 테스트가 성공한것을 확인할 수 있었으며 이걸로 리팩토링에 실수는 없었다고 판단할 수 있었습니다.

추가적인 리팩토링

마지막으로 진행할 리팩토링은 현재 반환을 위해서 선언하는 tripList 변수는 처음 초기화 없이 결과를 바로 반환해도 되는 함수입니다. 따라서 해당 변수를 바로 반환할 수 있도록 아래의 코드처럼 변경하도록 하겠습니다.

그렇게 만들어진 최종 코드는 아래와 같습니다. 아래의 코드는 최초의 코드에 비해서 많이 읽기 편해지고 수정이 필요할 때 어떤 부분을 수정하면 될지 훨씬 명확해 진것을 확인할 수 있습니다.

@Throws(UserNotLoggedInException::class)
fun getTripByUser(user: User): List<Trip> {

    val loggedUser: User = getLoggedUser()
        ?: throw UserNotLoggedInException()

    if (!isFriends(user, loggedUser)) {
        return emptyList()
    }

    return tripRepository.findTripsByUser(user)
}

이 또한 테스트 코드를 통해서 실수가 없었는지 판단할 수 있습니다.

마무리

오늘은 이렇게 안전하게 리팩토링을 진행해보는 시간을 가져보았습니다.

리팩토링을 실수하기 없이 하기위해서는 테스트 코드가 무조건적으로 필요합니다. 하지만 실무 환경에서는 그렇게 하지 못할 경우도 있습니다. 하지만 그럼에도 불구하고 만약 정말 중요한 부분을 리팩토링해야한다면 필요성을 충분히 어필하고 그 시간을 얻어내는것도 중요합니다.

감사합니다.

참조

[1] https://www.youtube.com/watch?v=LSqbXorkyfQ

[2] https://blog.timoxley.com/post/47041269194/avoid-else-return-early

[3] https://medium.com/swlh/return-early-pattern-3d18a41bba8

[4] http://wiki.c2.com/?ElseConsideredSmelly

[5] http://wiki.c2.com/?ArrowAntiPattern