How to avoid repeated code?

Question

I'm still quite new to programming and I noticed that I'm repeating code:

protected void FillTradeSetups()
{
    DBUtil DB = new DBUtil();
    DataTable dtTradeSetups;

    dtTradeSetups = DB.GetTradeSetups();
    ddlSetups.DataValueField = "tradeSetupId";
    ddlSetups.DataSource = dtTradeSetups;
    ddlSetups.DataBind();
}

protected void FillTimeFrames()
{
    DBUtil DB = new DBUtil();
    DataTable dtTimeFrames;

    dtTimeFrames = DB.GetTimeFrames();
    ddlTimeFrames.DataValueField = "tfCode";
    ddlTimeFrames.DataSource = dtTimeFrames;
    ddlTimeFrames.DataBind();
} 

protected void FillTradeGrades()
{
    DBUtil DB = new DBUtil();
    DataTable dtTradeGrades;

    dtTradeGrades = DB.GetTradeGrades();
    ddlTradeGrades.DataValueField = "tradeGrade";
    ddlTradeGrades.DataTextField = "descr";
    ddlTradeGrades.DataSource = dtTradeGrades;
    ddlTradeGrades.DataBind();
}

protected void FillExecutionGrades()
{
    DBUtil DB = new DBUtil();
    DataTable dtExecutionGrades;

    dtExecutionGrades = DB.GetExecutionGrades();
    ddlExecutionGrades.DataValueField = "executionGrade";
    ddlExecutionGrades.DataTextField = "descr";
    ddlExecutionGrades.DataSource = dtExecutionGrades;
    ddlExecutionGrades.DataBind();
}

How can I be a bit smarter about this? Can you help me re-write the code so that it's not repeating so much?

UPDATE

Wow, thanks for the replies, I thought I'd post what I'm thinking of implementing. I also created myself another little worker to remove some other ugly duplicated code. What do you think of this?

void FillDropDownList(DropDownList ddl, DataTable dt, string dataValueField, string dataTextField, string defValue)
{
    ddl.DataValueField = dataValueField;
    ddl.DataSource = dt;
    if (!string.IsNullOrEmpty(dataTextField))
    {
        ddl.DataTextField = dataTextField;
    }

    ddl.DataBind();
    ddl.SelectedValue = defValue;
}

private string GetTradeItem(DataTable tradeDetails, string attribute)
{
    return tradeDetails.Rows[0][attribute].ToString();
}

and then call it with something like:

int tradeId = int.Parse(Request.QueryString["tradeId"]);
DBUtil DB = new DBUtil();
DataTable tradeDetails = DB.GetTrade(tradeId);
FillDropDownList(ddlTradeGrades, DB.GetTradeGrades(), "tradeGrade", "descr", GetTradeItem(tradeDetails, "tradeGrade"));

Coding feels great when something ugly turns into something more elegant.

A good question to ask at an early point in your career. The ability to avoid "snippets" and "copy & paste programming" is an important skill to acquire. — , May 19 '11 at 17:49
You want to read [Refactoring](http://www.amazon.com/Refactoring-Improving-Design-Existing-Code/dp/0201485672). — John Saunders, May 19 '11 at 18:01
possible duplicate of [Need generic utility C# method for populating ASP.NET DropDownList](http://stackoverflow.com/questions/1894646/need-generic-utility-c-method-for-populating-asp-net-dropdownlist) — John Saunders, May 19 '11 at 18:02
Voted to close as duplicate, but great answers should be merged with original. — John Saunders, May 19 '11 at 18:02
Isn't this question about how to avoid repeating code, not about this specific example? I have to say, I'm a bit disappointed by the answers, which are too specific. — , May 19 '11 at 18:06
@NeilButterworth: Read the OP's question at the bottom of the post. It's clearly about this specific example for learning purposes. — mellamokb, May 19 '11 at 18:08
@mellamokb Well, it SHOULD be about the more general case, which would be much more interesting and useful, both to questioner and answerers. — , May 19 '11 at 18:11
@Neil: I agree, but that's a blog post (or a read of Refactoring), and not a Stackoverflow answer. — John Saunders, May 19 '11 at 18:17

Bala R · Accepted Answer · 2011-05-19T18:01:57.773

8

Something like this, maybe?

void SetupDataSource(DropDownList ddl, DataTable dt, string dataValueFieldm, string dataTextField)
{
    if (ddl != null)
    {
        ddl.DataValueField = dataValueField;
        ddl.DataSource = dt;
        if (!string.IsNullOrEmpty(dataTextField)) 
        {
            ddl.DataTextField  = dataTextField;
        }

        ddl.DataBind();
    }
    else
    {
       throw new ArgumentNullException("ddl");
    }
}

edited May 19 '11 at 18:01

answered May 19 '11 at 17:52

Bala R

107,317
23
199
210

@John to avoid NullReferenceException but I guess the requirement for the check is situational and sometimes it's better not to check. – Bala R May 19 '11 at 17:58
@mellamokb I just added `DataTextField` – Bala R May 19 '11 at 17:58
@Bala: my point was that the original code didn't have the check, so one presumes it was not required. – John Saunders May 19 '11 at 17:59
9

I would argue the null-check there is *bad practice* because it hides a programming error (with an implied contract the values passed in are "valid"). If anything it should be `if (ddl == null) throw new ArgumentNullException("ddl");`. – May 19 '11 at 17:59

score 7 · Answer 2 · 2011-05-19T18:11:30.773

FWIW, perhaps something like:

void BindControl(DropDownList ddl, string valueField, string textField, DataTable data) {
    ddl.DataValueField = valueField;
    ddl.DataTextField = textField ?? valueField; // textField can be null
    ddl.DataSource = data;
    ddl.DataBind();
}

DBUtil DB = new DBUtil();
BindControl(ddlTradeGrades, "tradeGrade", "descr", DB.GetTradeGrades());
...

However, that plumbing isn't terribly duplicated by itself and ease of future modifications/maintenance should be considered.

Happy coding.

Jim Mischel · Answer 3 · 2011-05-19T18:01:40.953

2

Create a method that sets the things that you want:

void SetValues(DropDownList ddl, string datavalue, string text, object ds)
{
    ddl.DataValueField = dataValue;
    ddl.DataTextField = text;
    ddl.DataSource = ds;
    ddl.DataBind();
}

Then you can call it with:

SetValues(ddlTradeGrades, "tradeGrade", "descr", dtTradeGrades);

edited May 19 '11 at 18:01

answered May 19 '11 at 17:54

Jim Mischel

131,090
20
188
351

@John: A type. Unfortunately, not a type that's defined in the Framework. Changed to `Object` to match what `DropDownList` uses. – Jim Mischel May 19 '11 at 18:01

score 2 · Answer 4 · answered May 19 '11 at 17:57

How about something like:

void FillData(DataTable dataSource, DropDownList ddl, string dataValueField, string dataTextField)
{
    ddl.DataSource = dt;
    ddl.DataValueField = dataValueField;
    ddl.DataTextField = dataTextField;
    ddl.DataBind();
}

Then you can call it like:

FillData(DB.GetTradeSetups(), ddlSetups, "tradeSetupId", string.Empty)

Jemes · Answer 5 · 2011-05-19T17:59:43.017

1

You can do something like this:

   private void BindMyLists()
    {
        DBUtil DB = new DBUtil();
        BindDropDownList(this.ddlExecutionGrades, DB.GetExecutionGrades(), "executionGrade", "descr");
        BindDropDownList(this.ddlTradeGrades, DB.GetTradeGrades(), "tradeGrade", "descr");
        //etc
    }
    protected void BindDropDownList(DropDownList dropDownList, DataTable dataTable, string dataValueField, string dataTextField)
    {
        dropDownList.DataValueField = dataValueField;
        dropDownList.DataTextField = dataTextField;
        dropDownList.DataSource = dataTable;
        dropDownList.DataBind();
    }

edited May 19 '11 at 17:59

answered May 19 '11 at 17:53

Jemes

2,822
21
22

`ddlTradeGrades` -> `dropDownList` in `BindDropDownList` – mellamokb May 19 '11 at 17:55
Yeaaah... I hit submit way too quickly and had loads of errors. Down vote deserved :). Fixed for future reference. My apologies. – Jemes May 19 '11 at 18:01

score 1 · Answer 6 · edited May 23 '17 at 11:55

1

See Need generic utility C# method for populating ASP.NET DropDownList

edited May 23 '17 at 11:55

Community

1
1

answered May 19 '11 at 17:59

Luke Hutton

10,612
6
33
58

score 1 · Answer 7 · answered May 19 '11 at 18:07

May be like this...

protected void FillDdl(DropDownList ddl, string dataValueField, Func<DataTable> dataTableMethod)
{
    FillDdl(ddl, dataValueField, null, dataTableMethod);
}

protected void FillDdl(DropDownList ddl, string dataValueField, string dataTextField, Func<DataTable> dataTableMethod)
{
    DataTable dt = dataTableMethod();

    ddl.DataSource = dt;

    ddl.DataValueField = dataValueField;
    ddl.DataTextField = dataTextField ?? dataValueField;

    ddl.DataBind();
}

and then call directly like this

DBUtil DB = new DBUtil();

FillDdl(ddlSetups, "tradeSetupId", DB.GetTradeSetups);
FillDdl(ddlTimeFrames, "tfCode", DB.GetTimeFrames);
FillDdl(ddlTradeGrades, "tradeGrade", "descr", DB.GetTradeGrades);
FillDdl(ddlExecutionGrades, "executionGrade", "descr", DB.GetExecutionGrades);

or you could still have skinny individual methods

protected void FillTradeSetups()
{
    DBUtil DB = new DBUtil();
    FillDdl(ddlSetups, "tradeSetupId", DB.GetTradeSetups);
}

protected void FillTimeFrames()
{
    DBUtil DB = new DBUtil();
    FillDdl(ddlTimeFrames, "tfCode", DB.GetTimeFrames);
} 

protected void FillTradeGrades()
{
    DBUtil DB = new DBUtil();
    FillDdl(ddlTradeGrades, "tradeGrade", "descr", DB.GetTradeGrades);
}

protected void FillExecutionGrades()
{
    DBUtil DB = new DBUtil();
    FillDdl(ddlExecutionGrades, "executionGrade", "descr", DB.GetExecutionGrades);
}

+1 but FYI, using delegates can trigger some obscure CLR bugs related to debugging and unit testing. In this case, the methods being called are so simple that the delegate is not actually required, so I would do it without the delegate. If they were methods with differing signatures, or if more than just the method call was required, then the delegate would be the way to go. — John Saunders, May 19 '11 at 18:15
Thanks. So would it be better to pass a string methodName in the method and then use reflection (slow and probably overkill) or case statement to call the appropriate method on DBUtil? — amit_g, May 19 '11 at 18:28
No it would be best simply to take a parameter of type `DataTable` and use `DB.GetTradeSetups()`, `DB.GetTradeGrades()`, etc. in your caller. — mellamokb, May 19 '11 at 18:35
Right, as @mellam: says, you don't need the full generality provided by a delegate or by using the method name. — John Saunders, May 19 '11 at 18:49

How to avoid repeated code?

7 Answers7

Linked