Forums
Wolferitza sends us a large chunk of a C# class. We'll take it in chunks because there's a lot here, but let's start with the obvious problem:
private int iID0;
private int iID1;
private int iID2;
private int iID3;
private int iID4;
private int iID5;
private int iID6;
private int iID7;
private int iID8;
private int iID9;
private int iID10;
private int iID11;
private int iID12;
private int iID13;
If you say, "Maybe they should use an array," you're missing the real problem here: Hungarian notation. But sure, yes, they should probably use arrays. And you might think, "Hey, they should use arrays," would be an easy fix. Not for this developer, who used an ArrayList.
private void Basculer(DataTable dtFrom, DataTable dtTo)
{
ArrayList arrRows = new ArrayList();
int index;
DataRow drNew1 = dtTo.NewRow();
DataRow drNew2 = dtTo.NewRow();
DataRow drNew3 = dtTo.NewRow();
DataRow drNew4 = dtTo.NewRow();
DataRow drNew5 = dtTo.NewRow();
DataRow drNew6 = dtTo.NewRow();
DataRow drNew7 = dtTo.NewRow();
DataRow drNew8 = dtTo.NewRow();
DataRow drNew9 = dtTo.NewRow();
DataRow drNew10 = dtTo.NewRow();
DataRow drNew11 = dtTo.NewRow();
DataRow drNew12 = dtTo.NewRow();
DataRow drNew13 = dtTo.NewRow();
DataRow drNew14 = dtTo.NewRow();
DataRow drNew15 = dtTo.NewRow();
arrRows.Add(drNew1);
arrRows.Add(drNew2);
arrRows.Add(drNew3);
arrRows.Add(drNew4);
arrRows.Add(drNew5);
arrRows.Add(drNew6);
arrRows.Add(drNew7);
arrRows.Add(drNew8);
arrRows.Add(drNew9);
arrRows.Add(drNew10);
arrRows.Add(drNew11);
arrRows.Add(drNew12);
arrRows.Add(drNew13);
arrRows.Add(drNew14);
arrRows.Add(drNew15);
// more to come…
Someone clearly told them, "Hey, you should use an array or an array list", and they said, "Sure." There's just one problem: arrRows is never used again. So they used an ArrayList, but also, they didn't use an ArrayList.
But don't worry, they do use some arrays in just a moment. Don't say I didn't warn you.
if (m_MTTC)
{
if (m_dtAAfficher.Columns.Contains("MTTCRUB" + dr[0].ToString()))
{
arrMappingNames.Add("MTTCRUB" + dr[0].ToString());
arrHeadersTexte.Add(dr[4]);
arrColumnsFormat.Add("");
arrColumnsAlign.Add("1");
Ah, they're splitting up the values in their data table across multiple arrays; the "we have object oriented programming at home" style of building objects.
And that's all the setup. Now we can get into the real WTF here.
if (iCompt == Convert.ToInt16(0))
{
iID0 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(1))
{
iID1 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(2))
{
iID2 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(3))
{
iID3 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(4))
{
iID4 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(5))
{
iID5 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(6))
{
iID6 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(7))
{
iID7 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(8))
{
iID8 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(9))
{
iID9 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(10))
{
iID10 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(11))
{
iID11 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(12))
{
iID12 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(13))
{
iID13 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
}
}
Remember those private iID* values? Here's how they get populated. We check a member variable called iCompt and pull the first value out of a dr variable (a data reader, also a member variable). You may have looked at the method signature and assumed dtFrom and dtTo would be used, but no- they have to purpose in this method at all.
And if you liked what happened in this branch of the if, you'll love the else:
else
{
if (m_dtAAfficher.Columns.Contains("MTTHRUB" + dr[0].ToString()))
{
arrMappingNames.Add("MTTHRUB" + dr[0].ToString());
arrHeadersTexte.Add(dr[4]);
arrColumnsFormat.Add("");
arrColumnsAlign.Add("1");
if (iCompt == Convert.ToInt16(0))
{
iID0 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(1))
{
iID1 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(2))
{
iID2 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(3))
{
iID3 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(4))
{
iID4 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(5))
{
iID5 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(6))
{
iID6 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(7))
{
iID7 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(8))
{
iID8 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(9))
{
iID9 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(10))
{
iID10 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(11))
{
iID11 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(12))
{
iID12 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
else if (iCompt == Convert.ToInt16(13))
{
iID13 = Convert.ToInt32(dr[0]);
iCompt = iCompt + 1;
}
}
}
}
I can only assume that this function is called inside of a loop, though I have to wonder about how that loop exits? Maybe I'm being too generous, this might not be called inside of a loop, and the whole class gets to read 13 IDs out before it's populated. Does iCompt maybe get reset somewhere? No idea.
Honestly, does this even work? Wolferitza didn't tell us what it's actually supposed to do, but found this code because there's a bug in there somewhere that needed to be fixed. To my mind, "basically working" is the worst case scenario- if the code were fundamentally broken, it could just be thrown away. If it mostly works except for some bugs (and terrible maintainability) no boss is going to be willing to throw it away. It'll just fester in there forever.
[Advertisement]
BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!