CodeSOD: Gridding My Teeth

Dan‘s co-workers like passing around TDWTF stories, mostly because seeing code worse than what they’re writing makes them feel less bad about how often they end up hacking things together.

One day, a co-worker told Dan: “Hey, I think I found something for that website with the bad code stories!”

Dan’s heart sank. He didn’t really want to shame any of his co-workers. Fortunately, the source-control history put the blame squarely on someone who didn’t work there any more, so he felt better about submitting it.

This is another ASP .Net page, and this one made heavy use of GridView elements. GridView controls applied the logic of UI controls to generating a table. They had a page which contained six of these controls, defined like this:

<asp:GridView ID="gvTaskMonth1" runat="server" CssClass="leadsGridView" AutoGenerateColumns="False" OnRowDataBound="gvTaskMonth1_RowDataBound"> ... </asp:GridView>

<asp:GridView ID="gvTaskMonth2" runat="server" CssClass="leadsGridView" AutoGenerateColumns="False" OnRowDataBound="gvTaskMonth1_RowDataBound"> ... </asp:GridView>

<asp:GridView ID="gvTaskMonth3" runat="server" CssClass="leadsGridView" AutoGenerateColumns="False" OnRowDataBound="gvTaskMonth1_RowDataBound"> ... </asp:GridView>

The purpose of this screen was to display a roadmap of coming tasks, broken up by how many months in the future they were. The first thing that leaps out to me is that they all use the same event handler for binding data to the table, which isn’t in-and-of-itself a problem, but the naming of it is certainly a recipe for confusion.

Now, to bind these controls to the data, there needed to be some code in the code-behind of this view which handled that. That’s where the WTF lurks:

/// <summary>
/// Create a roadmap for the selected client
/// </summary>

private void CreateRoadmap()
{
	for (int i = 1; i < 7; i++)
	{
		switch (i)
		{
			case 1:
				if (gvTaskMonth1.Rows.Count > 0)
				{
					InsertTasks(gvTaskMonth1, DateTime.Parse(txtDatePeriod1.Text), "1");
				}
				break;
			case 2:
				if (gvTaskMonth2.Rows.Count > 0)
				{
					InsertTasks(gvTaskMonth2, DateTime.Parse(txtDatePeriod2.Text), "2");
				}
				break;
			case 3:
				if (gvTaskMonth3.Rows.Count > 0)
				{
					InsertTasks(gvTaskMonth3, DateTime.Parse(txtDatePeriod3.Text), "3");
				}
				break;
			case 4:
				if (gvTaskMonth4.Rows.Count > 0)
				{
					InsertTasks(gvTaskMonth4, DateTime.Parse(txtDatePeriod4.Text), "4");
				}
				break;
			case 5:
				if (gvTaskMonth5.Rows.Count > 0)
				{
					InsertTasks(gvTaskMonth5, DateTime.Parse(txtDatePeriod5.Text), "5");
				}
				break;
			case 6:
				if (gvTaskMonth6.Rows.Count > 0)
				{
					InsertTasks(gvTaskMonth6, DateTime.Parse(txtDatePeriod6.Text), "6");
				}
				break;
		}
	}
}

Ah, the good old fashioned loop-switch sequence anti-pattern. I understand the motivation: “I want to do the same thing for six different controls, so I should use a loop to not repeat myself,” but then couldn’t quite figure out how to do that, so they just repeated themselves, but inside of a loop.

The “fix” was to replace all of this with something more compact:

	private void CreateRoadmap()
	{
		InsertTasks(gvTaskMonth1, DateTime.Parse(txtDatePeriod1.Text), "1");
		InsertTasks(gvTaskMonth2, DateTime.Parse(txtDatePeriod2.Text), "2");
		InsertTasks(gvTaskMonth3, DateTime.Parse(txtDatePeriod3.Text), "3");
		InsertTasks(gvTaskMonth4, DateTime.Parse(txtDatePeriod4.Text), "4");
		InsertTasks(gvTaskMonth5, DateTime.Parse(txtDatePeriod5.Text), "5");
		InsertTasks(gvTaskMonth6, DateTime.Parse(txtDatePeriod6.Text), "6"); 
	}

That said, I’d recommend not trying to parse date times inside of a text box inside of this method, but that’s just me. Bubbling up the inevitable FormatException that this will generate is going to be a giant nuisance. It’s likely that they’ve got a validator somewhere, so it’s probably fine- I just don’t like it.

[Advertisement]
Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.

Source: Read More