CodeSOD: Blocked Up

Agatha has inherited some Windows Forms code. This particular batch of such code falls into that delightful category of code that's wrong in multiple ways, multiple times. The task here is to disable a few panels worth of controls, based on a condition. Or, since this is in Spanish, "bloquear controles". Let's see how they did it.

private void BloquearControles(){bool bolBloquear = SomeConditionTM; // SomeConditionTM = a bunch of stuff. Replaced for clarity.// Some code. Removed for clarity.// private System.Windows.Forms.Panel pnlPrincipal;foreach (Control C in this.pnlPrincipal.Controls){if (C.GetType() == typeof(System.Windows.Forms.TextBox)){C.Enabled = bolBloquear;}if (C.GetType() == typeof(System.Windows.Forms.ComboBox)){C.Enabled = bolBloquear;}if (C.GetType() == typeof(System.Windows.Forms.CheckBox)){C.Enabled = bolBloquear;}if (C.GetType() == typeof(System.Windows.Forms.DateTimePicker)){C.Enabled = bolBloquear;}if (C.GetType() == typeof(System.Windows.Forms.NumericUpDown)){C.Enabled = bolBloquear;}}// private System.Windows.Forms.GroupBox grpProveedor;foreach (Control C1 in this.grpProveedor.Controls){if (C1.GetType() == typeof(System.Windows.Forms.TextBox)){C1.Enabled = bolBloquear;}if (C1.GetType() == typeof(System.Windows.Forms.ComboBox)){C1.Enabled = bolBloquear;}if (C1.GetType() == typeof(System.Windows.Forms.CheckBox)){C1.Enabled = bolBloquear;}if (C1.GetType() == typeof(System.Windows.Forms.DateTimePicker)){C1.Enabled = bolBloquear;}if (C1.GetType() == typeof(System.Windows.Forms.NumericUpDown)){C1.Enabled = bolBloquear;}}// private System.Windows.Forms.GroupBox grpDescuentoGeneral;foreach (Control C2 in this.grpDescuentoGeneral.Controls){if (C2.GetType() == typeof(System.Windows.Forms.TextBox)){C2.Enabled = bolBloquear;}if (C2.GetType() == typeof(System.Windows.Forms.ComboBox)){C2.Enabled = bolBloquear;}if (C2.GetType() == typeof(System.Windows.Forms.CheckBox)){C2.Enabled = bolBloquear;}if (C2.GetType() == typeof(System.Windows.Forms.DateTimePicker)){C2.Enabled = bolBloquear;}if (C2.GetType() == typeof(System.Windows.Forms.NumericUpDown)){C2.Enabled = bolBloquear;}}// Some more code. Removed for clarity.}

This manages two group boxes and a panel. It checks a condition, then iterates across every control beneath it, and sets their enabled property on the control. In order to do this, it checks the type of the control for some reason.

Now, a few things: every control inherits from the base Control class, which has an Enabled property, so we're not doing this check to make sure the property exists. And every built-in container control automatically passes its enabled/disabled state to its child controls. So there's a four line version of this function where we just set the enabled property on each container.

This leaves us with two possible explanations. The first, and most likely, is that the developer responsible just didn't understand how these controls worked, and how inheritance worked, and wrote this abomination as an expression of that ignorance. This is extremely plausible, extremely likely, and honestly, our best case scenario.

Because our worse case scenario is that this code's job isn't to disable all of the controls. The reason they're doing type checking is that there are some controls used in these containers that don't match the types listed. The purpose of this code, then, is to disable some of the controls, leaving others enabled. Doing this by type would be a terrible way to manage that, and is endlessly confusing. Worse, I can't imagine how this behavior is interpreted by the end users; the enabling/disabling of controls following no intuitive pattern, just filtered based on the kind of control in use.

The good news is that Agatha can point us towards the first option. She adds:

They decided to not only disable the child controls one by one but to check their type and only disable those five types, some of which aren't event present in the containers. And to make sure this was WTF-worthy the didn't even bother to use else-if so every type is checked for every child control

She also adds:

At this point I'm not going to bother commenting on the use of GetType() == typeof() instead of is to do the type checking.

Bad news, Agatha: you did bother commenting. And even if you didn't, don't worry, someone would have.

[Advertisement] Picking up NuGet is easy. Getting good at it takes time. Download our guide to learn the best practice of NuGet for the Enterprise.