Forums
We’ve discussed the evil of the for-case pattern in the past, but Russell F offers up a finding which is an entirely new riff on this terrible, terrible idea.
We’re going to do this is chunks, because it’s a lot of code.
protected void setDivColor()
{
List<HtmlControl> Divs = new List<HtmlControl>();
HtmlControl divTire1Det = divShopCart.FindControl("divTire1Det") as HtmlControl;
if (divTire1Det.Visible)
{
Divs.Add(divTire1Det);
}
HtmlControl divTire2Det = divShopCart.FindControl("divTire2Det") as HtmlControl;
if (divTire2Det.Visible)
{
Divs.Add(divTire2Det);
}
…So, this method starts with a block of C# code which tries to find different elements in the HTML DOM, and if they’re currently visible, adds them to a list. How many elements is it finding? If I counted correctly (I didn’t count correctly), I see 13 total. Many of them are guarded by additional if statements:
if (!bMAlignAdded)
{
HtmlControl divManufAlign_Add = divShopCart.FindControl("divManufAlign_Add") as HtmlControl;
if (divManufAlign_Add.Visible)
{
Divs.Add(divManufAlign_Add);
}
}
if (!bRoadHazardAdded)
{
HtmlControl divRoadHazard_Add = divShopCart.FindControl("divRoadHazard_Add") as HtmlControl;
if (divRoadHazard_Add.Visible)
{
Divs.Add(divRoadHazard_Add);
}
}That’s the first 80 lines of this method- variations on this pattern. And at the end of it, we have this shiny list of divs. The method is called setDivColor, so you know what we’ve got to do: change the UI presentation by flipping some CSS classes around in a for loop, but also, in a terrible way.
int j = 0;
foreach (HtmlControl dDiv in Divs)
{
if (dDiv.ClientID.Contains("divTire1Det"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divTire2Det"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divDownloadRebate"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divTire1WheelBal"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divTire2WheelBal"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divStudding"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divFWheelAlign"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divTireDisp"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divTireFee"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divSalesTax"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divManufAlign_Remove") && bMAlignAdded)
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divRoadHazard_Remove") && bRoadHazardAdded)
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divManufAlign_Add") && !bMAlignAdded)
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divRoadHazard_Add") && !bRoadHazardAdded)
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divDiscount") && !bRoadHazardAdded)
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
j++;
}We iterate across every element in the list, which we just selected those elements, mind you, and then on alternating rows, swap between GreyBackground and WhiteBackground, giving us a nice contrast between rows of divs. It’s not enough, however, to just do that, we need to have this bizarre addition of a chain of if/else ifs that have it behave like a for-switch, but none of those cases are actually necessary, since we’ve already built the list in the previous block.
Maybe I’ve been writing too much of my own bad code lately, but I think I understand what happened here. At one point, instead of using FindControl, this code just tried to get a list of all the children and iterated across it, and thus the for-switch section was born. But the DOM changed, or maybe that never worked, and thus the developer went back and added the top block where they FindControl each element they need, but never updated the for loop to simplify the logic.
As a bonus WTF, and I recognize that I risk starting a flamewar with this: but CSS classnames should never explicitly reference the UI effect they have (GreyBackground and WhiteBackground) and instead the logical classification of the element (form-row, form-row-alt, for example). There are CSS frameworks which disagree with me on this, but they are wrong.
Full code:
protected void setDivColor()
{
List<HtmlControl> Divs = new List<HtmlControl>();
HtmlControl divTire1Det = divShopCart.FindControl("divTire1Det") as HtmlControl;
if (divTire1Det.Visible)
{
Divs.Add(divTire1Det);
}
HtmlControl divTire2Det = divShopCart.FindControl("divTire2Det") as HtmlControl;
if (divTire2Det.Visible)
{
Divs.Add(divTire2Det);
}
HtmlControl divTire1WheelBal = divShopCart.FindControl("divTire1WheelBal") as HtmlControl;
if (divTire1WheelBal.Visible)
{
Divs.Add(divTire1WheelBal);
}
HtmlControl divTire2WheelBal = divShopCart.FindControl("divTire2WheelBal") as HtmlControl;
if (divTire2WheelBal.Visible)
{
Divs.Add(divTire2WheelBal);
}
HtmlControl divStudding = divShopCart.FindControl("divStudding") as HtmlControl;
if (divStudding.Visible)
{
Divs.Add(divStudding);
}
HtmlControl divFWheelAlign = divShopCart.FindControl("divFWheelAlign") as HtmlControl;
if (divFWheelAlign.Visible)
{
Divs.Add(divFWheelAlign);
}
if (bMAlignAdded)
{
HtmlControl divManufAlign_Remove = divShopCart.FindControl("divManufAlign_Remove") as HtmlControl;
if (divManufAlign_Remove.Visible)
{
Divs.Add(divManufAlign_Remove);
}
}
if (bRoadHazardAdded)
{
HtmlControl divRoadHazard_Remove = divShopCart.FindControl("divRoadHazard_Remove") as HtmlControl;
if (divRoadHazard_Remove.Visible)
{
Divs.Add(divRoadHazard_Remove);
}
}
HtmlControl divTireDisp = divShopCart.FindControl("divTireDisp") as HtmlControl;
if (divTireDisp.Visible)
{
Divs.Add(divTireDisp);
}
HtmlControl divTireFee = divShopCart.FindControl("divTireFee") as HtmlControl;
if (divTireFee.Visible)
{
Divs.Add(divTireFee);
}
HtmlControl divSalesTax = divShopCart.FindControl("divSalesTax") as HtmlControl;
if (divSalesTax.Visible)
{
Divs.Add(divSalesTax);
}
if (!bMAlignAdded)
{
HtmlControl divManufAlign_Add = divShopCart.FindControl("divManufAlign_Add") as HtmlControl;
if (divManufAlign_Add.Visible)
{
Divs.Add(divManufAlign_Add);
}
}
if (!bRoadHazardAdded)
{
HtmlControl divRoadHazard_Add = divShopCart.FindControl("divRoadHazard_Add") as HtmlControl;
if (divRoadHazard_Add.Visible)
{
Divs.Add(divRoadHazard_Add);
}
}
HtmlControl divDiscount = divShopCart.FindControl("divDiscount") as HtmlControl;
if (divDiscount.Visible)
{
Divs.Add(divDiscount);
}
int j = 0;
foreach (HtmlControl dDiv in Divs)
{
if (dDiv.ClientID.Contains("divTire1Det"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divTire2Det"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divDownloadRebate"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divTire1WheelBal"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divTire2WheelBal"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divStudding"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divFWheelAlign"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divTireDisp"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divTireFee"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divSalesTax"))
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divManufAlign_Remove") && bMAlignAdded)
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divRoadHazard_Remove") && bRoadHazardAdded)
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divManufAlign_Add") && !bMAlignAdded)
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divRoadHazard_Add") && !bRoadHazardAdded)
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
else if (dDiv.ClientID.Contains("divDiscount") && !bRoadHazardAdded)
{
Divs[j].Attributes["class"] = (j % 2 == 0 ? "GreyBackground" : "WhiteBackground");
}
j++;
}
}
[Advertisement]
ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.