I often get to play the maintenance role in my coding life, and I actually enjoy it, as I get to see how others tackle certain problems. I also like to challenge myself by fixing bugs with the smallest footprint possible. In doing so, I find a lot of common things that bring down the 'maintainability' of a class. I thought I'd share some pain that I encountered today.
All too often I find code that roughly translates to this example:
internal class MyForm : System.Windows.Forms.Form { MyClass _myClass; public MyForm() { _myClass = new MyClass(); _myClass.MyPropertyChanged += DodgySideEffectEventDrivenCode; _myClass.MyProperty = new object(); } internal void DodgySideEffectEventDrivenCode(Object sender, EventArgs e) { //Lets do something that requires, ooh I dunno....SQL access } } I won't go into all the other reasons why this could be seen as bad code, but for the purpose of the example, changing DodgySideEffectEventDrivenCode to look like this can save a whole lotta WSOD's… internal void DodgySideEffectEventDrivenCode(Object sender, EventArgs e) { if (this.DesignMode) return; //Lets do something that requires SQL access, but only if we aren't in design mode }
The following is an example of a useless property. There are precious few situations where this would be beneficial to a class.
private static CultureInfo CC { get { return CultureInfo.CurrentCulture; } }
private readonly static CultureInfo CC = CultureInfo.CurrentCulture;
Personally, I think the proof is in the IL pudding…
.field private static initonly class [mscorlib]System.Globalization.CultureInfo CC
Compare that to the IL of a readonly property get method:
Sometimes coding isn't enough; sometimes you need to express your intent explicitly. Take the following function declaration which clearly does not:
internal void SetUIState(bool value)
and its usage:
SetUIState(true);
Can you tell how this function will work? Personally I like comments and documentation – and they would help, but self documenting code is worth a page of English documentation. For example:
internal enum UIState{ enabled, disabled, } internal void SetUIState(UIState state)
Combined with a decent IDE, the usage and effect becomes apparent:
SetUIState(UIState.enabled);
An even simpler approach would be to just name the function to imply the usage of the state parameter in the first place. I.E
EnableUI(true);
Personal choice comes into this a lot, but I think we can all agree that it's all about taking care with naming.
On that….
If I ever see another TextBox control named 'TextBox1' I'm going to have to hurt somebody. Seriously though, if you have labels and other miscellaneous controls laying about that you don't need to name because you never reference them in code, and you are using VS 2005 or greater then consider the following approach:
This will keep the controls initialization local to the InitializeComponents routine, thus reducing the clutter in Intellisense and other areas in the IDE quite significantly.
For the rest of your controls and objects, pick a naming convention, and stick to it like glue.
Subscribe