10

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?

3
  • 1
    Related: stackoverflow.com/questions/18430915/…
    – jaco0646
    Commented Jul 16, 2015 at 14:20
  • 1
    @ahmehri Not directly related to your question but why do you delete the organization first and then the users? Shouldn't it be the other way around? Delete all the users and then delete the organization. Commented Jul 16, 2015 at 15:35
  • @CKing it's a legacy code that it's implemented that way (related to the business logic used).
    – ahmehri
    Commented Jul 19, 2015 at 13:49

4 Answers 4

4

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.)

3
  • The fact that I was obliged to treat the different conditions needed to pass the deleteUser method while testing something completely independent (deleteOrganization) suggests that my code was hard to test. This is usually an indication that it wasn't well designed. And I don't think that this has anything to do with personal or team style.
    – ahmehri
    Commented Jul 19, 2015 at 14:05
  • Some methods are simply hard to test, especially if they have high standards for the structure of the input given. I'm guessing here, but if it takes a lot of work to create a valid user, it may take 6 times as much to create a valid 5-user organization for black box testing. This suggests creating a testing library or helper method, not necessarily refactoring the system under test. Commented Jul 19, 2015 at 16:19
  • (FWIW the class you're testing may need refactoring, especially if it's mixing high level business logic and low level deletion operations. I just can't conclude that yet myself from what you've shared so far, including the follow-up comment.) Commented Jul 19, 2015 at 16:44
1

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*/
}
0

Don't spy on the class under test, extend it and override deleteUser to do something benign--or at least something under the control of your test.

1
  • Is this really any different from partial mocking? Also, how is a spy not under the control of the test?
    – jaco0646
    Commented Jul 16, 2015 at 17:59
0
Self Use of only override-able method is not proper design pattern.

Solution to above problem
You can eliminate a class’s self-use of overridable methods mechanically, without changing its behavior. Move the body of each overridable method to a private “helper method” and have each overridable method invoke its private helper method.
public class MyClass
{

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

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

    public void deleteUser(User user)
    {
        deleteUserHelper(user);
    }

   private void deleteUserHelper(User user){
       /*delete user*/
   }
}

Now this solution over come self use of override-able method

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.