CodeSOD: On the Border of Badness

The WebForms APIs in ASP .Net tried to make web programming look like native programming- you designed pages in a WYSIWYG editor, and bound event handlers to controls, and pretended like the request/response cycle didn't exist. It was a bit of a mess.

One of the ways it was messy was that you now had two different approaches to styling elements in your UI. You could go the route of using CSS, like a normal web page. But every web Control object also exposed a bunch of properties that you could access directly from your server-side code, allowing you to do things like myControl.BorderColor = Color.Red.

All that's fine and good, or fine and bad, if we're being honest, but it brings us to Leandro's submission. While trawling through a legacy code base, this little snippet leap out:

protected void BorderColourSet(Control ctrl, Color col){switch (ctrl.GetType().Name.ToString()){case "TextBox":((TextBox)ctrl).BorderColor = col;break;case "RadioButtonList":((RadioButtonList)ctrl).BorderColor = col;break;case "DropDownList" :((DropDownList)ctrl).BorderColor = col;break;case "CheckBox" :((CheckBox)ctrl).BorderColor = col;break;}}

Someone didn't understand how inheritance and polymorphism worked. BorderColor is a property of Control, and gets inherited down into all other controls. So, if nothing else, the extra casts in each branch are unnecessary.

Now, there's a world where this function is doing validation, by ensuring that you are only setting the border color on these specific controls- perhaps they don't want the border color to be changed on buttons, for example? If you pass in anything not in this list, the function does nothing. Even then, I'm sure there are more elegant ways to express that restriction.

Elegant ways which are irrelevant- they were not attempting to express a restriction here. What brought Leandro to this snippet was tracking down a bug where buttons weren't getting their border color set properly. It seems that the original developer wrote this without thinking about buttons.

And once you realize that this was only meant to be a convenience function wrapping the BorderColor property, you have the second realization that- this function is useless. ctrl.BorderColor = col would have accomplished the same thing, more simply and clearly- which is what Leandro refactored the code to do.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!