CODEWITHSUNDEEP
c#winforms
witoong623
witoong623

Reputation: 1199

Check which object call to event method

I have ComboBoxs, each one have its SelectedIndexChanged event. I don't want to write each ComboBox's event but I want to write one event method that check which ComboBox call to an event and let control do specific method for each Combobox. I've searched and wrote like this:

private void eventmethod(object sender, EventArgs s)
{
    ComboBox cb = sender as ComboBox;
    if (cb != null & cb.Name.Equals("combobox1"))
    {
        method1();
    }
    else if (cb != null & cb.Name.Equals("combobox2"))
    {
        method2();
    }
}

Is there more efficient way to check?

Upvotes: 1

Views: 2801

Answers (4)

Will Marcouiller
Will Marcouiller

Reputation: 24152

This approach breaks the Single Responsibility Principle (SRP) unless every ComboBox.SelectedIndexChanged Event does the same.

It is always best to have an event handler for each control so that specific behaviour can be easily separated. If a specific behaviour is given for each ComboBox, and they also have a common behaviour, then use a helper method to perform common tasks.

private void comboBox1_SelectedIndexChanged(object sender, EventArgs e) {
    doesCommonTasksToBePerformedThroughAHelperMethod();
}

private void comboBox2_SelectedIndexChanged(object sender, EventArgs e) {
    // Does specific tasks
    doesCommonTasksToBePerformedThroughAHelperMethod();
}

private void doesCommonTasksToBePerformedThroughAHelperMethod() {
    // Perform common tasks
}

This is the prefered practice.

Otherwise, check for the control itself.

var cb = sender as ComboBox;

if (cb == null) return;

switch(cb) {
    case cb.Equals(comboBox1): 
        // Do something
        break;
    case cb.Equals(comboBox2):
        // Do something
        break;
}

switch statements are rarely a good thing. Here are some links on the topic.

  1. How to explain why a developer should avoid case statements and create some new classes?
  2. Refactoring a Switch statement
  3. Switch Statements

Upvotes: 4

Adam H
Adam H

Reputation: 1573

You could use a switch statement:

private void eventmethod(object sender, EventArgs s)
{
    ComboBox cb = sender as ComboBox;
    if (cb == null) return;
    switch (cb.Name)
    {
        case "combobox1":
            method1();
            break;
        case "combobox2":
            method2();
            break;
    }
}

This isn't necessarily the best way of doing this, but it does remove the need for multiple null checks.

Upvotes: 0

n8wrl
n8wrl

Reputation: 19765

It is much better practice to have individual event handlers for each control and then have them call the same method if they do the same thing:

void combo1_OnChange(object sender, EventArgs e)
{
    CommonMethod();
}
void combo2_OnChange(object sender, EventArgs e)
{
    CommonMethod();
}

In your case, they do not do the same thing, so it makes even more sense to have separate handlers:

void combo1_OnChange(object sender, EventArgs e)
{
    Method1();
}
void combo2_OnChange(object sender, EventArgs e)
{
    Method2();
}

An exception to this rule would be if you were creating controls on the fly, but even then you're probably going to be using delegates.

Upvotes: 1

Sorceri
Sorceri

Reputation: 8053

No need to check for null multiple times, just do it once and return if found. also no need to check the name as you can just check for the control ie combobox. Being that you are calling a different method there is no harm in having multiple controls pointing to this event. I would also surround it with a try catch statement.

private void eventmethod(object sender, EventArgs s)
{
    ComboBox cb = sender as ComboBox;
    //check for null once and return if found
    if(cb == null) return;
    //no need to call for the name just check for the combobox
    //assuming the name of the control is combobox1 and combobox2
    if (cb.Equals(combobox1))
    {
        method1();
    }
    else if (cb.Equals(combobox2))
    {
        method2();
    }
}

Upvotes: 0

Related Questions