Efficient way to avoid duplicated code of buttons clicks

Question

Say I have 2 buttons witch supposed to perform the same operation but on different objects.

Currently I'm passing all the needed references to the method like this:

    private void sub1_add_to_db_btn_Click(object sender, EventArgs e)
    {
        Add_Substance_To_Database(
            substanse1, sub1_add_to_db_btn, sub2_add_to_db_btn, sub1_found_in_db_list,
            sub2_found_in_db_list, false, sub1_listBox, sub2_listBox);
    }

    private void sub2_add_to_db_btn_Click(object sender, EventArgs e)
    {
        Add_Substance_To_Database(
            substanse2, sub2_add_to_db_btn, sub1_add_to_db_btn, sub2_found_in_db_list,
            sub1_found_in_db_list, false, sub2_listBox, sub1_listBox);
    }

I was wondering if there is some other, more efficient way to do that. Thanks.

EDIT:

This is how some of my code looks like and it making me CRAZY!!!

private void sub1_add_to_db_btn_Click(object sender, EventArgs e)
    {
        Add_Substance_To_Database(substanse1, sub1_add_to_db_btn, sub2_add_to_db_btn,
            sub1_found_in_db_list, sub2_found_in_db_list, false, sub1_listBox, sub2_listBox);
    }

    private void sub2_add_to_db_btn_Click(object sender, EventArgs e)
    {
        Add_Substance_To_Database(substanse2, sub2_add_to_db_btn, sub1_add_to_db_btn,
            sub2_found_in_db_list, sub1_found_in_db_list, false, sub2_listBox, sub1_listBox);
    }

    private void sub1_edit_name_btn_Click(object sender, EventArgs e)
    {
        Add_Substance_To_Database(substanse1, sub1_add_to_db_btn, sub2_add_to_db_btn,
            sub1_found_in_db_list, sub2_found_in_db_list, true, sub1_listBox, sub2_listBox);
    }

    private void sub2_edit_name_btn_Click(object sender, EventArgs e)
    {
        Add_Substance_To_Database(substanse2, sub2_add_to_db_btn, sub1_add_to_db_btn,
            sub2_found_in_db_list, sub1_found_in_db_list, true, sub2_listBox, sub1_listBox);
    }

    private void sub1_delete_from_db_btn_Click(object sender, EventArgs e)
    {
        Delete_Substance_From_DB(sub1_listBox,
            sub2_listBox,sub2_list_is_from_file,sub1_delete_from_db_btn,
            sub2_delete_from_db_btn);
    }

    private void sub2_delete_from_db_btn_Click(object sender, EventArgs e)
    {
        Delete_Substance_From_DB(sub2_listBox,
            sub1_listBox,sub1_list_is_from_file,sub2_delete_from_db_btn,
            sub1_delete_from_db_btn);
    }

For example: If I want to delete a substance, I need to delete it from both of the lists and remove it from other lists, change the selection to the next substance etc...

You can wire the same event handler up to several buttons, then choose which Add_Substance_To_Database variant to run based on the sender. However, you might actually find it better to change your UI such athat substance1 and other variable arguments are sourced from controls like drop down lists. Do you have a sample of the GUI you could show? — dash, Jun 30 '12 at 23:21
For privacy reasons I would prefer not to publish the GUI. The thing is that for convenience reasons I must have 2 lists of substances compared side by side. Each selection of one substance from one list will perform some compare actions relative to the other selected substance from the other list. Also next to the each list there are buttons like: delete, edit, add to database and so on... Each of those buttons have similar methods like the one I wrote above. — Igor, Jun 30 '12 at 23:39
Why do you pass the buttons and list boxess to your function? The list boxes I can understand as you are probably looking to see what is in substance1 list and substance2 list. Strictly speaking, if you let the user choose two items, one from each list, you could just pass the selected items and the lists to your method - you can search each list for the substance inside Add_Substance_To_Database for example. — dash, Jun 30 '12 at 23:52
The reason for passing the buttons is because the listboxes are multipurpose. When I press a button named "Show from data base" the list shows substances from database instead from local files and the button changes to "Show from file". Also when the list is in database mode the button "Add to database" must be disabled. Mostly all my buttons are enabled/disabled to the current "state" of the list. That is why I need to pass the relevance buttons references to each method. — Igor, Jul 01 '12 at 00:04

score 5 · Answer 1 · answered Jun 30 '12 at 23:20

5

Given that you are passing different values to Add_Substance_To_Database what you have now is probably the most maintainable code you can hope for.

You could attach one event handler to both buttons, but you'd have to work out which button was pressed and pass the relevant arguments anyway:

if (sender == button1)
{
    Add_Substance_To_Database(
        substanse1, sub1_add_to_db_btn, sub2_add_to_db_btn, sub1_found_in_db_list,
        sub2_found_in_db_list, false, sub1_listBox, sub2_listBox);
}
else
{
    Add_Substance_To_Database(
        substanse2, sub2_add_to_db_btn, sub1_add_to_db_btn, sub2_found_in_db_list,
        sub1_found_in_db_list, false, sub2_listBox, sub1_listBox);
}

Which gets you exactly nowhere.

answered Jun 30 '12 at 23:20

ChrisF

134,786
31
255
325

+1 agree; This feels like a situation where a change to the GUI could simplify the save logic immensely – dash Jun 30 '12 at 23:21
I was thinking will it be more efficient (from a coder point of view) to save all the controls to an object array and then pass it to the function like: if (sender == button1) Add_Substance_To_Database(array1).. else array2.. and then each method will "extract" only the related controls by casting? – Igor Jul 01 '12 at 00:14

score 3 · Answer 2 · edited May 23 '17 at 11:48

3

This is not a case of code duplication at all. Refactoring SUCH case would only make it:

more complicated
unreadable

Please see: Any valid reason for code duplication?

EDIT:

If you had any more 'meat' in event handlers that is duplicated, I would suggest maybe something different. But you pulled everything down into Add_Substance_To_Database so you already de-duplicated your code successfully.

edited May 23 '17 at 11:48

Community

1
1

answered Jun 30 '12 at 23:21

Daniel Mošmondor

19,718
12
58
99

Please look at the edited post... You think I should leave it like that? What do you think about the suggestion I made in the comment of the first answer? – Igor Jul 01 '12 at 00:32
I would stay at my position, and would ask you: what would be the look of the code that won't make you crazy? Why it makes you crazy? Do you find it not aesthetically pleasing, or do you find it hard to maintain, or do you think that it would be hard to debug? Refactoring such case (in your case some kind of table would be appropriate to hold parameter lists) has to be DRIVEN with some need. – Daniel Mošmondor Jul 01 '12 at 23:44

burning_LEGION · Answer 3 · 2012-06-30T23:43:51.643

event handler may be one for few controls, in sender you have button control, which call event, cast it to Button class and use tham as parametrs of method

var button = sender as Button;

Add_Substance_To_Database( substanse2, button, sub1_add_to_db_btn, sub2_found_in_db_list, sub1_found_in_db_list, false, sub2_listBox, sub1_listBox);

for other parameter you can use property Tag, it's object for developer purpose

Igor · Accepted Answer · 2012-07-03T13:49:23.583

Eventually I followed my own suggestion... I created an object array with all the controllers and added same methods to the controllers event handlers. In the method I simply select the right controller according to the sender.

substance1_controllers = new object[]{
            sub1_main_listbox, sub1_peaks_list,sub1_found_in_db_list,
            sub1_similar_in_db_list, sub1_eigenvector_list,
            sub1_sourse_switch_btn, sub1_folder_btn,sub1_add_to_db_btn,
            sub1_edit_name_btn,sub1_delete_btn, sub1_picture_box,
            chart_peaks.Series[0], chart_compare.Series[0], true, -1};
substance2_controllers = new object[]{
            sub2_main_listbox, sub2_peaks_list, sub2_found_in_db_list,
            sub2_similar_in_db_list,sub2_eigenvector_list,
            sub2_sourse_switch_btn, sub2_folder_btn, sub2_add_to_db_btn,
            sub2_edit_name_btn,sub2_delete_btn, sub2_picture_box, 
            chart_peaks.Series[1],chart_compare.Series[1], true, -1};

It might look like it is harder to maintain that way but personally I found it very comfortable to maintain and use with the help of this table (and it looks great):

        // [0]  - Main Listbox
        // [1]  - Peaks Listbox
        // [2]  - Found in Database Listbox
        // [3]  - Found similar Listbox
        // [4]  - Eigenvectors Listbox
        // [5]  - Switch sourse Button
        // [6]  - Change Folder Button
        // [7]  - Add to Database Button
        // [8]  - Edit Name Button
        // [9]  - Delete Button
        // [10] - Picture Box
        // [11] - Peaks Chart Series
        // [12] - Compare Chart Series
        // [13] - List is from File Boolean
        // [14] - Previous Selected Index

Method Example:

void Delete_Substance_From_DB(object sender, EventArgs e)
    {
        object[] controller;
        object[] other_controller;
        if (((Button)sender).Name == "sub1_delete_btn")
        {
            controller = substance1_controllers;
            other_controller = substance2_controllers;
        }
        else
        {
            controller = substance2_controllers;
            other_controller = substance1_controllers;
        }
    }

Using examle:

if (((ListBox)other_controller[0]).Items.Count != 0)
        {
            if (((ListBox)other_controller[0]).Items.Count == index2)
            {
                ((ListBox)other_controller[0]).SelectedIndex = index2 - 1;
            }
            else
            {
               ((ListBox)other_controller[0]).SelectedIndex = index2;
            }
            Main_Listbox_Index_Changed(((ListBox)other_controller[0]), null);
        }

Efficient way to avoid duplicated code of buttons clicks

4 Answers4