In this simple article, I want to demonstrate the importance of Test Driven Development approach during the software development process.
Introduction
For example, I will use a simple code from a real-world scenario. The code was merged into the codebase of the legacy application, and the only way I discovered it was when it caused one particular bug in the acceptance environment. Luckily, the bug did not ever leak in the production environment, so it never caused an issue. So instead of risking a bug bubbling up, I put on my defensive programmer hat and proactively fixed the bug to prevent any possible future problems.
Let’s have a hypothetical scenario where a big company wants to close a month to compute employees’ salaries. You have a special report which computes the sum of all employees for every department in the company. For our proof, we do not care about employees’ formulas for calculating their salary. But what we care about is whether the employees closed their worksheets at the end of the month. If the employee does not close the worksheet at the end of the month, the salary will be NULL.
All employees want their salaries, so it never happened before that one of them would not close their worksheet. So when making a department report, either all employees had their worksheets closed (all salaries are not NULL) or non of the employees had worksheets closed which meant that month was not over. In this case, we are also safe as reports are never run before the end of the month they report.
And in the end, I need to add only the last information. If the report does not have employees set or the list of employees, thus salaries are empty, you know it is okay. But it would be best if you handled such situations.
Original code
Here is an original code for the scenario described above:
public class MonthlySalaryReport {
private List<Employee> employees;
public MonthlySalaryReport() {
}
public List<Employee> getEmployees() {
return employees;
}
public void setEmployees(List<Employee> employees) {
this.employees = employees;
}
/**
* @return :
* true when reporting month is over (if employee salaries are set)
* false when reporting month is not over (if employee salaries are not set, they equal to null)
* null if report does not contain any employees
*/
public Boolean canComputeMonthlySalaries() {
if (CollectionUtils.isNotEmpty(getEmployees())) {
return getEmployees().get(0).getSalary() != null;
}
return null;
}
}As you might immediately suspect, there is something wrong with the method canComputeMonthlySalaries().
First, the method definition is misleading as it expects to give you an idea about the internal state based on the condition outside the method scope. When and how it will be used is not the method’s concern and should definitely not be part of the method description. It would be wise to describe only the internal state, which is described by the text in brackets.
Second, the method naively decides if we can compute salaries for the whole department based on the state of the status of the first employee in the list. And here lies a giant bug which went undetected and flew under the radar for so many years for a single reason – there were no tests written for MonthlySalaryReport.
Discovering the bug
Imagine your department has like ten people. The probability that one of them forgets to close the worksheet at the end of the month is small. As the company where this reporting application was used had only 50 employees max, the probability was higher but still small enough that the bug never occurred. But now, imagine a company like Amazon with millions of employees. What is the chance that one out of all employees will forget to close the worksheet?
So we have a case that the algorithm does not take into account. One or more employees may not have the worksheet closed. Or vice versa, one or more employees might complete their worksheets early. So there is the possibility of ambiguity in the employees’ worksheets. Yet, the algorithm cares only about the first person on the list. The report can be incorrectly run or stopped if such an employee is first on the list.
Writing tests
The first step before the refactoring was to write test scenarios for already existing algorithm implementation. This way, I can refactor the existing code much faster.
public class MonthlySalaryReportTest {
@Test
public void testCanComputeMonthlySalaries_returnNull_whenEmployeesAreNotSet() {
MonthlySalaryReport monthlySalaryReport = new MonthlySalaryReport();
assertNull(monthlySalaryReport.canComputeMonthlySalaries());
}
@Test
public void testCanComputeMonthlySalaries_returnNull_whenEmployeesAreSetButAreEmptyList() {
MonthlySalaryReport monthlySalaryReport = new MonthlySalaryReport();
monthlySalaryReport.setEmployees(new ArrayList<>());
assertNull(monthlySalaryReport.canComputeMonthlySalaries());
}
@Test
public void testCanComputeMonthlySalaries_returnTrue() {
Employee em1 = new Employee();
em1.setSalary(1000);
Employee em2 = new Employee();
em2.setSalary(2000);
List<Employee> employees = new ArrayList<>();
employees.add(em1);
employees.add(em2);
MonthlySalaryReport monthlySalaryReport = new MonthlySalaryReport();
monthlySalaryReport.setEmployees(employees);
assertFalse(monthlySalaryReport.canComputeMonthlySalaries());
}
@Test
public void testCanComputeMonthlySalaries_returnFalse() {
Employee em1 = new Employee();
em1.setSalary(null);
Employee em2 = new Employee();
em2.setSalary(null);
List<Employee> employees = new ArrayList<>();
employees.add(em1);
employees.add(em2);
MonthlySalaryReport monthlySalaryReport = new MonthlySalaryReport();
monthlySalaryReport.setEmployees(employees);
assertTrue(monthlySalaryReport.canComputeMonthlySalaries());
}
@Test(expected = IllegalStateException.class)
public void testCanComputeMonthlySalaries_returnIllegalStateException() {
Employee em1 = new Employee();
em1.setSalary(null);
Employee em2 = new Employee();
em2.setSalary(2000);
List<Employee> employees = new ArrayList<>();
employees.add(em1);
employees.add(em2);
MonthlySalaryReport monthlySalaryReport = new MonthlySalaryReport();
monthlySalaryReport.setEmployees(employees);
monthlySalaryReport.canComputeMonthlySalaries();
}
}Creating a few simple unit tests quickly gave me a solid base for further refactoring. All tests were passing except the last one, which caught the ambiguity in the algorithm. Removing ambiguity will be the aim of the next step.
Refactored code
Immediately after creating unit tests, I could refactor the existing method. The code below is refactored code passing all tests and cases:
/**
* Figure out if computing monthly salary repost is possible.
* <p>
* @return An optional of boolean:
* <b>Optional(TRUE)</b> if all the employees have salary.
* <b>Optional(FALSE)</b> if at least one employee do not have salary.
* <b>Optional.empty()</b> if the employee list is either null or has no employees
*/
public Optional<Boolean> canComputeMonthlySalaries() {
if (this.employees == null) {
return Optional.empty();
}
if (CollectionUtils.isNotEmpty(this.employees)) {
int hasSalaryCount = 0;
int doNotHaveSalaryCount = 0;
for (Employee employee : this.employees) {
if (employee.getSalary() == null) {
doNotHaveSalaryCount++;
} else {
hasSalaryCount++;
}
}
if (hasSalaryCount != this.employees.size() && doNotHaveSalaryCount != this.employees.size()) {
throw new IllegalStateException("Monthly report has incorrect state.");
}
return hasSalaryCount == this.employees.size() ? Optional.of(Boolean.TRUE) : Optional.of(Boolean.FALSE);
}
return Optional.empty();
}Yes, the code is not simply one-line logic of O(1) time complexity as it was before. Instead, it decreases its runtime to O(N).
Indeed it is debatable if it is worth being so strict in such a case. But the enforcement should be discussed before, and the note should be part of the code. Otherwise, the code contained a bug, and it was pure luck it did not occur so far in a production environment.
Going from O(1) to O(N) for a small number of employees is nothing. But in the case of a company like Amazon, with millions of employees, other techniques and algorithms should be used to report people’s salaries.
We not only improved the code robustness. But we even included Java’s Optional as the NULL replacement and covered two other cases.
And on top of that, we even improved the method documentation.
Conclusion
Through this simple article, I wanted to show why Test Driven Development is essential. You do not need a lot, and immediately your code becomes more robust and less error-prone.
I am figuring out the test of TDD lately and hope you are or will be too. Practising TDD is like one of the signs 10X developers or good developers; at least, I would ask if I offer somebody a software engineer job.
Do you have your trick or know another way how to refactor and make code better? Let us know in the comments below the article. All future readers would like to hear your ideas and stories.