Today, I'm just going to share some of the funny code that I have come across over the years. With most of these, I still fail to wrap my mind around the logic of how the code got into the state that it is in. In many cases, I asked the original author how they thought the code would work and I often received responses like “It just does” or “I didn't write that” (even in cases where that person had just checked the code into source control earlier that day).
What’s a For Loop?
int i = 1;
foreach (Features f in PageFeatures)
{
if (i == 4)
{ break; }
i++;
}
I actually spent time looking at this code for several minutes trying to see if I was missing something. Disregard the fact that the object class name is plural for something that is a singular object, that’s a topic for another day. Just look at this loop. The fact that it is not using a standard “for” loop with an iterator threw me off big time. There just had to be a reason that this was done this way. After what seemed like an eternity, I gave up and asked the developer. His response – he didn't know what a standard “for” loop was. He had only ever used the “foreach” construct in C#. He learned to program exclusively with C#, so I didn't even bother asking him about other languages. I could understand (to an extent) someone new to C# and new to programming not knowing about a standard “for” loop since very few samples online show anything but looping through all objects in a collection. But this was a developer that had supposedly received a bachelor’s degree in computer science, and had been working at the same place as me for nearly two years. At this point in the conversation, I just had to walk away. I honestly could think of nothing to say in response to that. Well, nothing remotely nice, at least.
Um, Right…
string strStateID = "";
if (strStateID == "")
{
ddlState.Items.Insert(0, "Select");
ddlState.SelectedIndex = 0;
ddlState.SelectedItem.Value = "-1";
}
else
{
ddlState.Items.Insert(0, "");
ddlState.SelectedItem.Value = "-1";
ddlState.Items.Insert(0, strStateID);
ddlState.SelectedIndex = 0;
ddlState.SelectedItem.Value = "-1";
}
This is another example of code where I just sat there staring at it, thinking I must be missing something. But no, I wasn't. This sample of code really can be boiled down a single line of code, if you know what you are doing. While the code inside the “if” statement technically works the way it is intended, it is quite simple to replace this with just a line adding a new ListItem object to the collection. That “else” statement, however? To this day, I still can't figure out what the intention is in there. Good thing it didn't matter since there was absolutely no way that the code would ever run as written.
The funniest part of this code was when the developer who wrote it swore that it actually got into the “else” and did something different when the circumstances were right. He couldn't tell me what those circumstances were, but he swore that it happened.
Imagine the Query Plan on This One
SELECT distinct mm.EMail, mr.AccountNumber
FROM Member_Master AS mm
INNER JOIN Member_Extension AS me ON me.MemberID = mm.MemberID
INNER JOIN Member_Roles AS mr ON mm.MemberID = mr.MemberID
WHERE
(
mm.EMail IN
(SELECT Email
FROM HCKMagSubs)
)
AND (not mr.AccountNumber is null)
AND (mm.FirstName IS NULL
OR mm.FirstName = '')
OR
(mm.EMail IN
(
SELECT Email
FROM HCKMagSubs AS HCKMagSubs_4
)
)
AND (mm.LastName IS NULL)
OR
(mm.EMail IN
(
SELECT Email
FROM HCKMagSubs AS HCKMagSubs_3
)
)
AND (mm.LastName = '')
OR
(mm.EMail IN
(
SELECT Email
FROM HCKMagSubs AS HCKMagSubs_2
)
)
AND (me.Address1 IS NULL)
OR
(mm.EMail IN
(
SELECT Email
FROM HCKMagSubs AS HCKMagSubs_1
)
)
AND (me.Address1 = '')
We were consistently getting timeouts on our production SQL server and I was determined to find out why. Turns out that a developer was running this ad-hoc query every day. And due to the size of the tables (all tables referenced were in the 100’s of thousands, minimum), the query took about two hours to run. Oh, and the largest table in the bunch was HCKMagSubs, which had over a million records, being joined to across all rows 5 times. He said it had to be run every Monday, during the day because of a request from a business person (they needed to know all subscriber records that were missing data, and they needed to know now) and that there was no way to speed it up. I spent about 10 minutes and came up with this:
DECLARE @table TABLE(MemberID INT)
INSERT INTO @table
SELECT mm.MemberID
FROM HCKMagSubs hm
INNER JOIN Member_Master AS mm
ON hm.Email = mm.Email
SELECT distinct mm.EMail, mr.AccountNumber
FROM Member_Master AS mm
INNER JOIN @table t
ON t.MemberID = mm.MemberID
INNER JOIN Member_Extension AS me ON me.MemberID = mm.MemberID
INNER JOIN Member_Roles AS mr ON mm.MemberID = mr.MemberID
WHERE
mr.AccountNumber is NOT null AND (mm.FirstName IS NULL OR mm.FirstName = '')
OR
(mm.LastName IS NULL)
OR
(mm.LastName = '')
OR
(me.Address1 IS NULL)
OR
(me.Address1 = '')
The new query, even with the number of records, came back in seconds, not hours. The developer in question wasn't really happy that I did this, because it made him look really bad, but honestly, a little common sense needed to be knocked into him if he thought it was okay to take down our sites for a couple of hours every week for this one internal query request.
If You Can Comprehend This, You Deserve a Medal
And now, I'm going to leave you with an incredibly convoluted mess. I didn't even bother asking what this was supposed to do. I just selected the whole thing, hit “Delete” and started new. I can't remember, but I think I was able to get the code working correctly (this wasn't right anyways, the only reason I even attempted to look at it) in about 5 lines of code.
if (strHeader != "" | strBooleans != "")
{
if (strHeader == "")
{
strBooleans = strBooleans.Substring(4);
}
if ((strKeyWord == "" & strNotKeyWord == "") & strCats == "")
{
Trace.Write("Spot1");
dtt = crlRecipeList.LoadSearch(ref gstrParams, ref gstrSort,
"", "", "", strHeader, "", "", "", "", strBooleans, "",
PullSort(strSort), intSearchType, connRecipe);
}
else if ((strKeyWord == "" & strNotKeyWord == "") & strCats != "")
{
Trace.Write("Spot2");
dtt = crlRecipeList.LoadSearch(ref gstrParams, ref gstrSort,
"", strCats, "", strHeader, "", "", "", "", strBooleans, "",
PullSort(strSort), intSearchType, connRecipe);
}
else if ((strKeyWord != "" | strNotKeyWord != "") & strCats == "")
{
dtt = crlRecipeList.LoadSearch(ref gstrParams, ref gstrSort, "",
"", "", strHeader, "", "", strKeyWord, strNotKeyWord, strBooleans,
"", strForLog, PullSort(strSort), intSearchType, connRecipe, intLog);
}
else if ((strKeyWord != "" | strNotKeyWord != "") & strCats != "")
{
Trace.Write("Spot4");
dtt = crlRecipeList.LoadSearch(ref gstrParams, ref gstrSort,
"", strCats, "", strHeader, "", "", strKeyWord, strNotKeyWord,
strBooleans, "", strForLog, PullSort(strSort), intSearchType,
connRecipe, intLog);
}
}
else
{
if ((strKeyWord == "" & strNotKeyWord == "") & strCats != "")
{
dtt = crlRecipeList.LoadSearch(ref gstrParams, ref gstrSort,
"", strCats, "", "", "", "", "", "", "", "", strForLog,
PullSort(strSort), intSearchType, connRecipe, intLog);
}
else if ((strKeyWord != "" | strNotKeyWord != "") & strCats == "")
{
dtt = crlRecipeList.LoadSearch(ref gstrParams, ref gstrSort,
"", "", "", "", "", "", strKeyWord, strNotKeyWord, "", "",
strForLog, PullSort(strSort), intSearchType, connRecipe, intLog);
}
else if ((strKeyWord != "" | strNotKeyWord != "") & strCats != "")
{
dtt = crlRecipeList.LoadSearch(ref gstrParams, ref gstrSort,
"", strCats, "", "", "", "", strKeyWord, strNotKeyWord, "", "",
strForLog, PullSort(strSort), intSearchType, connRecipe, intLog);
}
}
