Let’s look at this piece of code:
public List<Trip> getTripsByUser(User user) throws UserNotLoggedInException {
List<Trip> tripList = new ArrayList<Trip>();
User loggedUser = UserSession.getInstance().getLoggedUser();
boolean isFriend = false;
if (loggedUser != null) {
for (User friend : user.getFriends()) {
if (friend.equals(loggedUser)) {
isFriend = true;
break;
}
}
if (isFriend) {
tripList = TripDAO.findTripsByUser(user);
}
return tripList;
} else {
throw new UserNotLoggedInException();
}
}
Horrendous, isn’t it? The code above has loads of problems, but before we change it, we need to have it covered by tests.
There are two challenges when unit testing the method above. They are:
User loggedUser = UserSession.getInstance().getLoggedUser(); // Line 3 tripList = TripDAO.findTripsByUser(user); // Line 13
As we know, unit tests should test just one class and not its dependencies. That means that we need to find a way to mock the Singleton and the static call. In general we do that injecting the dependencies, but we have a rule, remember?
We can’t change any existing code if not covered by tests. 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.
Besides that, many of the mocking frameworks are not be able to mock static methods anyway, so injecting the TripDAO would not solve the problem.
Overcoming the hard-dependencies problem
NOTE: In real life I would be writing tests first and making the change just when I needed but in order to keep the post short and focused I will not go step by step here .
First of all, let’s isolate the Singleton dependency on it’s own method. Let’s make it protected as well. But wait, this need to be done via automated “extract method” refactoring. Select just the following piece of code on TripService.java:
UserSession.getInstance().getLoggedUser()
Go to your IDE’s refactoring menu, choose extract method and give it a name. After this step, the code will look like that:
public class TripService {
public List<Trip> getTripsByUser(User user) throws UserNotLoggedInException {
...
User loggedUser = loggedUser();
...
}
protected User loggedUser() {
return UserSession.getInstance().getLoggedUser();
}
}
Doing the same thing for TripDAO.findTripsByUser(user), we will have:
public List<Trip> getTripsByUser(User user) throws UserNotLoggedInException {
...
User loggedUser = loggedUser();
...
if (isFriend) {
tripList = findTripsByUser(user);
}
...
}
protected List<Trip> findTripsByUser(User user) {
return TripDAO.findTripsByUser(user);
}
protected User loggedUser() {
return UserSession.getInstance().getLoggedUser();
}
In our test class, we can now extend the TripService class and override the protected methods we created, making them return whatever we need for our unit tests:
private TripService createTripService() {
return new TripService() {
@Override protected User loggedUser() {
return loggedUser;
}
@Override protected List<Trip> findTripsByUser(User user) {
return user.trips();
}
};
}
And this is it. Our TripService is now testable.
First we write all the tests we need to make sure the class/method is fully tested and all code branches are exercised. I use Eclipse’s eclEmma plugin for that and I strongly recommend it. If you are not using Java and/or Eclipse, try to use a code coverage tool specific to your language/IDE while writing tests for existing code. It helps a lot.
So here is the my final test class:
public class TripServiceTest {
private static final User UNUSED_USER = null;
private static final User NON_LOGGED_USER = null;
private User loggedUser = new User();
private User targetUser = new User();
private TripService tripService;
@Before
public void initialise() {
tripService = createTripService();
}
@Test(expected=UserNotLoggedInException.class) public void
shouldThrowExceptionWhenUserIsNotLoggedIn() throws Exception {
loggedUser = NON_LOGGED_USER;
tripService.getTripsByUser(UNUSED_USER);
}
@Test public void
shouldNotReturnTripsWhenLoggedUserIsNotAFriend() throws Exception {
List<Trip> trips = tripService.getTripsByUser(targetUser);
assertThat(trips.size(), is(equalTo(0)));
}
@Test public void
shouldReturnTripsWhenLoggedUserIsAFriend() throws Exception {
User john = anUser().friendsWith(loggedUser)
.withTrips(new Trip(), new Trip())
.build();
List<Trip> trips = tripService.getTripsByUser(john);
assertThat(trips, is(equalTo(john.trips())));
}
private TripService createTripService() {
return new TripService() {
@Override protected User loggedUser() {
return loggedUser;
}
@Override protected List<Trip> findTripsByUser(User user) {
return user.trips();
}
};
}
}
Are we done?
Of course not. We still need to refactor the TripService class.
public class TripService {
public List<Trip> getTripsByUser(User user) throws
UserNotLoggedInException {
List<Trip> tripList = new ArrayList<Trip>();
User loggedUser = loggedUser();
boolean isFriend = false;
if (loggedUser != null) {
for (User friend : user.getFriends()) {
if (friend.equals(loggedUser)) {
isFriend = true;
break;
}
}
if (isFriend) {
tripList = findTripsByUser(user);
}
return tripList;
} else {
throw new UserNotLoggedInException();
}
}
protected List<Trip> findTripsByUser(User user) {
return TripDAO.findTripsByUser(user);
}
protected User loggedUser() {
return UserSession.getInstance().getLoggedUser();
}
}
How many problems can you see? Take your time before reading the ones I found.. :-)
Refactoring
NOTE: When I’ve done it, I’ve done it step by step running the tests after every step. Here I’ll just summarise my decisions.
The first thing I noticed is that the tripList variable does not need to be created when the logged user is null, since an exception is thrown and nothing else happens. I’ve decided to invert the outer if and extract the guard clause.
public List<Trip> getTripsByUser(User user) throws UserNotLoggedInException {
User loggedUser = loggedUser();
validate(loggedUser);
List<Trip> tripList = new ArrayList<Trip>();
boolean isFriend = false;
for (User friend : user.getFriends()) {
if (friend.equals(loggedUser)) {
isFriend = true;
break;
}
}
if (isFriend) {
tripList = findTripsByUser(user);
}
return tripList;
}
private void validate(User loggedUser) throws UserNotLoggedInException {
if (loggedUser == null) throw new UserNotLoggedInException();
}
Feature Envy
When a class gets data from another class in order to do some calculation or comparison on that data, quite often it means that the client class envies the other class. This is called Feature Envy (code smell) and it is a very common occurrence in long methods and is everywhere in legacy code. In OO, data and the operations on that data should be on the same object.
So, looking at the code above, clearly the whole thing about determining if an user is friends with another doesn’t belong to the TripService class. Let’s move it to the User class. First the unit test:
@Test public void
shouldReturnTrueWhenUsersAreFriends() throws Exception {
User John = new User();
User Bob = new User();
John.addFriend(Bob);
assertTrue(John.isFriendsWith(Bob));
}
Now, let’s move the code to the User class. Here we can use the Java collections API a bit better and remove the whole for loop and the isFriend flag all together.
public class User {
...
private List<User> friends = new ArrayList<User>();
public void addFriend(User user) {
friends.add(user);
}
public boolean isFriendsWith(User friend) {
return friends.contains(friend);
}
...
}
After a few refactoring steps, here is the new code in the TripService
public List<Trip> getTripsByUser(User user) throws UserNotLoggedInException {
User loggedUser = loggedUser();
validate(loggedUser);
return (user.isFriendsWith(loggedUser))
? findTripsByUser(user)
: new ArrayList<Trip>();
}
Right. This is already much better but it is still not good enough.
Layers and dependencies
Some of you may still be annoyed by the protected methods we created in part one in order to isolate dependencies and test the class. Changes like that are meant to be temporary, that means, they are done so we can unit test the whole method. Once we have tests covering the method, we can start doing our refactoring and thinking about the dependencies we could inject.
Many times we would think that we should just inject the dependency into the class. That sounds obvious. TripService should receive an instance of UserSession. Really?
TripService is a service. That means, it dwells in the service layer. UserSession knows about logged users and sessions. It probably talks to the MVC framework and/or HttpSession, etc. Should the TripService be dependant on this class (even if it was an interface instead of being a Singleton)? Probably the whole check if the user is logged in should be done by the controller or whatever the client class may be. In order NOT to change that much (for now) I’ll make the TripService receive the logged user as a parameter and remove the dependency on the UserSession completely. I’ll need to do some minor changes and clean up in the tests as well.
Naming
No, unfortunately we are not done yet. What does this code do anyway? Return trips from a friend. Looking at the name of the method and parameters, or even the class name, there is no way to know that. The word “friend” is no where to be seen in the TripService’s public interface. We need to change that as well.
So here is the final code:
public class TripService {
public List<Trip> getFriendTrips(User loggedUser, User friend)
throws UserNotLoggedInException {
validate(loggedUser);
return (friend.isFriendsWith(loggedUser))
? findTripsForFriend(friend)
: new ArrayList<Trip>();
}
private void validate(User loggedUser) throws UserNotLoggedInException {
if (loggedUser == null) throw new UserNotLoggedInException();
}
protected List<Trip> findTripsForFriend(User friend) {
return TripDAO.findTripsByUser(friend);
}
}
Better, isn’t it? We still have the issue with the other protected method, with the TripDAO static call, etc. But I’ll leave this last bit for another post on how to remove dependencies on static methods. I’ll park my refactoring for now. We can’t refactoring the entire system in one day, right? We still need to deliver some features. :-)
Conclusion
This was just a toy example and may not even make sense. However, it represents many of the problems we find when working with legacy (existing) code. It’s amazing how many problems we can find in such a tiny piece of code. Now imagine all those classes and methods with hundreds, if not thousands of lines.
We need to keep refactoring our code mercilessly so we never get to a position where we don’t understand it any more and the whole business starts slowing down because we cannot adjust the software quick enough.
Refactoring is not just about extracting methods or making a few tweaks in the logic. We need to think about the dependencies, the responsibilities that each class and method should have, the architectural layers, the design of our application and also the names we give to every class, method, parameter and variable. We should try to have the business domain expressed in the code.
We should treat our code base as if it was a big garden. If we want it to be pleasant and maintainable, we need to be constantly looking after it .
If you want to give this code a go or find more details about the implementation, check: https://github.com/sandromancuso/testing_legacy_code
Reference: Testing legacy: Hard-wired dependencies (part 1 and 2) from our JCG partner Sandro Mancuso at the Crafted Software blog.
