How to avoid class self-use

Question

I have the following class:

public class MyClass
{

    public void deleteOrganization(Organization organization)
    {
        /*Delete organization*/

        /*Delete related users*/
        for (User user : organization.getUsers()) {
            deleteUser(user);
        }
    }

    public void deleteUser(User user)
    {
        /*Delete user logic*/
    }
}

This class represents a self-use in that its public method deleteOrganization uses its other public method deleteUser. In my case this class is a legacy code against which I started adding unit tests. So I started by adding a unit test against the first method deleteOrganization and ended up figuring out that this test has been extended to also test the deleteUser method.

Problem

The problem is that this test is not isolated anymore (it should only test the deleteOrganization method). I was obliged to treat the different conditions related to deleteUser method in order to pass it so to pass the test, which has hugely increased the complexity of the test.

Solution

The solution was to spy the class under test and stub deleteUser method:

@Test
public void shouldDeleteOrganization()
{
    MyClass spy = spy(new MyClass());

    // avoid invoking the method
    doNothing().when(spy).deleteUser(any(User.class));

    // invoke method under test
    spy.deleteOrganization(new Organization());
}

New Problem

Though the previous solution solves the problem, it's not recommended as the javadoc of the spy method states:

As usual you are going to read the partial mock warning: Object oriented programming is more less tackling complexity by dividing the complexity into separate, specific, SRPy objects. How does partial mock fit into this paradigm? Well, it just doesn't... Partial mock usually means that the complexity has been moved to a different method on the same object. In most cases, this is not the way you want to design your application.

The complexity of the deleteOrganization method has been moved to deleteUser method and this is caused by the class self-use. The fact that this solution is not recommended, in addition to the In most cases, this is not the way you want to design your application statement, suggests that there is a code smell, and refactoring is indeed needed to improve this code.

How to remove this self-use? Is there a design pattern or a refactoring technique that can be applied?

Answer 1

Class self-use isn't necessarily a problem: I'm not yet convinced that "it should only test the deleteOrganization method" for any reason other than personal or team style. Though it's helpful to keep deleteUser and deleteOrganization into independent and isolated units, that isn't always possible or practical—especially if the methods call one another or rely on a piece of common state. The point of testing is to test the "smallest testable unit"—which does not require that methods are independent or that they can be tested independently.

You have a few choices, depending on your needs and how you expect your codebase to evolve. Two of these are from your question, but I'm restating them below with advantages.

• Test black-box.

  If you were to treat MyClass as an opaque interface, you likely wouldn't expect or require deleteOrganization to repeatedly call deleteUser, and you can imagine the implementation changing such that it doesn't do so. (Maybe a future upgrade would have a database trigger take care of a cascading delete, for instance, or a single file deletion or API call might take care of your organizational delete.)

  If you treat deleteOrganization's call to deleteUser as a private method call, then you're left testing MyClass's contract and not its implementation: Create an organization with some users, call the method, and check that the organization is gone and that the users are too. It may be verbose, but it is the most correct and flexible test.

  This may be an attractive option if you expect your MyClass to change drastically, or to get a whole new replacement implementation.

• Split the class horizontally into OrganizationDeleter and UserDeleter.

  To make the class more "SRPy" (i.e. conforming better to the Single Responsibility Principle), as the Mockito documentation alludes, you'd see that deleteUser and deleteOrganization are independent. By separating those out into two different classes, you could have the OrganizationDeleter accept a mock UserDeleter, eliminating your need for a partial mock.

  This may be an attractive option if you expect the user-deleting business logic to vary, or you want to expect to write another UserDeleter implementation.

• Split the class vertically into MyClass and MyClassService/MyClassHelper.

  If there's enough difference between the low-level infrastructure and high-level calls, you may want to separate them out. MyClass would keep deleteUser and deleteOrganization, perform whatever preparation or validation steps are needed, and then make a few calls to the primitives in MyClassService. deleteUser would probably be a straightforward delegate, whereas deleteOrganization could call the service without invoking its neighbor.

  If you have enough low-level calls to warrant this extra separation, or if MyClass deals with both high-level and low-level concerns, this may be an attractive option—especially if it's a refactor that's occurred to you before. Be careful to avoid the pattern of baklava code, though (too many thin, permeable layers than you could ever keep track of).

• Continue using partial mocks.

  Though this does leak the implementation detail of internal calls, and this does go against the Mockito warning, on balance partial mocks could provide the best pragmatic choice. If you have one method calling its sibling multiple times, it may not be well-defined whether the inner method is a helper or a peer, and a partial mock would save you from having to make that label.

  This may be an attractive option if your code has an expiration date, or is experimental enough not to warrant a full design.

(Side note: It's true that unless MyClass is final it is open to subclassing, which is part of what makes partial mocks possible. If you were to document that deleteOrganization's general contract involved multiple calls to deleteUser, then it would be entirely fair game to create a subclass or partial mock. If it's not documented, then it's an implementation detail, and should be treated as such.)

Answer 2

Does the contract for deleteOrganization() say that all Users in that organization will also be deleted?
If it does then you have to keep at least part of the delete user logic in deleteOrganization() because clients of your class might rely on that feature.

Before I go into the options I will also point out that each of these delete methods is public and the class and methods are both not-final. This will let someone extend the class and override the method which could be dangerous.

Solution 1 - delete organization still has to delete its users

Taking my comment about the overriding dangers into account we pull out the part that actually deletes users from deleteUser(). I assume the public method deleteUser does additional validation and perhaps other business logic that isn't needed by deleteOrganization().

public void deleteOrganization(Organization organization)
{
    /*Delete organization*/

    /*Delete related users*/
    for (User user : organization.getUsers()) {
        privateDeleteUser(user);
    }
}

private void privateDeleteUser(User user){
   //actual delete logic here, without anything delete organization doesn't need
}


public void deleteUser(User user)
{
    //do validation
   privateDeleteUser(user);
  //perform any extra business locic
}

This re-uses the actual code that does the delete and avoids the dangers of subclasses changing deleteUser() to behave differently.

Solution 2 - don't need to delete users inside deleteOrganization() method

If we don't need to delete the users from the organization all the time we can remove that part of the code from deleteOrganization(). In the few places where we need to delete the Users too, we can just do the loop like deleteOrganization has now. Since it users public methods only any client can call them. We can also pull out the logic into a Service class as well.

public void DeleteOrganizationService(){

   private MyClass myClass;
   ...

   public void delete(Organization organization)
   {
       myClass.deleteOrganization(organization);
        /*Delete related users*/
        for (User user : organization.getUsers()) {
            myClass.deleteUser(user);
        }
    }

 }

And here is the updated MyClass

public void deleteOrganization(Organization organization)
{
    /*Delete organization only does not modify users*/
}

public void deleteUser(User user)
{
    /*same as before*/
}