HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Design issues, tendency to write custom methods

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
issuesdesignwritemethodscustomtendency

Problem

I seem to have this tendency to write custom methods, i.e. code that isn't reusable. The following seems to be one example of trying to do too much in a single method.
But the point I'm not sure about is, in order to split it up, I'd have to redo the for loop each time. Is that acceptable?

Any general pointers on how this should be structured would really be appreciated.

public static void ShowInputs(DataTable dtSProcList, List inputLabels, 
    ComboBox cbSProcList, List inputTxtboxes, 
    List selectedSProcInputs, 
    List selectedSprocDataTypes, List selectedSprocParamterNames)
{
    int counter = 0;

    for (int i = 0; i < dtSProcList.Rows.Count; i++)
    {
        if (dtSProcList.Rows[i][0].ToString() == cbSProcList.SelectedItem.ToString())
        {
            string temp = dtSProcList.Rows[i][0].ToString();
            if (dtSProcList.Rows[i][1] != DBNull.Value)
            {
                inputLabels[counter].Text = dtSProcList.Rows[i][1].ToString().Remove(0, 1);
                inputLabels[counter].Visible = true;
                inputLabels[counter].Enabled = true;
                inputTxtboxes[counter].Visible = true;
                inputTxtboxes[counter].Enabled = true;

                selectedSProcInputs.Add(inputTxtboxes[counter]);
                selectedSprocDataTypes.Add(dtSProcList.Rows[i][2].ToString());

                counter++;
            }
            else
            {
                //Returns control to calling method if the parameter name is null.
                return;
            }
        }
    }
}


EDIT: I had originally posted this on SO and was asked to report it here.

Solution

Just a few small notes:

-
The method has seven parameters which isn't a good smell. Maybe some of them could be class field.

-
First, I'd reverse some conditions and use guard clauses to make the code flatten.

for (int i = 0; i < dtSProcList.Rows.Count; i++)
{
    if (dtSProcList.Rows[i][0].ToString() != cbSProcList.SelectedItem.ToString())
    {
        continue;
    }

    string temp = dtSProcList.Rows[i][0].ToString();
    if (dtSProcList.Rows[i][1] == DBNull.Value)
    {
        //Returns control to calling method if the parameter name is null.
        return;
    }

    inputLabels[counter].Text = dtSProcList.Rows[i][1].ToString().Remove(0, 1);
    inputLabels[counter].Visible = true;
    inputLabels[counter].Enabled = true;
    inputTxtboxes[counter].Visible = true;
    inputTxtboxes[counter].Enabled = true;

    selectedSProcInputs.Add(inputTxtboxes[counter]);
    selectedSprocDataTypes.Add(dtSProcList.Rows[i][2].ToString());

    counter++;
}


References:

  • Replace Nested Conditional with Guard Clauses in Refactoring: Improving the Design of Existing Code;



  • Flattening Arrow Code



-
0, 1 and 2 are magic numbers. Using named constants instead of numbers would make the code more readable and less fragile.

-
What does the counter count? You should give it a name which shows the intent of the variable.

-
dtSProcList.Rows[i] repeats five times in the code. A named local variable could remove this duplication.

-
The temp variable seems to be unused. If it's superfluous remove it.

Code Snippets

for (int i = 0; i < dtSProcList.Rows.Count; i++)
{
    if (dtSProcList.Rows[i][0].ToString() != cbSProcList.SelectedItem.ToString())
    {
        continue;
    }

    string temp = dtSProcList.Rows[i][0].ToString();
    if (dtSProcList.Rows[i][1] == DBNull.Value)
    {
        //Returns control to calling method if the parameter name is null.
        return;
    }

    inputLabels[counter].Text = dtSProcList.Rows[i][1].ToString().Remove(0, 1);
    inputLabels[counter].Visible = true;
    inputLabels[counter].Enabled = true;
    inputTxtboxes[counter].Visible = true;
    inputTxtboxes[counter].Enabled = true;

    selectedSProcInputs.Add(inputTxtboxes[counter]);
    selectedSprocDataTypes.Add(dtSProcList.Rows[i][2].ToString());

    counter++;
}

Context

StackExchange Code Review Q#13125, answer score: 6

Revisions (0)

No revisions yet.