Looking for great developers Looking for great developers [email protected]
Facebook Linkedin Dribbble Behance Cluth

The most boring article about refactoring you'll ever read

26-04-2022 11 min read

The most boring article about refactoring you'll ever read

Today let's talk about legacy code. Programmers read code more often than write a new one. It is why code quality should be necessary. Developers' time is a cost. This time is much longer and more significant when the project's scope changes. There are too many things that can change it. Legal stuff. Users of the product. Clients with new ideas. Or some obvious one like bugs.

One of the most developer-friendly techniques to learn to write clean code is practice refactoring. Today I would like you to show how you can learn from one of them. For this occasion, I found some refactoring Kata. And decided to use TripServiceKata. It is an abstract project showing a lot of problems we see in the legacy codebase.

Refactoring Kata - developer-friendly techniques

The rules of Kata

The rules of this Kata are simple:

  • We can't change any existing code if tests do not cover it. We can't afford to break any current behavior,
  • The only exception is if we need to change the code to add unit tests, but in this case, automated refactoring (via IDE) is allowed.

The first rule shows as one of the wrong design decisions. Lack of the test. So this is my starting point. To test all of the code behavior without changing them. The only allowed changes are automated refactoring by IDE (and this exception of the rule safe us because without this code, it is untestable).

Refactoring in practical way 

See the code and talk about it

Let's see the code and talk a little about it. (Btw. I decided to use Kotlin, but all the related stuff should be easily applied in any object-oriented language).

class TripService_Original {

    fun getTripsByUser(user: User): List<Trip> {

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

        val loggedUser: User? = UserSession.instance.loggedUser

        var isFriend: Boolean = false

        if (loggedUser != null) {

            for (friend in user.friends) {

                if (friend == loggedUser) {

                    isFriend = true

                    break

                }

            }

            if (isFriend) {

                tripList = TripDAO.findTripsByUser(user)

            }

            return tripList

        } else {

            throw UserNotLoggedInException()

        }

    }

}

Spaghetti code

Spagetti enemy

This method returns a list of a user trip or throws an exception. But before any changes. I have to start by writing a unit test.

class TripServiceTest {

    private val tripService = TripService()

    private val user = User()

    @Test

    fun getTripTest() {

        val trips = tripService.getTripsByUser(user)

        assertEquals(null, trips)

    }

}

I was asserting null because I would like to hear what the code says. I don't know how it works. So let code speak the truth.
But immediately I faced first issue:

org.craftedsw.tripservicekata.exception.CollaboratorCallException: UserSession.getLoggedUser()

should not be called in an unit test It related to second line of the method:

val loggedUser: User? = UserSession.instance.loggedUser

UserSession is hard-wired into code. Without changing it, you cannot write any test. So one of the allowed refactors to inject UserSession into the constructor of the TripService. But still, you cannot test it well. One more allowed refactor of an extracted interface from UserSession. Automatically refactor this interface:

interface IUserSession {

    val loggedUser: User?

}

Introduce parameter from method to a constructor give this structure:

class TripService_Original(private val userSession: IUserSession) {

    fun getTripsByUser(user: User): List<Trip> {

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

        val loggedUser: User? = userSession.loggedUser

Let's tweak a test to use a mock version of UserSession:

class TripServiceTest {

    private val userSession = mockk<UserSession>()

    private val tripService = TripService_Original(userSession)

    private val user = User()

    @Test

    fun getEmptyListOfUserTripsTest() {

        every { userSession.loggedUser } returns user

        val trips = tripService.getTripsByUser(user)

        assertEquals(emptyList(), trips)

    }

}

How would you know what your tests are? One of the available tools is line test coverage. After running tests with range, IDE comes with green/red highlighting.
Now let's test the other from line 28. I am taking step by step to achieve a full green status bar with my test coverage.

@Test

fun getTripsByUser_ThrowsException() {

    every { userSession.loggedUser } returns null

    assertThrows<UserNotLoggedInException> { tripService.getTripsByUser(user) }

}

Ok. Coverage is better; let's write a test to friend logic which we can see in lines: 19-21. One more exception we occur when trying to reach line 25 is:

TripDAO should not be invoked on a unit test.

So you have to mock the TripDAO object. One more important thing is that we do not break any existing code. So with these changes of constructor with parameters. We should create a default one to use the old code version of its dependencies.

constructor() : this(UserSession.instance, TripDAO.Companion)

With that, you do not break any of the existing code. Because the default constructor is still there and passes the requested parameters as it is before changes. Here is a good moment for it because, as you see, there are no more hard-wired dependencies.

@Test

fun getTripsByUserWhenYouAreFriends() {

    val trip = Trip()

    friend.addFriend(user)

    friend.addTrip(trip)

    every { userSession.loggedUser } returns user

    every { TripDAO.findTripsByUser(friend) } returns friend.trips

    val trips = tripService.getTripsByUser(friend)

    assertEquals(listOf(trip), trips)

}

@Test

fun testDefaultConstructorException() {

    val tripServiceOriginal = TripService_Original()

    assertThrows<CollaboratorCallException> { tripServiceOriginal.getTripsByUser(user) }

}

With these two more tests, coverage reaches a hundred percent. All of them are green. We are happy here. Now I can focus on this code to ensure I don't break anything. Because the test still checks for correctness.
Focus on the loggedUser id. Code nesting is bad. Harder to read. Consider the turnaround condition to a guard if.

if (loggedUser == null) throw UserNotLoggedInException()

for (friend in user.friends) {

    if (friend == loggedUser) {

        isFriend = true

        break

    }

}

if (isFriend) {

    tripList = tripDao.findTripsByUser(user)

}

return tripList

Tests are still green.
Remove unnecessary flags and return a value when the user is a friend. Temporary variable for the list of trips is unnecessary too. After removing code is tiny:

fun getTripsByUser(user: User): List<Trip> {

    val loggedUser: User = userSession.loggedUser ?: throw UserNotLoggedInException()

    for (friend in user.friends) {

        if (friend == loggedUser) {

            return tripDao.findTripsByUser(user)

        }

    }

    return emptyList()

}

But there are still two indications. Code with one indication per method looks better, and it's easier to read. On the other hand, what check-in a user is a friend of a logged one can probably be done many times in code. So we can extract it and reuse it later. A Simple decorator of a user class can be used. Do not change any classes because you do not have to break any existing code. Start from the tests:

class UserFriendTest {

    @Test

    fun loggedUserIsAFriend() {

        val loggedUser = User()

        val friend = User()

        friend.addFriend(loggedUser)

        val userFriend = UserFriend(friend)

        val areFriends = userFriend.areFriends(loggedUser)

        assertEquals(true, areFriends)

    }

    @Test

    fun loggedUserIsNotAFriend() {

        val loggedUser = User()

        val friend = User()

        val userFriend = UserFriend(friend)

        val areFriends = userFriend.areFriends(loggedUser)

        assertEquals(false, areFriends)

    }

}

Tests are still passed. So now I can fix these two indications to ensure it's correct by test. And after this, copy a pasted code from getTripsByUser. This code checks up a list of friends containing a loggedUser. So it can be a one-liner.

fun areFriends(loggedUser: User): Boolean {

    return user.friends.contains(loggedUser)

}

Now use it on TripService. Tests are green.

class TripService(

    private val userSession: IUserSession,

    private val tripDao: TripDAO.Companion

) {

    constructor() : this(UserSession.instance, TripDAO.Companion)

    fun getTripsByUser(user: User): List<Trip> {

        val loggedUser: User = userSession.loggedUser ?: throw UserNotLoggedInException()

        return if (UserFriend(user).areFriends(loggedUser))

            tripDao.findTripsByUser(user)

        else emptyList()

    }

}

Summary of the code overview - compare the changes

Let's compare the changes in TripService.
End with guard Elvis Operator on the first line instead of nesting and throwing on if-else. Remove unnecessary flags and ifs. Move logic to other classes. Rename names to match what has to be done. Inject a third-dependent class instead of using it hard-wired in the method.

Before:

class TripService_Original {

    fun getTripsByUser(user: User): List<Trip> {

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

        val loggedUser: User? = UserSession.instance.loggedUser

        var isFriend: Boolean = false

        if (loggedUser != null) {

            for (friend in user.friends) {

                if (friend == loggedUser) {

                    isFriend = true

                    break

                }

            }

            if (isFriend) {

                tripList = TripDAO.findTripsByUser(user)

            }

            return tripList

        } else {

            throw UserNotLoggedInException()

        }

    }

}

After:

class TripService(

    private val userSession: IUserSession,

    private val tripDao: TripDAO.Companion

) {

    constructor() : this(UserSession.instance, TripDAO.Companion)

    fun getTripsByUser(user: User): List<Trip> {

        val loggedUser: User = userSession.loggedUser ?: throw UserNotLoggedInException()

        return if (UserFriend(user).areFriends(loggedUser))

            tripDao.findTripsByUser(user)

        else emptyList()

    }

}

We can help you with refactoring your code.

mDevelopers

Software development company

Go to clutch mdevelopers

We’ve been in the business for over 12 years and have delivered over 200 mobile and web projects. We know what it takes to be a reliable software partner.

Let our people take your business to the next level.

Sounds interesting? Give us some details and get your free estimation.

Cookies.

By using this website, you automatically accept that we use cookies. What for?

Understood