💡 Disclaimer: I do not claim to be an expert. These blog posts portray my understanding of the subject matter. Please comment below or direct message me if you find any errors.

Introduction

In this blog post, we will attempt to solve the Trip service kata. The starting point for the code kata can be downloaded from here. The source code for the solution depicted here is available on GitHub (https://github.com/rsachira/trip-service-kata).

The objective of this code kata is as follows.

The objective is to test and refactor the legacy TripService class. The end result should be well-crafted code that expresses the domain. We do not need tests for collaborators (right now); we want to test only the logic contained in the TripService class.

The rules are,

• We can’t change any existing code if not covered by tests. We can’t afford to break any existing behavior.
• The only exception is if we need to change the code to add unit tests, but in this case, just automated refactorings (via IDE) are allowed.

This post uses C# .Net framework 4.8, MSTest for unit testing, and the visual studio IDE.

First look

The TripService class has a single function GetTripsByUser(...) that takes in a user. It checks if the given user is friends with the currently logged-in user. If friends, it returns the list of trips of the given user. If not friends, it returns an empty list. If a user is not currently logged in, it throws an error.

Problem

We have to solve several problems before we can write unit tests for GetTripsByUser(...). Our objective is to test the logic in GetTripsByUser(...). However, GetTripsByUser(...) has two dependencies.

• Singleton UserSession class
• and a static method from TripDAO

We cannot use dependency injection to mock these dependencies, because, according to the rules, we are not allowed to change the code before testing. We are only allowed to make automated refactoring supported by the IDE.

Mocking the dependencies

UserSession

One solution is to extract the User.User loggedUser = GetLoggedInUser(); into a separate method and override it with our mock value. We can do this by highlighting the line, right click -> Quick Actions and Refactoring -> Extract method. Name the method GetLoggedInUser() and make it a protected method.

TripDAO

We can do the same for TripDAO. Highlight tripList = TripDAO.FindTripsByUser(user);, right click -> Quick Actions and Refactoring -> Extract method. Name the method GetTripsOfUser(...) and make it a protected method.

Now we can create a subclass of TripService with mocked dependencies to test the GetTripsByUser(...) logic.

using Microsoft.VisualStudio.TestTools.UnitTesting;
using System;
using System.Collections.Generic;
using TripServiceKata.Exception;
using TripServiceKata.Trip;
using TripServiceKata.User;

namespace TripServiceKata.Test
{
    [TestClass]
    public class TripServiceKataTest
    {
        private static readonly User.User NOT_LOGGED_USER = null;

        private class TestTripService : TripService
        {
            private readonly User.User loggedInUser;

            public TestTripService(User.User loggedInUser)
            {
                this.loggedInUser = loggedInUser;
            }

            protected sealed override User.User GetLoggedInUser()
            {
                return loggedInUser;
            }

            protected sealed override List<Trip.Trip> GetTripsOfUser(User.User user)
            {
                return user.Trips();
            }
        }

        private static TestTripService CreateTripService(User.User loggedInUser)
        {
            return new TestTripService(loggedInUser);
        }
    }
}

Writing unit tests

User not logged-in

We will first test the logic when a user is not logged-in (i.e., loggedUser == null). We can test whether GetTripByUser(...) throws a UserNotLoggedInException as follows.

[TestMethod]
[ExpectedException(typeof(UserNotLoggedInException), "null logged in user inappropriately allowed")]
public void ShouldThrowExceptionWhenUserIsNotLoggedIn()
{
    var tripService = TripServiceKataTest.CreateTripService(TripServiceKataTest.NOT_LOGGED_USER);
    tripService.GetTripsByUser(new User.User());
}

User logged-in, but is not a friend

Next, let's test the logic when the logged-in user is not a friend of the given user. In this case, GetTripsByUser(...) should return an empty list.

[TestMethod]
public void ShouldReturnAnEmptyListIfNotFriends()
{
    var loggedInUser = new User.User();
    var givenUser = new User.User();
    Trip.Trip[] givenUsersTrips = { new Trip.Trip(), new Trip.Trip(), new Trip.Trip() };

    foreach (var trip in givenUsersTrips)
    {
        givenUser.AddTrip(trip);
    }

    List<Trip.Trip> trips = TripServiceKataTest.CreateTripService(loggedInUser).GetTripsByUser(givenUser);
    Assert.AreEqual(0, trips.Count);
}

User logged-in, and is a friend

Finally, let's test the normal operation of GetTripsByUser(...). That is when the logged-in user is a friend of the given user. Then, it should return the list of trips of the given user.

public void ShouldReturnTheTripsOfTheUser()
{
    var loggedInUser = new User.User();
    var givenUser = new User.User();
    Trip.Trip[] givenUsersTrips = { new Trip.Trip(), new Trip.Trip(), new Trip.Trip() };

    givenUser.AddFriend(loggedInUser);
    foreach(var trip in givenUsersTrips)
    {
        givenUser.AddTrip(trip);
    }

    List<Trip.Trip> trips = TripServiceKataTest.CreateTripService(loggedInUser).GetTripsByUser(givenUser);
    Assert.AreEqual(3, trips.Count);
}

Improving the TripService class

Now that we have tests for the TripService class, we can improve it without breaking the existing behavior. After each new change, we can run our unit tests to validate that the class is still performing as expected.

At a glance, we can see that GetTripsByUser(...) has multiple responsibilities.

1. It checks if there is a logged-in user.
2. It checks if the logged-in user is a friend of the given user.
3. It returns the trips of the given user.

Apart from that, it also has unnecessary nesting, which makes the code look more complicated.

Unnecessary nesting

The nesting from the if and else statements surrounding the loggedUser can be removed by inverting the condition as follows.

if (loggedUser == null) throw new UserNotLoggedInException();

Feature envy (code smell)

When a class gets data from another class to perform calculations or comparisons, it often means that the client class envies the other class. This is identified as a code smell known as Feature envy. This code smell can be seen in GetTripsByUser(...) in the logic used to identify whether the logged-in user is a friend of the given user.

Checking if one user is a friend of another user should ideally be a task of the User object.

namespace TripServiceKata.Test
{
  [TestClass]
  public class UserTest
  {
    [TestMethod]
    public void ShouldBeAbleToIdentifyWhetherFriends()
    {
      var user1 = new User.User();
      var user2 = new User.User();
      var user3 = new User.User();

      user1.AddFriend(user2);

      Assert.IsTrue(user1.isFriendsWith(user2));
      Assert.IsFalse(user1.isFriendsWith(user3));
    }
  }
}

public class User
{
    ...

    public bool isFriendsWith(User anotherUser)
    {
        return this.friends.Contains(anotherUser);
    }
    ... 
}

GetTripsByUser(...) can now be simplified.

Naming

The name GetTripsByUser(...) is not exactly correct. We are actually returning the trips of users who have the logged-in user as a friend. GetTripsOfFriend(...) would be a better name.

Now we are left with a simplified GetTripsOfFriend(...) as shown below.

public List<Trip> GetTripsOfFriend(User.User user)
{
  User.User loggedUser = GetLoggedInUser();    
  if (loggedUser == null) throw new UserNotLoggedInException();

  return user.isFriendsWith(loggedUser) ? GetTripsOfUser(user) : List<Trip>();
}

Decoupling dependencies

Now we can look into decoupling the two dependencies of TripService.

UserSession

The TripService class should not really depend on the UserSession. TripService, as the name suggests, is a service. On the other hand, UserSession should probably have to connect to the HTTP session and/or the MVC framework to obtain the user status. The logged-in user should probably be obtained from a controller or a client class, and should be passed into GetTripsOfFriends(...).

However, passing the logged-in user as just another User object could be a security risk. GetTripsOfFriends(...) can then be called with any two users, and we will be able to obtain the trips of other users with their friends. It will be better to differentiate the logged in user as a derived class of User.

namespace TripServiceKata.User
{
  public class CurrentUser : User
  {
  }
}

We will have to change the UserSession class to return a CurrentUser object rather than a User object.

using TripServiceKata.Exception;

namespace TripServiceKata.User
{
  public class UserSession
  {
    ...
    public CurrentUser GetLoggedUser()
    {
      throw new DependendClassCallDuringUnitTestException(
          "UserSession.GetLoggedUser() should not be called in an unit test");
    }
  }
}

Now, GetTripsOfFriend(...) can be made to accept the CurrentUser, and we can remove the protected method we added to mock the value.

public List<Trip> GetTripsOfFriend(User.CurrentUser loggedUser, User.User user)
{           
  if (loggedUser == null) throw new UserNotLoggedInException();

  return user.isFriendsWith(loggedUser) ? GetTripsOfUser(user) : new List<Trip>();
}

TripDAO

There are several ways to refactor static methods to dependency injection. However, surrounding the static call with a virtual method as we did previously is a decent way to inject mock values, and can be left as is.

Conclusion

Here we attempted to solve the trip service code kata. Code katas are more or less open-ended questions, and the solutions can vary with different ways of thinking and techniques used. Therefore, it is difficult to find a singular exact solution. However, I believe the attempt made in this post addresses the key objectives outlined in the trip service kata and constitutes a good enough solution. Trip service kata is a good exercise for incrementally improving legacy code. It also provides a practical example for spotting the feature envy code smell.

The complete code can be found on GitHub.