Last update: 14/10/2019 21:30:41

Resolving God Objects:
Tests Dispatch

Context

This article is part of the God object resolution series. To illustrate it, we use a running example through the whole series. So far, we dispatched the logics and the state of the God object into new features. This article is about the dispatch of the tests of the God object to the relevant features.

Problem

Each feature is now complete, but is not properly tested. The tests of the God object cover them but, since these features may be directly used elsewhere, it is important to ensure that they are correct, without the God object layer.

Solution

As we did for the code, we can move the test methods iteratively, one after the other. Following what we did for the code, we start with testing the hiring method. Two tests are on this method: retrieving a hired employee, and ensuring hiring generates different IDs. Let's take the latter:


	@Test
	public void testEmployeesHaveDifferentIDs() {
		EmployeeManager manager = new EmployeeManager();
		int employeeID1 = manager.hireEmployee(new TestEmployee());
		int employeeID2 = manager.hireEmployee(new TestEmployee());
		int employeeID3 = manager.hireEmployee(new TestEmployee());
		assertNotEquals(employeeID1, employeeID2);
		assertNotEquals(employeeID1, employeeID3);
		assertNotEquals(employeeID2, employeeID3);
	}

This logics is now in the EmployerImpl, so we can replace the God object by this feature:


	@Test
	public void testEmployeesHaveDifferentIDs() {
		Employer employer = new EmployerImpl(new HashMap<>());
		int employeeID1 = employer.hireEmployee(new TestEmployee());
		int employeeID2 = employer.hireEmployee(new TestEmployee());
		int employeeID3 = employer.hireEmployee(new TestEmployee());
		assertNotEquals(employeeID1, employeeID2);
		assertNotEquals(employeeID1, employeeID3);
		assertNotEquals(employeeID2, employeeID3);
	}

This test can now be moved to a dedicated EmployerImplTest class, unless you prefer to do move all its tests at once. Notice the additional map we must create to instantiate the employer. Beside this, the test remains unchanged, showing a trivial case. But it is not always the case, as we will see with the other hiring test:


	@Test
	public void testEmployeeCanBeRetrievedAfterHiring() {
		Employee employee = new TestEmployee();
		EmployeeManager manager = new EmployeeManager();
		int employeeID = manager.hireEmployee(employee);
		assertEquals(employee, manager.getEmployee(employeeID));
	}

By replacing the God object by the feature, here is what we obtain:


	@Test
	public void testEmployeeCanBeRetrievedAfterHiring() {
		Employee employee = new TestEmployee();
		Map<Integer, Employee> employees = new HashMap<>();
		Employer employer = new EmployerImpl(employees);
		int employeeID = employer.hireEmployee(employee);
		assertEquals(employee, employees.get(employeeID));
	}

Beside the expected replacement, we must interact with the employees map to retrieve the hired employee. Indeed, the employer provides no method to retrieve it. This is not an issue, though, because it provides a test focusing on the hiring method alone.

We can now pass to the next test, which is firing the employee:


	@Test
	public void testEmployeeCannotBeRetrievedAfterFiring() {
		Employee employee = new TestEmployee();
		EmployeeManager manager = new EmployeeManager();
		int employeeID = manager.hireEmployee(employee);
		manager.fireEmployee(employeeID);
		assertNull(manager.getEmployee(employeeID));
	}

After the minimal replacement, here is what we obtain:


	@Test
	public void testEmployeeCannotBeRetrievedAfterFiring() {
		Employee employee = new TestEmployee();
		Map<Integer, Employee> employees = new HashMap<>();
		Employer employer = new EmployerImpl(employees);
		int employeeID = employer.hireEmployee(employee);
		employer.fireEmployee(employeeID);
		assertNull(employees.get(employeeID));
	}

Similarly to the previous test, we had to interact directly with the map to retrieve the employee. However, now that the employees map is used as an external dependency, it is not enough to ensure that hiring and firing work together. Indeed, other features will interact with our dependency, so we need to ensure that we properly use it. In other words, we also need to ensure that when another feature adds an employee, we can properly fire it:


	@Test
	public void testEmployeeCannotBeRetrievedAfterFiring() {
		Employee employee = new TestEmployee();
		Map<Integer, Employee> employees = new HashMap<>();
		Employer employer = new EmployerImpl(employees);
		int employeeID = 123;
		employees.put(employeeID, employee);
		employer.fireEmployee(employeeID);
		assertNull(employees.get(employeeID));
	}

Rather than needing both tests (hire and fill map), we only need to keep the second (fill map). Indeed, a previous test already validates that hiring updates the map, so we can consider that they are equivalent. Said another way, the complete refactoring of the initial test correspond to the last version.

Another aspect highlighted by this test is the evolution of the coverage. If you execute all the tests we have so far, you can see that EmployeeManager.fireEmployee(employeeID) is not covered anymore. In fact, the coverage is decreasing since we started the dispatch of the tests. The reason why we didn't see it is that we removed calls to the hiring method of the God object, which is still in use in many other tests. Thus, we reduced the amount of calls, but as long as we call it at least once it will remain covered. The firing method, however, was called only in its own test, and now that the test have been dispatched, it is not called anymore.

The coverage decrease should occur iteratively, after each dispatched test, until the God object reaches a 0% coverage. Indeed, our current goal is to dispatch all the tests to their relevant features. If you plan to get rid of the God object, then it should be fine, since you will remove the code and the tests altogether. If instead you plan to transform it into a façade, then the next phases will provide you the right tests for that.

Coming back to our test dispatch, we have now refactored all the tests of EmployerImpl. If you didn't do it yet, you can move them to a dedicated EmployerImplTest class. The next test is for the EmployeeManager.setEmployee(employeeID, newDetails):


	@Test
	public void testEmployeeDetailsCanBeChanged() {
		EmployeeManager manager = new EmployeeManager();
		Employee employee = new TestEmployee();
		int employeeID = manager.hireEmployee(employee);
		Employee newDetails = new TestEmployee();
		manager.setEmployee(employeeID, newDetails);
		assertNotEquals(employee, manager.getEmployee(employeeID));
		assertEquals(newDetails, manager.getEmployee(employeeID));
	}

This method has been moved, but several elements have also been renamed, which leads us to a significant refactoring:


	@Test
	public void testEmployeeDetailsCanBeChanged() {
		// Setup
		Map<Integer, Employee> contracts = new HashMap<>();
		int employeeID = 123;
		Employee oldContract = new TestEmployee();
		Employee newContract = new TestEmployee();
		contracts.put(employeeID, oldContract);
		
		// Execute
		new EmployeeDetailsImpl(contracts).setContract(employeeID, newContract);
		
		// Test
		assertNotEquals(oldContract, contracts.get(employeeID));
		assertEquals(newContract, contracts.get(employeeID));
	}

As a summary, we:

added the map of employees, renamed as contracts ;
replaced the hiring by the filling of the map ;
replaced the setEmployee(...) by the renamed setContract(...) ;
renamed corespondingly the employee variables by contract variables ;
wrote the assertions on the contracts map directly.

Since we didn't create a Contract class, we still use the Employee class for our contracts. It may seem odd, but this is the result of the iterative process: you may do it before to reach this step, or you may leave it for later.

Although we didn't give it a proper highlight so far, you should not only run your tests after dispatching your code or test, but also check your test coverage. As we mentionned, the progressive dispatch of the tests decreases the coverage of the God object, something we also observe here. However, with the deeper refactoring of the test, we also decreased the coverage of EmployeeDetailsImpl. Indeed, the method getContract(employeeID) is not covered anymore. This should be fixed before to go further: since it is not covered, we know that no other test calls it, so we need to add one:


	@Test
	public void testEmployeeDetailsCanBeRetrieved() {
		// Setup
		Map<Integer, Employee> contracts = new HashMap<>();
		int employeeID = 123;
		Employee expectedContract = new TestEmployee();
		contracts.put(employeeID, expectedContract);

		// Execute
		Employee actualContract = new EmployeeDetailsImpl(contracts).getContract(employeeID);

		// Test
		assertEquals(expectedContract, actualContract);
	}

With this new test, we resolved our test coverage. We could have done it another way, by using the method in the previous test, like the original version. However, we wouldn't have properly tested the interaction with the map dependency. Rather than consisten behaviours between methods, we seek for consistency through this dependency.

Let's go now to the next test:


	@Test
	public void testEmployeeAddressCanBeChanged() {
		EmployeeManager manager = new EmployeeManager();
		Employee employee = new TestEmployee();
		int employeeID = manager.hireEmployee(employee);

		Address address1 = new TestAddress();
		manager.setAddress(employeeID, address1);
		assertEquals(address1, manager.getAddress(employeeID));

		Address address2 = new TestAddress();
		manager.setAddress(employeeID, address2);
		assertNotEquals(address1, manager.getAddress(employeeID));
		assertEquals(address2, manager.getAddress(employeeID));
	}

Which we refactor as such:


	@Test
	public void testEmployeeAddressCanBeChanged() {
		EmployeeDetails details = new EmployeeDetailsImpl(new HashMap<>());
		int employeeID = 123;

		Address address1 = new TestAddress();
		details.setAddress(employeeID, address1);
		assertEquals(address1, details.getAddress(employeeID));

		Address address2 = new TestAddress();
		details.setAddress(employeeID, address2);
		assertNotEquals(address1, details.getAddress(employeeID));
		assertEquals(address2, details.getAddress(employeeID));
	}

Beside replacing the God object by its dedicated feature, we should notice another important point: the absence of contract. Indeed, the God object allows to generate an employeeID only through its hiring method. We may dig in the code and see that it is not required, but these are implementation details, not something we can guess from the methods of the object. With the dedicated feature, however, the employeeID can be generated by hand, so we don't need to hire (or set a contract). This is possible because this part of the state is externalized: we provide the contracts map to the constructor. This is also for this reason that we could make the assertions directly on that map for the previous tests. At the opposite, the addresses map is managed inside the details instance, forcing us to use its getAddress(employeeID) to do the assertions.

The test about the phone numbers is highly similar:


	@Test
	public void testEmployeePhoneNumberCanBeChanged() {
		EmployeeManager manager = new EmployeeManager();
		Employee employee = new TestEmployee();
		int employeeID = manager.hireEmployee(employee);

		String phone1 = "123456789";
		manager.setPhoneNumber(employeeID, phone1);
		assertEquals(phone1, manager.getPhoneNumber(employeeID));

		String phone2 = "987654321";
		manager.setPhoneNumber(employeeID, phone2);
		assertNotEquals(phone1, manager.getPhoneNumber(employeeID));
		assertEquals(phone2, manager.getPhoneNumber(employeeID));
	}

Consequently, it goes through the same refactoring:


	@Test
	public void testEmployeePhoneNumberCanBeChanged() {
		EmployeeDetails details = new EmployeeDetailsImpl(new HashMap<>());
		int employeeID = 123;

		String phone1 = "123456789";
		details.setPhoneNumber(employeeID, phone1);
		assertEquals(phone1, details.getPhoneNumber(employeeID));

		String phone2 = "987654321";
		details.setPhoneNumber(employeeID, phone2);
		assertNotEquals(phone1, details.getPhoneNumber(employeeID));
		assertEquals(phone2, details.getPhoneNumber(employeeID));
	}

With that, we have refactored all the tests of the EmployeeDetailsImpl. If you didn't do it after each test, you can move them to a dedicated EmployeeDetailsImplTest class.

The last tests are the following:


	@Test
	public void testEmployeeSalaryCanBeChanged() {
		EmployeeManager manager = new EmployeeManager();
		Employee employee = new TestEmployee();
		int employeeID = manager.hireEmployee(employee);

		double salary1 = 123;
		manager.setSalary(employeeID, salary1);
		assertEquals(salary1, manager.getSalary(employeeID), 0);

		double salary2 = 456;
		manager.setSalary(employeeID, salary2);
		assertNotEquals(salary1, manager.getSalary(employeeID));
		assertEquals(salary2, manager.getSalary(employeeID), 0);
	}

	@Test
	public void testEmployeeSalaryCanBeRaised() {
		EmployeeManager manager = new EmployeeManager();
		Employee employee = new TestEmployee();
		int employeeID = manager.hireEmployee(employee);

		manager.setSalary(employeeID, 123);
		manager.raiseSalary(employeeID, 321);
		assertEquals(444, manager.getSalary(employeeID), 0);
	}

We refactor them for SalarySettingsImpl:


	@Test
	public void testEmployeeSalaryCanBeChanged() {
		SalarySettings settings = new SalarySettingsImpl();
		int employeeID = 42;

		double salary1 = 123;
		settings.setSalary(employeeID, salary1);
		assertEquals(salary1, settings.getSalary(employeeID), 0);

		double salary2 = 456;
		settings.setSalary(employeeID, salary2);
		assertNotEquals(salary1, settings.getSalary(employeeID));
		assertEquals(salary2, settings.getSalary(employeeID), 0);
	}

	@Test
	public void testEmployeeSalaryCanBeRaised() {
		SalarySettings settings = new SalarySettingsImpl();
		int employeeID = 42;

		settings.setSalary(employeeID, 123);
		settings.raiseSalary(employeeID, 321);
		assertEquals(444, settings.getSalary(employeeID), 0);
	}

And that's it! All the tests are now refactored and should be moved to the test classes of their corresponding features if you (still) didn't do it yet. The God object has no remaining test, and has reached a test coverage of 0%. At the opposite, the features it builds on are fully tested, with a 100% coverage.

Now, you should be aware of what you want to do with your God object. If you want to get rid of it, then you can directly refactor its uses to replace it with the features. If you want instead to keep it as a façade, then we must now transform it into one. If you are not sure what to do, check the final refactoring phase to see if the façade may be useful.

Resolving God Objects:Tests Dispatch

Context

Problem

Solution

Resolving God Objects:
Tests Dispatch