A while ago, I wrote about refactoring 6K LoC class The main benefit of such a refactoring is to have a maintainable codebase.
Last week, I fixed a small bug. The bug itself is trivial. However, the way I enjoyed the new code and wrote unit test was more fun to share. I called it a beautiful solution. And I loved that work.
The Story
In the Refactoring post, I introduced the School and Student to demonstrate the problems. One day, I got a bug report (speak in the blog language, not my real project language)
When removing a student by student’s id, an exception was thrown if the id is invalid.
The expected behavior is not throwing an exception if id is invalid (or not found). It is just a normal business rule in the application.
To demonstrate the story here is the simplest code
public class School
{
protected internal IList<Student> _students = new List<Student>();
private readonly StudentManager _studentManager;
public School()
{
_studentManager = new StudentManager(this);
}
public virtual void AddStudent(Student student)
{
_studentManager.Add(student);
}
public virtual Student FindStudent(Guid id)
{
throw new ArgumentException($"The student you are looking for does not exist {id}", nameof(id));
}
public virtual void Remove(Student student)
{
_students.Remove(student);
}
}
public class Student
{
public virtual Guid Id { get; protected set; }
}
And the consumer code (a simple console application)
class Program
{
static void Main(string[] args)
{
Console.WriteLine("Welcome to the test");
try
{
var school = new School();
var studentId = Guid.NewGuid();
var student = school.FindStudent(studentId);
school.Remove(student);
}
catch (Exception e)
{
Console.WriteLine(e);
}
Console.WriteLine("Press key to exit.");
Console.Read();
}
}
Once you run the code
The Manager
Remember that School is a ghost class. And the student management is delegated to StudentManager. Any attempt to fix directly in School class is a big risk.
There is a small issue in the consumer code. The client has to do 2 things
- Find a student by id
- Ask School to remove that student
They are too much for a client to know and make decisions. What the client wants is the ability to remove a student by id without throwing an exception.
public class StudentManager
{
private readonly School _school;
public StudentManager(School school)
{
_school = school;
}
public void Add(Student student)
{
_school._students.Add(student);
}
public bool TryRemove(Guid studentId)
{
var student = _school._students.FirstOrDefault(x => x.Id == studentId);
if (student == null)
return false;
return _school._students.Remove(student);
}
}
The StudentManager will take care of the logic.
The Client will be happy
class Program
{
static void Main(string[] args)
{
Console.WriteLine("Welcome to the test");
try
{
var school = new School();
var studentId = Guid.NewGuid();
var studentManager = new StudentManager(school);
var result = studentManager.TryRemove(studentId);
Console.WriteLine($"Are you removed? {result}");
}
catch (Exception e)
{
Console.WriteLine(e);
}
Console.WriteLine("Press key to exit.");
Console.Read();
}
}
Why? Because it just needs to ask StudentManager to handle the logic (in a single method call) and deal with result in its own way.
The Unit Test
So far I am very happy with the fix without touching the existing giant class (School class). Good!
Good! How am I going to test this implementation?
The system data access layer uses NHibernate which requires all domain properties virtual. That design brings us a big benefit in term of unit testing. We can either mock or substitute objects.
To understand more about unit test I highly recommend this course from Pluralsight Writing Highly Maintainable Unit Tests.
Due to the complexity of the legacy code, I tended to use Mock (which is an interaction test) rather than Substitute (which is a state test).
Ok, what do I mean by Interaction Test and State Test in this example? We will test the StudentManager.TryRemove method.
What To Test?
From the business logic, we will test at least 2 things
- A student is removed if the student id is valid.
- If a student id is not valid, do not throw an exception.
How To Test?
The #2 logic should be simple to test. This code will ensure that
[TestClass]
public class StudentManagerTests
{
[TestMethod]
public void TryRemove_InvalidStudentId_NotThrowException()
{
// Arrange
var school = new School();
var sut = new StudentManager(school);
// Act
var result = sut.TryRemove(Guid.NewGuid());
// Assert
Assert.IsFalse(result);
}
}
However, #1 is not straight away depending on the mind of the developer. Here are at least 2 ways to think about it
- The implementation has to find a student, then remove him if found. Keywords here are the 2 verbs (find and remove). Some might refer to verify those 2 actions invoked. That is the interaction test.
- The other might simply think: it does not matter what it does (the implementation) as far as a student is removed. Before the school has 10 students. After invoking the method, it has exactly 9 students, and cannot find that student in the school anymore. That is the state test.
I would go for the state test.
Everything should be easy if all properties are public. But that is not the case. School holds an internal protected student list. Student has a protected Id setter. And we do not want to expose them as public.
The key is to Substitute them. In the Unit Test we create derived classes and override the method we want to hook in or bypass.
Prepare school and student substituted.
internal class StudentSchool : School
{
public StudentSchool(IEnumerable<Student> students)
{
_students = students.ToList();
}
}
internal class IdStudent : Student
{
public IdStudent(Guid id)
{
Id = id;
}
}
They are internal (only visible in the unit test assembly).
Without any change to the production code, I have tests covered
internal class StudentSchool : School
{
public StudentSchool(IEnumerable<Student> students)
{
_students = students.ToList();
}
public int StudentCount => _students.Count;
public bool Exists(Guid studentId)
{
return _students.Any(x => x.Id == studentId);
}
}
internal class IdStudent : Student
{
public IdStudent(Guid id)
{
Id = id;
}
}
[TestClass]
public class StudentManagerTests
{
[TestMethod]
public void TryRemove_InvalidStudentId_NotThrowException()
{
// Arrange
var school = new School();
var sut = new StudentManager(school);
// Act
var result = sut.TryRemove(Guid.NewGuid());
// Assert
Assert.IsFalse(result);
}
[TestMethod]
public void TryRemove_ValidStudentId_CountReduce1AndCannotFindAgain()
{
// Arrange
var batman = new IdStudent(Guid.NewGuid());
var superman = new IdStudent(Guid.NewGuid());
var spiderman = new IdStudent(Guid.NewGuid());
var school = new StudentSchool(new[] {batman, spiderman, superman});
var sut = new StudentManager(school);
// Act
var result = sut.TryRemove(superman.Id);
// Assert
Assert.IsTrue(result);
Assert.AreEqual(2, school.StudentCount);
Assert.IsFalse(school.Exists(superman.Id));
}
}
Oh yeah! Superman is removed 🙂 Who needs Superman btw 😛
There are pain and joy in refactoring + unit testing code. The more you code with the improvement mindset, the more joy you have. It is a journey where you become better and better after each line of code. Keep going my friends.
Happy weekend fellows.
