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?

JMP
  • 127
  • 1
  • 2
  • 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. –  Oct 27 '15 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 '15 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 '15 at 21:58
  • 1
    @jp I follow [The way of Testivus](http://www.agitar.com/downloads/TheWayOfTestivus.pdf). 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. –  Oct 27 '15 at 22:02
  • @MichaelT that's a good read. – JMP Oct 27 '15 at 22:10
  • These are not all specific to C#, but there are [many similar questions a search away](http://programmers.stackexchange.com/search?q=test+private+method). –  Oct 27 '15 at 22:13
  • @jp you may also find [the codeless code](http://thecodelesscode.com/topics/testing) enlightening. –  Oct 27 '15 at 22:16
  • Recommended reading: http://meta.stackexchange.com/a/142354 – Robert Harvey Oct 27 '15 at 22:16
  • In what way is it hard to unit test? I'm guessing it's because eg `GetPartstoReject()` performs hard-coded actions, or at least actions that are hard to mock. The most likely solution to your testing challenge is to start using dependency injection to inject test end-points into instances of `DoRejection` that are under test. – David Arno Oct 27 '15 at 22:59
  • @DocBrown, internals are implementation details. Attempting to test them leads to brittle tests that are expensive to maintain when those implementation details change. Suggesting exposing them to tests is very poor advice. – David Arno Oct 27 '15 at 23:00
  • @David Arno Thanks David, yes I;d say hard to mock - the first method retrieves some data and the rest are updating it. I'm a beginner at unit testing and as the DoRejection method is just really a skeleton for these methods and none of them return anything I'm struggling to work out what to look for to get a result. I'll look into how I could use dependency injection for this... – JMP Oct 28 '15 at 05:59
  • @DavidArno: yes, the question of testing "private" or "internals" has been asked before here, there are pros and cons on both sided, and sometimes this becomes a flame war. But I think there is often a misconception - the methods which should be "public" to a client of a component are not necessarily the same methods which should be "public" in terms of unit testing. This happens when there are different levels of abstraction involved, and a complex component is constructed of smaller sub-units, each of it worth a unit test on its own. – Doc Brown Oct 28 '15 at 06:42
  • 4
    I recommend the **second** answer for [this](http://programmers.stackexchange.com/questions/100959/how-do-you-unit-test-private-methods) 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 '15 at 06:53
  • @DocBrown, we shall have to disagree here. "When faced with a choice between encapsulation and testability", then the encapsulation is broken. If an assembly cannot be tested via its public API, then it's been built badly and needs redesigning. Resorting to making tests that dive into the internals is a case of fixing the symptoms,; not the cause. – David Arno Oct 28 '15 at 11:21
  • @DavidArno: yes, you can ignore what I wrote about different levels of abstraction completely and go and start a flamewar, if you think this helps the OP. But fwiw: IMHO if an assembly cannot be tested via its public API, then it **may** been built badly and **may** need redesigning. – Doc Brown Oct 28 '15 at 13:09
  • Thanks all, appreciate the suggestions. Sounds like an area of ongoing debate in general! Definitely got some useful information going forward though and I guess the approach must be decided by sizing up the pros and cons based on the situation you are applying it to, have I got time to refactor, does this add complexity etc. I can see both sides of the argument but I'm not informed or experienced enough to side with either as yet. – JMP Oct 28 '15 at 16:26
  • functions that do not take any arguments and do not return anything is your **real** code smell! –  Nov 14 '15 at 02:29

1 Answers1

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.

  • 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 –  Nov 14 '15 at 02: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 '15 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? –  Nov 15 '15 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 '15 at 09:38
  • @jp its one of those areas that has me thinking too. I've hinted at it in the past with [this answer](http://programmers.stackexchange.com/a/289442/40980) and [this answer](http://programmers.stackexchange.com/a/294466/40980) which work based on adding a state machine to the type system of the Builder pattern. I've even played with it [in chat](http://chat.stackexchange.com/transcript/message/24512393#24512393) 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. –  Nov 19 '15 at 14:00