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 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
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
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.