Am I creating too many interfaces and classes as a result of unit testing

[Stan Belen]

Suppose I'm working on an application that allows the user to assign employees to the department where they belong.

Here is an example:

Suppose there are 4 employees: Emp01, Emp02, Emp03, and Emp04.

And there are 2 departments: DeptA and DeptB.

The user assigns Emp01 and Emp03 to DeptA and Emp04 to DeptB. This is what it looks like at this point:

DeptA has employees Emp01 and Emp03

DeptB has employee Emp04

Emp02 has not been assigned to any department, yet

I start implementing using TDD using the following steps.

Step 1: I create a class with the method that will be called to update the employee-department mapping

package com.somecompany.service;

@Service
public class EmployeeService {

  public void updateEmployeeDepartmentMapping(Map<String, String> employeeIdToDepartmentId) {

  }

}

Step 2: I create a test class for EmployeeService::updateEmployeeDepartmentMapping method

package com.somecompany.service;

@RunWith(MockitoJUnitRunner.class)
public class EmployeeServiceTest {

  @Test
  public void updateEmployeeDepartmentMapping_whenShouldSave_thenCallRepo() {

  }

}

Step 3: I write the test using interfaces that don't exist yet

package com.somecompany.service;

@RunWith(MockitoJUnitRunner.class)
public class EmployeeServiceTest {

  @InjectMocks
  private EmployeeService employeeService;

  @Mock
  private EmployeeSource employeeSource;

  @Mock
  private ShouldUpdateDecide shouldUpdateDecide;

  @Mock
  private EmployeeRepository employeeRepository;

  @Test
  public void updateEmployeeDepartmentMapping_whenShouldSave_thenCallRepo() {
    Employee emp01 = new Employee();
    Employee emp02 = new Employee();
    Employee emp03 = new Employee();
    Employee emp04 = new Employee();

    List<Employee> empList = List.of(emp01, emp02, emp03, emp04);

    doReturn(empList).when(employeeSource).getEmpList();

    doReturn(true).when(shouldUpdateDecide).shouldUpdate(emp01);
    doReturn(false).when(shouldUpdateDecide).shouldUpdate(emp02);
    doReturn(true).when(shouldUpdateDecide).shouldUpdate(emp03);
    doReturn(true).when(shouldUpdateDecide).shouldUpdate(emp04);

    verify(employeeRepository).updateEmployeeList(List.of(emp01, emp03, emp04));
  }

}

At this point I'll be creating 3 interfaces:

EmployeeSource

ShouldUpdateDecide

EmployeeRepository

And, the interfaces will require classes that will implement them.

The code that I wrote is just a rough example. My question is: is it okay to be creating so many interfaces?

[Stephan van Hulst]

Yes, why wouldn't it?

I'm a big fan of creating interfaces for all my services to implement. I don't just do it for unit testing, but it's definitely a big factor.

Keep going the way you're going.

[Junilu Lacar]

I'm going to take a slightly different tack.

TDD is mostly a design technique. When you're writing tests before the production code, the tests are actually specifications, specifically, design specifications. One of the things that jumps out at me when I read your post is that you're immediately using mocks. That means you're already pulling dependencies into your unit test. This can be a signal that your design is more complicated than it needs to be and that there might be a simpler way to organize the ideas in your code.

You started out with a service class. Why do you need a service class to establish a relationship between Employee and Department? Why bring in the concept of "mapping" right away? Do you even need "mapping"? Why couldn't the Employee and Department classes maintain their own relationships with each other?

If it were me, I'd probably start with a test like this:

@Test
void new_employee_is_not_assigned_to_a_department() {
  Employee emp = new Employee();
  assertEquals(Department.UNASSIGNED, emp.getDepartment());
}

This is easy enough to implement without pulling in too many dependencies right off the bat. Granted, this may be a naive implementation but it gets me started with a simple design and one that I can easily understand. Prefer to always start with a simple system. then work your way incrementally to the more complex.

Keep Gall's Law in mind when you're designing:

John Gall wrote:A complex system that works is invariably found to have evolved from a simple system that worked. A complex system designed from scratch never works and cannot be patched up to make it work. You have to start over with a working simple system.

[Junilu Lacar]

Stan Belen wrote:
At this point I'll be creating 3 interfaces:

EmployeeSource

ShouldUpdateDecide

EmployeeRepository

And, the interfaces will require classes that will implement them.

The code that I wrote is just a rough example. My question is: is it okay to be creating so many interfaces?

My rule of thumb for interfaces is to have them along the boundaries between layers that need to be loosely coupled. Creating interfaces for things that are in the same layer tends to increase complexity with very little gain. The balance is ease of testing against complexity and cognitive weight.

Looking at the test you wrote more, I think there are ways to simplify and clarify it. I'd start with the test name because that's what sets the scene. What's the idea behind the test name updateEmployeeDepartmentMapping_whenShouldSave_thenCallRepo? The name is somewhat cryptic. Grammatically, "when should save" doesn't make sense to me. It looks like you're trying to follow some kind of formula for the name and you're forcing your idea into the form the formula dictates. While this might make your test name conform to the "standard", it creates technical debt because of its lack of clarity.

The thenCallRepo part signals a leaky abstraction for me. Prefer test names that talk about intent, avoid any implementation detail. Intent is more resilient than implementation, i.e., your design should be such that tests still are accurate and make sense even when the underlying implementation changes. By leaking the fact that the repo is called, that locks your design dependency in and makes it harder to change later. There's a psychology to this. Also, "thenCallRepo" is vague. What repo? To get the answer to that, you have to investigate further and read more code. In this example, it's easy enough to deduce that it's probably the EmployeeRepository. However, what if there were multiple repositories?

(continued)

[Junilu Lacar]

Going simpler, I probably would have tried this instead:

class EmployeeTest {
  @Nested
  class SavingChanges {
    private Employee saved;
    private Employee updated;
  
    @BeforeEach 
    void setup() {
      saved = new Employee();
      updated = new Employee();
    }

    @Test
    void save_when_department_changes() {
      saved.setDepartment("IT");
      updated.setDepartment("IT - Development");
      
      assertTrue(save.shouldUpdate(updated));
    }

    @Test
    void dont_save_when_department_is_unchanged() {
      saved.setDepartment("IT");
      updated.setDepartment("IT");
      
      assertFalse(saved.shouldUpdate(updated));
    }
  }
}

The story this test code tells has a small scope in terms of behaviors involved. There are also no dependencies involved in these tests, only Employee objects are being exercised.

[Stan Belen]

Junilu Lacar wrote:I'm going to take a slightly different tack.

TDD is mostly a design technique. When you're writing tests before the production code, the tests are actually specifications, specifically, design specifications. One of the things that jumps out at me when I read your post is that you're immediately using mocks. That means you're already pulling dependencies into your unit test. This can be a signal that your design is more complicated than it needs to be and that there might be a simpler way to organize the ideas in your code.

You started out with a service class. Why do you need a service class to establish a relationship between Employee and Department? Why bring in the concept of "mapping" right away? Do you even need "mapping"? Why couldn't the Employee and Department classes maintain their own relationships with each other?

If it were me, I'd probably start with a test like this:

@Test
void new_employee_is_not_assigned_to_a_department() {
  Employee emp = new Employee();
  assertEquals(Department.UNASSIGNED, emp.getDepartment());
}

This is easy enough to implement without pulling in too many dependencies right off the bat. Granted, this may be a naive implementation but it gets me started with a simple design and one that I can easily understand. Prefer to always start with a simple system. then work your way incrementally to the more complex.

Keep Gall's Law in mind when you're designing:

John Gall wrote:A complex system that works is invariably found to have evolved from a simple system that worked. A complex system designed from scratch never works and cannot be patched up to make it work. You have to start over with a working simple system.

Hi Junilu.

I think I see what you're saying but let me make sure by reiterating:

The way that I understood is that I took a sort of top-down approach. From the very beginning, I created a test class with mocks and basically defined the high-level stages, for example: first of all retrieve the employee list, then find which employee entities to update, and verify that they will be passed to the repository object to update.

A better approach would be to start sort of bottom-up and incrementally build on top of each design as it evolves. You gave the example of a first unit test that instantiates an Employee object and verifies that its initial state of it is that it is not assigned to any department.

I see the advantage of your approach, actually, from the very beginning, you made a design decision by using the Department.UNASSIGNED value as the default value of a newly created Employee object. At this initial point, you could have decided also that let the default value of the Employee department is null or Optional.

With this approach, I'm thinking that the next tests/steps can be:

I don't want my service to blindly update all the Employees in the repository because there will be employees whose department did not change so I will need a mechanism to choose. I'll create a class with a method that returns a boolean, takes as parameters an Employee object and a Department object. Even this step/test made me think about the small design of the method signature which will indicate whether to update or not.

Building on top of the previous test, I probably need a class with a method that will accept a list of the Employees and a Map of String key and Department object value (the key will be the Id of an Employee and the value is the department object for this employee). This method should return a list of employees that need to be updated in the repository

And so on

To be honest, I have a feeling that I might have took too big of steps/tests in the above example but not sure how to make them smaller. Let me know what you think. Thank you.

[Junilu Lacar]

I wrote:Grammatically, "when should save" doesn't make sense to me.

After reading the test code again, I understand now what "when should save" means. The problem is that the code tells a different story because it calls the shouldUpdate() method, not a shouldSave() method.

This is an example of a name that requires the reader to do some mental mapping. This adds to the code's cognitive weight because now the reader has to map the the idea of "should save" to the method shouldUpdate. You may not think it's much but it still slows people down, therefore, it's still technical debt. On the other hand, if you stick with the intent, then the test code can tell the more specific story.

Of course, that means the test names I chose for my previous example could also use some improvement. Maybe this tells a clearer story:

public class EmployeeTest {
  @Nested
  class SavingDepartmentUpdates {
    @Test
    void saved_when_department_is_different() {...}

    @Test
    void not_saved_when_department_is_the_same() {...}
  }
}

If you run this, the JUnit test report will tell a coherent story:

EmployeeTest
 SavingDepartmentUpdates
   saved_when_department_is_different
   not_saved_when_department_is_the_same

Anyone reviewing the test results can already start developing an understanding of what was tested.

[Junilu Lacar]

It seems you and I have very different definitions of top-down and bottom-up design. I look at my approach not from a directional perspective like top/bottom but rather from simpler to more complex, from the very small scope to the wider scope. I still keep the abstractions high level though so I don't see my approach as being bottom up.

[Junilu Lacar]

Stan Belen wrote:
I see the advantage of your approach, actually, from the very beginning, you made a design decision by using the Department.UNASSIGNED value as the default value of a newly created Employee object. At this initial point, you could have decided also that let the default value of the Employee department is null or Optional.

I do this sometimes when I'm pairing/mobbing to see if people are paying attention. You're right to point out Department.UNASSIGNED. There should be a discussion around that to see if that's too big of a step or if something simpler can be done. I find decisions around returning null or Optional to also be interesting. If people don't call this kind of thing out, I'd probably ask "Is this too big of step, to be dependent on Department.UNASSIGNED like this? Is there a simpler way to do this?"

[Tim Holloway]

On a slightly different note, if you've got all those interfaces, you probably shouldn't be using mocking.

My experience with mocking is that it can give feelings of false security. Plus it often requires a certain amount of code-kinking. I published a list of about 10 reasons on the perils of mocking a while back. I think I even made it into a Ranch wiki.

Mocking is a kludge for when you cannot simply swap out a component, but you cannot get full support for it in the limited framework that testing operates in. If your objects implement interfaces, it's a lot better if you simply implement dummy/test classes and swap them for the objects you would otherwise be mocking. Within a mock, you don't have the freedom act in a context-sensitive manner or do complex functions. In a test class, you can code virtually any logic you want. Although if you overdo that, then you end up needing to test the test class itself, so keep it simple!

[Stan Belen]

Junilu, thank you so much for your help. What you talk about is really awesome things. In addition to this forum, are there any resources that you can recommend to get better at this?

[Junilu Lacar]

Stan Belen wrote:

I don't want my service to blindly update all the Employees in the repository because there will be employees whose department did not change so I will need a mechanism to choose. I'll create a class with a method that returns a boolean, takes as parameters an Employee object and a Department object.

Premature optimization - you're adding complexity early on before you even know for sure performance is going to be a problem. Developers suck at performance tuning based on gut feeling / intuition. Don't do it. If performance becomes a problem, simple code is easier to tune than complex code.

[Junilu Lacar]

Stan Belen wrote:

Building on top of the previous test, I probably need a class with a method that will accept a list of the Employees and a Map of String key and Department object value (the key will be the Id of an Employee and the value is the department object for this employee). This method should return a list of employees that need to be updated in the repository

Premature generalization. Sure, you might end up here anyway but by skipping right to the generalization, you've deprived yourself of seeing other, possibly simpler designs. Start with specific examples, start simple. Then build up to more complicated. I like James Grenning's ZOMBIES for this: Zero-One-Many-Boundaries-Interfaces-Exceptions-Simple Scenarios/Solutions.

[Junilu Lacar]

Stan Belen wrote:To be honest, I have a feeling that I might have took too big of steps/tests in the above example but not sure how to make them smaller.

I think this is where the psychology of programming comes in and it has to do with the tendency to prefer generalizations over specifics. When we're thinking about generalizations, we're trying to allay our Fear of Missing Out (FOMO), the fear that we've left gaps in our code where bugs can creep in. We try to think of all the different scenarios our code needs to be able to handle. This adds complexity very early on. Starting with specifics doesn't feel right when we have this mindset because it makes us feel like we missed things.

The way I deal with that is to accept the fact that there will be gaps, so Eff it. Instead of trying to think of and fill in all possible gaps, I'll use tests to slowly but surely clearly identify each specific gap I can think of, and then plug them up one at a time as I discover them. This way, I can build up from the simple to the complex. On the way, I learn more about the design and keep as many of my options open until the last possible minute.

[Junilu Lacar]

Stan Belen wrote:Junilu, thank you so much for your help. What you talk about is really awesome things. In addition to this forum, are there any resources that you can recommend to get better at this?

I follow a lot of people and have formed my points of view by adopting their ideas when they resonate with my experience. In other words, I steal people's ideas.  

Kent Beck, Martin Fowler, Ward Cunningham, Ron Jeffries, James Grenning, Alistair Cockburn, Andy Hunt, Dave Thomas, Bob Martin. (These are some of the people who came up with the Agile Manifesto)

Michael Feathers, Corey Haines, Eric Evans, Joshua Kerievsky, Arlo Belshee, Doc Norton, Anthony Hoare, E.W. Dijkstra and many others have also influenced the way I approach design.

As you can probably tell, I read a lot. I also practice a lot. The reading gives you different perspectives. The practice gives you a sense of what ideas resonate with your current understanding and gives you the opportunity to develop and deepen that understanding.

[Junilu Lacar]

One more person I want to mention: Jack Reeves and his three essays on Code as Design

Reading these articles was how I formed the opinion that coding is a design activity and writing tests before writing production code is a design technique.

[Stan Belen]

A "leaky abstraction" has been mentioned in this thread. I did a bit of research on this but I still do not have a good understanding of it. I'm hoping that with the example unit test that I'll put here somebody can help me understand "leaky abstraction" better.

package com.somecompany.employeeapp.service;

import com.somecompany.employeeapp.model.Employee;
import com.somecompany.employeeapp.repository.EmployeeRepository;
import com.somecompany.employeeapp.source.EmployeeDepartmentSource;

// import junit and mockito stuff

public class EmployeeServiceImplTest {

    private EmployeeServiceImpl employeeServiceImpl;

    private EmployeeRepository employeeRepository;

    private EmployeeDepartmentSource employeeDepartmentSource;

    @BeforeEach
    public void setup() {
        employeeRepository = mock(EmployeeRepository.class);
        employeeDepartmentSource = mock(EmployeeDepartmentSource.class);

        employeeServiceImpl = new EmployeeServiceImpl(employeeRepository, employeeDepartmentSource);
    }

    @Test
    public void employee_department_change_update_in_data_store() {
        Employee employee = Employee.builder().id(10L).name("John Smith").department("marketing").build();

        doReturn(List.of(employee)).when(employeeDepartmentSource).getEmployee();

        employeeServiceImpl.updateEmployeeDepartment(Map.of(10L, "sales"));

        assertEquals("sales", employee.getDepartment());

        verify(employeeRepository).update(List.of(employee));
    }

}

Suppose that I encountered this kind of unit test that was written by somebody else or me some time ago

Regarding "leaky abstraction", the way that I understood it is that, say in this example, there are 3 layers: repository, service, and source. The unit test and the production code should have had an interface for each of these layers, I'm guessing. Please advise.

Also, I think that this code is basically tying the implementation of the production code to the unit test. So if a developer will need to change the production code that is being tested here, the unit test will also need to change. What would be a way to avoid testing the implementation so that changes in the production code would not entail having to change the unit test?

[Tim Holloway]

As a general rule, if you change production code, you should change the unit test. Otherwise you won't be testing the production changes adequately. Unit tests are for both regression testing (make sure changes didn't break existing functions) but also for feature testing (making sure the changes do what they are supposed to do).

[Tim Holloway]

By the way, a useful IDE hack.

As I said, unit tests and production code should generally be kept in sync. However, if you're laying out your project in a Maven-like manner, the two files are vastly separate in the overall project directory structure. So to ease maintenance, the JavaDoc comments in my tested classes typically include something like this:

/**
  * Foo Manager. Does what its name says.
  *
  * Author: Me
  * Written: 1984-13-02
  * Tested By: @see FooManagerTest
 */
public class FooManager {
...

In many IDE's the "@see" annotation will hyperlink to the test class, so to edit the test, simply do the appropriate click. Some IDEs many not even need "@see", but it shows up in generated JavaDocs, so why not do it anyway?

Going the other direction is, of course, usually even easier, since there's almost certain to be a usage of FooManager in FooManagerTest that you can link on.