Extended code style document

[noescape00]

No description provided.

[dangershony]

I would like some more brains on this, yes correct its probably bad practice but what are the alternatives?
Do we endorse the approach to interface out internal logic so it's testable?

@fassadlr @maciejzaleski @ArcSin2000X @Neurosploit what's your thoughts on that?
Also @majikandy input is helpful

[fassadlr]

If we are very strict with sticking to single responsibility throughout the application, we should never run into problems like this where internal logic can't be tested. @dangershony I mostly agree with the change as if a property or method is not called in production code from outside a class it should not be public. However, IF a private method is critical to a task in a component and it returns valuable information we must somehow find a way to test that method individually. It all depends on how big the class etc.

So to me we have to consider these things on a per case basis but yes we shouldn't expose private properties or methods that is not called in production just for testing. If we run into a situation where we need to, then the design is bad or wrong.

[majikandy]

Generally a unit test would be

-Get system into a state
-Perform a single action
-Test the system is in the expected state.

It sounds like in this instance the system would have no detectable state change without looking under the covers, which is slightly odd and suggests maybe the test could be at a higher level of abstraction (or the component decomposed into a group of components).

Pulling out internal logic into it’s own interfaced component only makes sense if it makes sense in terms of single responsibility and dependency inversion, not for the purposes of testing, although that might be a useful by-product.

When you find yourself making things public or even internal with the internals visible trick, it is a sign that something is not quite right.

When you consider that a unit test is the first client of that piece of production code, if it is difficult to test, it is also possibly difficult to reliably use. This situation is quite common when treating methods as the unit under test rather than the class.

Perhaps because the class is doing too much that method level testing was chosen? With smaller more focused classes, unit tests can even cover multiple components and need not be at method or even class level. The mistake here is hard coupling the tests to the class implementation.

So in short, it does make sense to isolate some parts of that internal logic and/or split this class into a group of components that are more loosely coupled (even though the base implementation wires them up the same way as if it were a single component as it is now). Another way of thinking about this is if you can imagine a second implementation of that internal logic (there doesn’t need to actually be one) then it is perfectly valid to depend on that interface and the concrete implementation be wired up to it. If that helps with the test in question, it is a bonus.

In greenfield code, Test Driven Development is the best tool to make this situation not arise at all because you will never (by definition) have the problem of trying to wrap a test around existing code. It will force you the direction of an API-like codebase even internally.

[bokobza]

Agree fully.
Please note that the OP didn't give any particular instructions on how to expose properties or methods that are needed for testing, leaving it to the developer to find creative ways to do so.