How to structure a chain of method calls [duplicate]

Question

To keep it simpler for any client of my class, I have put a sequence of private method calls within one public method.

The client then calls this method and all the methods within run to complete the action. E.g:

 public void DoRejection(string comments, string rejectionType)
    {
        GetPartstoReject();
        UpdateRejectedPartsStatus(rejectionType);
        RemoveInitiatingRejectionPart();
        ClearRejectedPartsData();
        CreateNewOpenPart();
        SaveRejectionComments(rejectionType, comments);
    }

The downside I can see to this is it is harder to unit test as each private method won't be tested in isolation (following the best practice of only unit testing public methods).

How can this be better designed to enhance testability but still make it simple for a client to call?

Answer 1

You have a problem. You see a symptom of it in the difficulty in testing the methods behind DoRejection(), but that is only a symptom of the problem, not the problem itself.

The underlying problem is that you have a method that hides sequential coupling.

A co-worker of mine (a few employers ago) had a saying:

If you use the word 'and' when describing what the method does, you need to break this method into smaller parts.

Another way of saying this is "You've violated the single responsibility principle for this method." It is doing too many things. The method does not simply "do a rejection", but it rather:

1. updates the part status
2. removes the rejected part
3. clears the rejected part data
4. creates a new part
5. commits the rejection with comments

This are far too many things that are going on behind the scene. If this is part of a transaction, expose the transaction. The reason you are having difficulty testing the parts in isolation is because they are not in isolation. The state of removing the rejected part data requires that the respected part be removed.

A classic approach to this, as part of exposing the methods, is to impose a design by contract. Check the preconditions, and if they are not met, raise an error.

You could also impose the sequence by returning an object that wraps the underlying class and only presents the appropriate next states.

This would be especially useful if you wanted to be able to update multiple parts but only do this as one commit.

There are other problems that creep into this that have corresponding code smells. Methods that take no arguments (and return no values), especially when sequential coupling is in play, are acting on the object state as a pseudo-global value. There are methods that indicate that there are side effects that the next method depends on. This raises some very serious questions about the underlying design and has a very strong code smell to them.

Its hard to say exactly where the problem is from this small sample of code, but it is fairly clear that there is a significant problem with the design foundations upon which this code is based.