Branches and Code Quality

A while back I posted about not reformatting code and the problems you incur when merging branches. After some thinking, reading and discussion, I'd like to partially recant that advice.

I still think that moving brackets around in the code is pretty much a waste of time compared to things you could tidy up which make code easier to understand.

But more and more I think that the cost of a merge made a little more difficult is repaid many times by improving the code quality.

Martin Fowler has quite a lot to say about branches.

But in general we read code a lot more often than we write it, so I'm more and more prepared to amortize the cost of fixing poor and mediocre code for the benefit of less confusion, less bugs, more tests.

The Partial Class anti-pattern

In c# you can have a partial class.

This has a very specific function. It allows you to separate "Widget Generated Code" from the rest of your code. Then if you run the widget again, it won't overwrite all the additional code you have added. Think Windows Forms.

Of course when a class gets too large, you could* break up the class into multiple files, and use partial classes.

But this is very much brushing the problem under the rug.

Now you have made things worse.

If you have succeeded in breaking up the class into two or more partial classes which do not access private variables or private functions from the remainder of the partial class, then make it a brand new stand alone class.

If not, you now have a mini program within a large program, where you essentially have Global variables and functions  (albeit marked as private) which can be changed by code from another file. You can no longer see what is going on.

OK, in fairness you can use Resharper or some such tool to "find usages", but really you have just acknowledged the class was too big. Break it up into separate classes.


This 

• makes it more testable.
• reduces the scope of private and protected members. 
• may well separate out dependencies.
• allows you to have higher level code in the primary class and hides the details of "the other stuff".

*if you do this, allow me to suggest that you don't go boasting about it to all your friends.

if (fred)

if (fred) {
...
}

Cryptic?

How much better is

if (preloadCaches()){
}

You would probably guess that this returns false for an error. An exception might be a better way of handling this.


So what about

if (reloadCaches()){
}

Any guesses when this would return true or false? maybe true if this is the first time it's loaded the cache? or perhaps true if anything changed? or perhaps true if there were no errors?


If a function name does not give away the result, don't return a bool.

• bool isValidId() 
• bool isEnabled()
• bool isOverDrawn()

The above are just fine.

• bool setAccountOverdraftLimit()
• bool preInitialisationCheck()
• bool fred()

These all require me to go looking into the function to see what it is all about.


I may be looking for a bug in the account balance, but when I see the code.

if (setAccountOverdraftLimit(10000)){
   ....
}


Then I don't know if I'm going into the if() or not. Now I need to read setAccountOverdraftLimit at the outset. Every time I come across this ambiguity I am forced to read more and more code which in all probability has no relation to the bug.

And wait for the fun that happens when someone overrides setAccountOverdraftLimit() and one instance returns true/false for the limit has been set/not set, and the other instance returns true/false for the limit has been increased compared to the previous limit.

Cut and Paste is not a valid design pattern

You would think in this day and age, that no-one would copy a 200 line function from one class to another.

Because if they did then surely one version would have changes made to it.

Then some poor fool would be left wondering if the two versions were actually slightly different, or if a fix was applied to one, not the other. Or if a change was simply because the functions were called in slightly different ways.

Of course having a 200 line function which looks like 20 x 10 lines blocks cut and paste into a switch statement... that would be a wrong thing in the first place.

And no-one would do that, because if they did, their name would be against the check-in in the source code control system.

Do one thing.

Classes should do one thing.

If they have "other stuff" to do, then they should use other classes.

Embedding the implementation of thread-safe, dictionaries of linked lists in your functions fails on several levels

• Next time you need a "Multimap" you will write it all over again.
• There's a name for this data type, that should be a hint not to write it yourself.
• Your function is now implementing what it's supposed to, and it's implementing MultiMap.Add()
• You have lock primitives in open code.
• Your code is harder to read. You now have many lines of code in your function. These lines could be replaced by Multimap.Add(). That would simplify your code and make it easier to see the main functionality.
• If just above the section of code you have a comment saying "add this to a thread safe multimap" then shame on you, you already knew it should not be there.
• If you have enclosed it in a #region (c#) and collapsed that region to a single line comment, then was this not a big hint that it could be taken out and put in it's own class.
• Your code is now much harder to test. How do you test the MultiMap code which is embedded in your function.
• If you need to update the Dictionary of Lists for performance, it's going to be fun fixing it in many different places.
• And sooner or later someone will read or update your dictionaryOfLists without noticing the dictionaryOfListsLockObject.

I do appreciate that a dictionary of lists is simple, and that you have probably implemented it correctly. In fact you have implemented it so many times by now, you can do it in your sleep. 

Please appreciate that this is not a good thing to boast about.

For further reading google "principle of single responsibility" or look here

http://en.wikipedia.org/wiki/SOLID_(object-oriented_design)