Best practice - Two functions with analog meaning

3968 views c#
-3

I want to know what is the best practice for that situation. Take those two functions

Option 1 - TableController.cs

void deleteRow(Row myRow) {
    // do some control on myRow
    if (!(myRow.hasThatValue()) && (myRow.Title.StartsWith("xxx"))
        return;

    Rows.Remove(myRow);
    Events.OnRemoveRow(myRow);
}


void deleteRows(List<Row> myRows) {
   foreach (var r in myRows) {
       deleteRow(r);     // That call each time OnRemoveRow()
   }
}

Option 2 - TableController.cs

void deleteRow(Row myRow) {
    // do some control on myRow
    if (!(myRow.hasThatValue()) && (myRow.Title.StartsWith("xxx"))
        return;

    Rows.Remove(myRow);
    Events.OnRemoveRow(myRow);   
}


void deleteRows(List<Row> myRows) {
   foreach (var r in myRows) {
        // do some control on myRow
        if (!(myRow.hasThatValue()) && (myRow.Title.StartsWith("xxx"))
            return;

        Rows.Remove(myRow);
   }

   Events.OnReloadTable();     // That call only one time OnReloadTable()
}

Can you spot the difference? Say you have 1000 rows to delete and in the event OnRemoveRow you delete a row in a ListView, instead in the event OnReloadTable you clear and reload all rows in the ListView.

With option 1 you raise 1000 times the event that do a lot of work in the GUI, instead with option 2 you call only one time an event that reload all rows.

Using a benchmark is obviously that option 2 perform a lot better.

So the question is:

There is an alternative or a better way that repeat two times the same code like in option 2 but with the good performance of that solution?

answered question

That completely depends on your environment and your context. However if you already did benchmarks why not trust them? When your code does what it should, what exactly are you asking here?

1 Answer

11

As you noted, option #2 would perform better, but it repeats a lot of code between the two methods, and will result in a hard to maintain codebase.

I'd go for a third option, where deleteRows contains the better performing logic of option #2 and deleteRow reuses it:

void deleteRow(Row myRow) {
    deleteRows(new List<Row>(){myRow});
}


void deleteRows(List<Row> myRows) {
   foreach (var r in myRows) {
        // do some control on myRow
        if (!(myRow.hasThatValue()) && (myRow.Title.StartsWith("xxx"))
            return;

        Rows.Remove(myRow);
   }

   Events.OnReloadTable();
}

posted this

Have an answer?

JD

Please login first before posting an answer.