1

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?

Improve this question
17
  • 4
    If the best practice gets in the way of doing the best thing, it isn't a best practice. Test what needs to be tested.
    – user40980
    Oct 27, 2015 at 21:54
  • 2
    Since you do not want to test private methods, make the methods public (or "internal", with "internals visible" to your test class). Then you can still follow the "best practice":
    – Doc Brown
    Oct 27, 2015 at 21:55
  • 1
    Thanks, I want to avoid making the methods public as any consumers don;t need to know about them. Let me clarify my meaning of 'best practice' as 'the most common opinion I've read on the subject'!
    – JMP
    Oct 27, 2015 at 21:58
  • 1
    @jp I follow The way of Testivus. Test what needs to be tested. Sometimes tests are ugly, but an ugly test is better than no test. And the test is more important than the unit. And everything else too.
    – user40980
    Oct 27, 2015 at 22:02
  • 4
    I recommend the second answer for this question. Especially the part "When faced with a choice between encapsulation and testability, I'd rather go for the second." So if you want better tests, you will sometimes have to make something more accessible and less "private" than "good OOP might dictate".
    – Doc Brown
    Oct 28, 2015 at 6:53

1 Answer 1

Reset to default
5

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.

Improve this answer
5
  • the functions that do not take arguments and do not return anything are the root cause of the code smell and a major design flaw
    – user7519
    Nov 14, 2015 at 2:29
  • @MichaelT These sound like good approaches. I knew that my implementation wasn't right for the sequence situation. I'll experiment with this. I've wrapped the method call sequence in a using TransactionScope so it is one transaction. Not quite sure what you mean by 'If this is part of a transaction, expose the transaction.' though, could you elaborate a bit on that? Thanks
    – JMP
    Nov 15, 2015 at 19:15
  • @jp you are wrapping it with a transaction, but what if I want to delete two items (or none and roll back) as part of the same transaction? What if I want a variation on that set of events? Can you trust your users to be sensible programmers? or are you going to try to hide all the complexity that exists behind roll up methods... and when that rollup method doesn't work, then you've got unfixable problems?
    – user40980
    Nov 15, 2015 at 20:26
  • @MichaelT which has got me thinking it could do with a generic sequence running class that controls the transaction and is flexible in the methods it can accept. Also, as you mention, need to have a way to check that any prereqs for the next method, whatever it might be, have successfully completed before it runs.
    – JMP
    Nov 19, 2015 at 9:38
  • @jp its one of those areas that has me thinking too. I've hinted at it in the past with this answer and this answer which work based on adding a state machine to the type system of the Builder pattern. I've even played with it in chat to make a Builder that can only build binary numbers which are evenly divisible by 3. If you are interested in following that path, please consider asking a new question.
    – user40980
    Nov 19, 2015 at 14:00

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