5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ How exactly are you thinking about splitting the method? Why do you think it would require redoing the loop? Why do you think that would be a problem? \$\endgroup\$ – svick Jun 27 '12 at 14:23
  • \$\begingroup\$ i was originally thinking of having 1 method display the labels, 2nd method display the necessary number of textboxes, 3rd method populate a list/array of parameter names and datatypes (i think thats a bad idea). Someone on SO suggested (kind of anyway) i just write one method to obtain a list of all the necessary rows. then call a second method to display the necessary labels and inputs. Would you agree with that or do u think there is a better way? \$\endgroup\$ – Hans Rudel Jun 27 '12 at 14:40
  • \$\begingroup\$ Judging by your use of the controls, I'd guess this is part of some WinForms code? (as opposed to WPF) \$\endgroup\$ – Jeff Mercado Jun 27 '12 at 15:16
  • \$\begingroup\$ @JeffMercado yeah its winforms, i havent gotten around to wpf yet + sorry for the late reply. \$\endgroup\$ – Hans Rudel Jun 27 '12 at 18:00
5
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.