Refactoring towards the Liskov Substitution Principle

January 4, 2015

Sometimes you write or encounter a method that return an object that represents a complex result upon which the caller should decide how to proceed.

For example, consider the method with the following signature:

StudentQueryResult FindStudent(string nameSubstring);

This method looks for a student by his name or part (substring) of it. This method can have 3 possible return values:

  1. 1. If there’s no student found that matches the substring, it should return null

2. If there’s exactly one student that matches the substring, it should return all of the details about this student, including the list of courses he completed and the his final grades

3. If there are many students that match the substring, it should return a list of all the relevant names for the user to choose from.

A general call to this method should look something like this:

var result = FindStudent("Arnon");
if (result == null)
{
    MessageBox.Show("No user found");
    return;
}
if (result.StudentInfo != null)
{
    DisplayStudentInfo(result.StudentInfo);
    return;
}
DisplayListOfPossibleMatches(result.PossibleMatches);

In this example, the StudentQueryResult class has multiple responsibilities, and its properties StudentInfo and PossibleMatches are never used together.

Another approach would be to define StudentQueryResult as an abstract base class and derive 2 classes from it:

class SingleStudentQueryResult : StudentQueryResult
{
    public StudentInfo StudentInfo { get; private set; }
    // ...
}

class MultipleMatchesQueryResult : StudentQueryResult
{
    public IEnumerable<string> PossibleMatches { get; private set; }
    // ...
}

And now the caller should look like this:

var result = FindStudent("Arnon");
if (result == null)
{
    MessageBox.Show("No user found");
    return;
}

var singleQueryResult = result as SingleStudentQueryResult;
if (singleQueryResult != null)
{
    DisplayStudentInfo(singleQueryResult.StudentInfo);
    return;
}

DisplayListOfPossibleMatches(((MutipleMatchesQueryResult)result).PossibleMatches);

This method is not much different, but at least the StudentQueryResult object is modeled somewhat more appropriately (and has better cohesiveness).

However, now we’re clearly violating the Liskov Substitution Principle.

What’s the problem with this code?

Let’s put aside the buzzwords and see what the real problems with it are. Frankly, if this method is only used as an internal or private method, it may be somewhat acceptable. But if such a method is exposed on a public API, it imposes few serious problems:

1. The caller must know some hidden knowledge that is not explicitly exposed as part of the API, and therefore creates coupling between the caller and the called method. Of course that this behavior can be documented, but – who really reads the documentation anyway??

2. In the future, if you’d want to change this method to return a 4th subclass of the result, you have a no way to know how existing code (that should still be supported for backward compatibility) will behave. It basically depend upon the order of the ‘if’ statements and the last option that is used as the “default”. For example, suppose that if the student is not yet registered, you want to return a new property that represents that UnregisteredStudentInfo. In the above example, the existing caller will “think” that the returned object is a MultipleMatchesQueryResult and the casting would fail.

Note: If the UnregisteredStudentInfo would derive from StudentInfo everything would be fine, but probably the (registered) student have more information attached to it (e.g. the list of courses, and number of credit points) than the unregistered one, so it doesn’t make sense to derive the UnregisteredStudentInfo from StudentInfo. It could also be that the UnregisteredStudentInfo should have different information attached to it (that is not relevant for a registered student), and then the right thing to do would be to derive both of them from a common base class, but that would break backward compatibility too because you’d need to change the StudentInfo property to be of the base class.

3. This signature implies that the method is blocking. Once you’d want to make it asynchronous, you will need to change the signature of the method or create an overload for it. By itself, this is not a big deal, but only new code will be able to benefit from this, and if you have many such methods in the system and you want to change many of them to work asynchronously, then you’ll end up introducing a whole new set of APIs for these methods. While if you fix the first two problems in the way I’ll describe shortly, you will be able to change the method to work asynchronously in a way that existing code will benefit from it too right away, without breaking backward compatibility!

So how can we refactor the code to resolve this violation?

Assuming that you have a proper test coverage in place, you can refactor the code as follows:

1. Create a new interface StudentQueryResultHandler like this:

Interface IStudentQueryResultHandler
{
    void OnNoMatchFound();
    void OnSingleMatchFound(StudentInfo studentInfo);
    void OnMultiplePossibleMatches(IEnumerable<string> possibleMatches);
}

Now create a new overload of FindStudent as follows:

public void FindStudent(string substring, IStudentQueryResultHandler handler)
{
    var result = FindStudent(substring); // call original method
if (result == null)
{
handler.OnNoMatchFound();
return;
}

if (result.StudentInfo != null)
{
handler.OnSingleMatchFound(result.StudentInfo);
return;
}

handler.OnMultiplePossibleMatches (result.PossibleMatches);
}

If you already released the old method and clients are using it (i.e. you must maintain backward compatibility for it), then you should stop at this point. However, if you can change all of the calling method, then you can proceed this way:

Find all calls to the original method and one by one change them to use the new overload (by creating a new class that implements IStudentQueryResultHandler as appropriate). For example:

class StudentQueryResultHandler : IStudentQueryResultHandler
{
public void OnNoMatch()
{
MessageBox.Show("No user found");
}

public void OnSingleMatchFound(StudentInfo studentInfo)
{
DisplayStudentInfo(singleQueryResult.StudentInfo);
}

public void OnMultiplePossibleMatches(IEnumerable<string> possibleMatches)
{
DisplayListOfPossibleMatches(possibleMatches);
}
}

// …
FindStudent("Arnon", new StudentQueryResultHandler());

After you converted all the calls to use the new overload, you can use the “inline method” refactoring on the call from the new overload to the old overload (this refactoring is available in Resharper, or can be done manually if you don’t have Resharper). This refactoring essentially moves the content of the first overload and embed it inside the call site – i.e. the new overload. It can then delete the old overload.

You’ll probably find out that it also allows you to refactor and simplify this method even further because you no longer have to create the StudentQueryResult object and can get rid of it too.

Additional refactoring steps

It may seem for now that the code is more complex instead of simpler, but in most cases it allows you to continue to refactor and simplify the code to a degree that was not possible before.

OK. Now I want to add the 4th subclass, how do I do that without breaking compatibility?

In this case you have to do the following:

1. Create a new interface that derives from IStudentQueryResultHandler and add the method to handle the 4th case to it.

2. Create an overload of the FindStudent that accepts the new interface instead of the old one. Move all the logic from the old overload to the new one, and change it to call the new method in the relevant case.

3. In the old overload simply call the new overload, supplying a private handler class that maps between the old interface and the new one and provide a default behavior for the new case (this is how you can control the behavior of the old code in the new case in a safe manner!)

Some closing words…

  1. 1. You may use events instead of an interface. That’s fine as long as you keep in mind:
    1. a. It could be that the caller haven’t registered to the event at all

b. There could be multiple callers that are registered to the event.

There shouldn’t be any problem with these cases, but it may depend on the case in hand to decide whether this is appropriate or not.

2. If you want to change the code to be asynchronous, you should let the caller specify a SynchronizationContext (possibly in the constructor or even through configuration, so you don’t need to change all the calls) to avoid race conditions in the code of your clients!

Add comment
facebook linkedin twitter email

Leave a Reply

Your email address will not be published.

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <s> <strike> <strong>

*