Design issues, tendency to write custom methods

Question

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<Label> inputLabels, 
    ComboBox cbSProcList, List<TextBox> inputTxtboxes, 
    List<TextBox> selectedSProcInputs, 
    List<String> selectedSprocDataTypes, List<string> 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.

Answer 1

Just a few small notes:

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

2. 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

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

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

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

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