CodeSOD: Just One Check

Christian works on an application that has some unusual conventions. For example, while I've seen many a codebase that Hungarians their private member variables like _myPrivateMember, his team did the opposite: myPrivateMember_.

Someone on the team had a problem: they needed to verify that at least one checkbox was checked. This was their solution to that, in C#:

private void CheckBox_func_member_CheckedChanged(object sender, System.EventArgs e) {if (((PVCheckBox)sender).Checked) {onechecked_ = true;} else {string from = ((CheckBox)sender).Name;if ((from.Equals("MEMBER") && !CheckBox_func.Checked) || (from.Equals("FUNCTION") && !CheckBox_member.Checked)) {onechecked_ = false;}}}

The first thing I notice is that their checkbox type is PVCheckBox. That's not the built-in .NET type, which implies that they extended it (or worse, rolled their own). Curiously, the only other place where PVCheckBox appears on the Internet is in a Java library for algorithmic music. Make of that what you will.

So, we look at the sender, which is the checkbox the user just interacted with. If it's Checked, then we know that at least one checkbox is checked, so voila. We're done.

If it's not checked, we look at the name of the checkbox. If the checkbox that got unchecked is named "MEMBER", we look at the CheckBox_func to see if that's checked. Conversely, if the name of the unchecked box is "FUNCTION", we look at CheckBox_member.

Notably, and another hint at a code smell, in the else clause they cast to CheckBox, implying that PVCheckBox is their own extension.

The logic works, but what a bizarre and hard to extend way to handle validation. There are a half dozen simpler and more obvious ways to solve this problem.

As you can imagine, this is pretty standard throughout the codebase. Their validation would be better if someone did the opposite of what they did here.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!