5
\$\begingroup\$

I have 3 simple similar event handler functions that I would like to refactor. Any suggestions?

private void btnBuildingList_Click(object sender, EventArgs e)
    {
        selectedExportType = (int)ExportType.Building;
        path = csvFilePath + String.Format("{0:yyyy-MM-dd}", datDate.DateTime) + "-BuildingList.csv";
        Export();
    }

    private void btnOwnerList_Click(object sender, EventArgs e)
    {
        selectedExportType = (int)ExportType.Persons;
        path = csvFilePath + String.Format("{0:yyyy-MM-dd}", datDate.DateTime) + "-OwnerList.csv";
        Export();
    }

    private void btnFacts_Click(object sender, EventArgs e)
    {
        selectedExportType = (int)ExportType.Facts;
        path = csvFilePath + String.Format("{0:yyyy-MM-dd}", datDate.DateTime) + "-FactsData.csv";
        Export();
    }
\$\endgroup\$
15
\$\begingroup\$
  1. You can use an Extract Method refactoring to get rid of the duplicate code:

    private static string GetExportFilePath(string csvFilePath, DateTime date, string fileSuffix)
    {
        return string.Format("{0}{1:yyyy-MM-dd}-{2}.csv", csvFilePath, date, fileSuffix);
    }
    
  2. I would add those selectedExportType and path variables as parameters to the Export() method. I hope they are not used anywhere. It is usually pretty subjective whether to add something as method parameters or leave it as class member, but here I would definitely pass them as parameters. I assume this class is a “View” since you have button click event handlers, and I also tend to move such methods out from the views.

If we use these two ideas, your event handlers would be simplified to:

private void btnBuildingList_Click(object sender, EventArgs e)
{
    var path = GetExportFilePath(csvFilePath, datDate.DateTime, "BuildingList.csv");
    Export((int)ExportType.Building, path);
}

private void btnOwnerList_Click(object sender, EventArgs e)
{
    var path = GetExportFilePath(csvFilePath, datDate.DateTime, "OwnerList.csv");
    Export((int)ExportType.Persons, path);
}

private void btnFacts_Click(object sender, EventArgs e)
{
    var path = GetExportFilePath(csvFilePath, datDate.DateTime, "FactsData.csv");
    Export((int)ExportType.Facts, path);
}
| improve this answer | |
\$\endgroup\$
8
\$\begingroup\$

In addition to what @Snowbear suggests. I thought you should know that all button's click event can be pointed to the same event handler. You can then distinguish each one using the sender parameter.

I prefer @Snowbear's recommendation but I mention this because many people don't seem to know this can be done.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

I would push the path formatting code down into the "Export" function. Creating a path formatting function merely adds noise to the program. I would modify the "Export" function to take one parameter of type ExportType. This parameter can be used to determine the type of export as well as the file prefix string by putting the file prefixes in a string array in which the strings are ordered the same as the class constants or enumeration.

Note: this example assumes that BuildingList resolves to 0, OwnerList resolves to 1, and FactsData resolves to 2.

private void Export( ExportType typeOfExport )
{

    string[] exportPrefixes = { "BuildingList", "OwnerList", "FactsData" };

    string path = csvFilePath + String.Format("{0:yyyy-MM-dd}", datDate.DateTime) + "-" +  exportPrefixes[(int)typeOfExport] + ".csv";

   ...

}

As Shiv Kumar mentioned, one can create a single event and use the parameter "sender" to determine the caller. In this case, we would have something like:

 private void unified_Click(object sender, EventArgs e)
 {
     if (sender is btnBuildingList)
         Export(ExportType.Building);
     else if (sender is btnOwnderList)
         Export(ExportType.Owner);
     else if (sender is btnFacts)
         Export(ExportType.Facts);
 }

Whether or not, you feel that a unified approach is cleaner is up to you.

| improve this answer | |
\$\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.