Monday, 18 July 2011

Testing legacy: Hard-wired dependencies (part 2)

In part one, I showed how to unit test a method that uses a Singleton and makes static calls. So now, let's have a look at common code problems we find in legacy code, using the same example:


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



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:


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.


After a few refactoring steps, here is the new code in the TripService


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:


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 haven't read it yet, check the part one of this post. If you want to give this code a go or find more details about the implementation, check: https://github.com/sandromancuso/testing_legacy_code

4 comments:

Rob Hillier said...

Is there any reason in the final code you validate the first argument (loggedUser), but not the second (friend)?

Michael said...

@Rob:
The method "getFriendTrips" should return all the trips of the friends of the currentUser, so it doesn't matter whether "friend" is logged in. Maybe the name of the method "getFriendTrips" could be better, like "getCurrentUserFriendTrips".

Rob Hillier said...

Even so, there is a potential NullPointerException when calling friend.isFriendsWith(loggedUser), the fix being either to validate, or assuming isFriendsWith is symmetric, called loggedUser.isFriendsWith(friend) instead.

Sandro Mancuso said...

My idea was not to introduce any new feature, change the business logic or make any assumptions during the refactoring. My aim was to make the code work exactly as it was working before the refactoring but now with a cleaner code.

@Rob: In terms of validating the parameters, my preferred option would be NOT to validate any of them. I, personally, don't like defensive code like that since my acceptance tests should be enough to guarantee that this situation would never happen. The responsibility of checking if the logged user is not null definitely does not belong to this class. Since there is no further information about the code that is calling this method, I left it in there.

@Rob: Good point about the NullPointerException. Well spotted. Just looking at this piece of code, it is a bit difficult to know if the isFriendsWith method is symmetric, as you rightly said so. In my view, this would be a good reason to keep it like that until we have a better understanding of the code base.

@Michael: I agree with you that the name "getFriendTrips" for the method is still not good enough. Probably your suggestion is a better one.

As this is supposed to be a "legacy" system (exercise), changes need to be done bit by bit. When working with legacy, we need to be careful with assumptions and also try not to be carried away and try to refactor the whole code base in one go.

Very good points guys. Thanks.

Post a Comment