One book that I recommend the reading is Clean Code, by Robert Martin. It is a well written book with wonderful techniques to create better code and improve your current programs, so they become easier to read, maintain and understand.
While going through it again, I found an excellent opportunity to improve my skills trying to do some refactoring: in listing 4.7 there is a prime generator function that he uses to show some refactoring concepts and turn int listing 4.8. I then thought do do the same and show my results here.
We can start with the listing converted to C#. This is a very easy task. The original program is written in Java, but converting it to C# is just a matter of one or two small fixes:
The first step is to put in place some tests, so we can be sure that we are not breaking anything while refactoring the code. In the solution, I added a new Class Library project, named it GeneratePrimes.Tests and added the packages NUnit, NUnit3TestAdapter and FluentAssertions to get fluent assertions in a NUnit test project. Then I added these tests:
These tests check that there are no primes for 0 and 1, one prime for 2, the primes for 10 are 2, 3, 5, 7 and that there are 1229 primes less than 10,000 and the largest one is 9973. Once we run the tests, we can see that the pass and we can start doing our changes.
The easiest fix we can do is to revise the comments at the beginning. We don't need the history of Erasthotenes (you can go to Wikipedia for that). We don't need the author and version, thanks to source control technology . We don't need either the initial comment:
Then we can invert the initial test, to reduce nesting. If we hover the mouse in the line of the first if, an arrow appears at the border, indicating a quick fix:
We can do the quick fix, then eliminate the else clause (don't forget to remove the extra comments that are not needed):
Save the code and check that all tests pass. The next step is to rename the variables:
- s can be renamed to sizeOfArray
- f can be renamed as isPrimeArray
Go to the declaration of s and press Ctrl-R-R to rename and rename it tosizeOfArray. Do the same with the f variable. Don't forget to remove the comments (and to run the tests):
To go to the next refactorings, we can use the comments as indicators for extracting methods. We can extract the InitializeArray method:
The extracted code isn't what I expected, so I change it to:
I can use the code like this:
After passing the tests, I can refactor the code of InitializeArray to:
The next step is the sieve:
The code for the sieve is really bad:
It has two out parameters (which, for me, is a code smell), and has an error (the out parameter j must be assigned) before exiting the method. So we can change it to remove the out parameters and remove the sizeOfArray parameter:
Then, we can extract the method to count primes:
CountPrimes has the same flaws as Sieve, so we change it to:
We can refactor it to:
The next step is MovePrimes:
After we tweak the MovePrimes code, we get:
Then we can refactor MovePrimes:
Notice that we aren't using the primes count in this case, so we can remove the calculation of the count and the parameter. After some cleaning and name changing, we get:
Much cleaner, no? Now, it's easier to read the method, the details are hidden, but the code still runs the same way. We have a more maintainable method, and it shows clearly what it does.
But there is a change we can do here: we are using static methods only. We can then use extension methods and add the keyword this to allow the methods to be used as extension methods. For example, if we change MovePrimes and Sieve to:
We can have the GetPrimes method to be changed to:
Cool, no? With this change, the tests become:
The full code is at https://github.com/bsonnino/PrimeNumbers. Each commit there is a phase of the refactoring.