This is the story of a developer who was asked to write a text transformation code and test cover it.
This is the unit test:
And test passing with code coverage here:
You can see that the task was fairly simple. You need two functions
makeUnimportant that in a few steps turn a string into a different string. The steps within the code are commented and the unit test uses a concise method of testing all these steps. The code appears short enough so you would not get alarmed by it and feel like it might be a good example of well-done KISS (keep it simple stupid) code.
Now let’s list all the problems.
There is an issue with the state of the object
bug!. contains extra exclamation mark that got there from the previous
Manipulating states and working with side effects are dangerous and hard to test or debug. If it is someone else using that code, they would see there is a character in the output that does not belong, so they would go investigate the method but nothing would appear wrong with its code either. Once you figure out what the root cause is, the decision how and where to fix it is actually not that simple either.
Quick fix: The developer should not modify the state and instead use a local variable and transform that on every step. In the end, the local variable is returned and internal object variable is not influenced. However, that does not mean that some code somewhere else is not going to modify your state. Also, you are likely creating a state with the intention to modify it in the first place.
Unexpected side effects are hard to manage because they are unexpected, the only real solution is to create a code less prone to side effects. You can also look at my other post with 3 reasons why not to focus on code coverage as your key performance indicator.
It’s not hard to see that the developer likely started the development by writing the
makeUnimportant was created by copy-pasting and editing the previous code. And as the last step, unit tests were written.
Instead of creating separate methods for reusability the code was copied over.
Quick fix: The developer should use a separate method for the logic shared among multiple methods. It could also be argued that that method might live in a different class to also allow for reusability by multiple methods in different classes. How you follow DRY (don’t repeat yourself) principle depends a lot on personal development style. Are you writing your code and then if you notice repetition you consider refactoring? Are you intentionally writing atomic functions trying to proactively increase reusability even if those functions may never be reused?
Single responsibility and black box issue
Breaking the single responsibility principle is visible when we try to test the method. To test it, we devise example input that would demonstrate for us every step of the transformation as
makeImportant: pReSiDeNt -> President!. In reality, the developer would often just create some simple example only to prove that some transformation happened because testing a complex method is difficult.
In fact, if writing a test feels difficult, it might be a code smell that the code you are testing is badly written.
Additionally, if one of the steps inside of the method breaks, then we get only a general test fail and we will have to dig in to see what actually went wrong. You end up wasting time debugging instead of writing new features.
Quick fix: The developer could follow defensive programming practices and actually create separate tests demonstrating the behaviour of every line of the code as it is being written. Also you can apply KISS principle by separating your code into independent atomic methods each with own unit test.
With this unit test:
And the test coverage result:
Notice that we are using static methods in the class to create methods dependent only on their inputs without any side-effects making the behaviour easily testable. All the new code then gets tested in our unit tests.
We can of course still decide to not use static methods and rather depend on the state to allow for intentional chaining such as:
new MyObject('value').change1().change2(); but you always have to be sure that you test all the permutations of chaining. For example, if you have 3 methods, that gives you 12 options.
With this unit test:
And this test coverage result:
Notice that the unit tests for both OOP with static methods and functional solution are the same and everything is called over the same API. The only difference is in the import method.
However, there is a major difference in code size, as the OOP solution needs 27 lines of code and the functional solution only 8 (actually only 5 if you don’t count the helper functions for
toLowerCase for which you could use a third-party library like lodash).
As was demonstrated, the testable and well-tested code is not defined by just passing tests and code coverage reports. Also, you can see that writing better testable code is not about writing more code or adding work for yourself. Rather, a testable tested code will make the work for you significantly easier.
Check out also my post about QA tool stack.