1

I'm working on a .NET application and I'm wondering if I should use separate methods to handle the click events of two different buttons. They essentially do the same thing, just on different objects on the form, in this case two separate maps.

Same method: 

Private Sub SingleButtonHandler(sender As Object, e As EventArgs) Handles btnToggleSatNew.Click, btnToggleSatOld.Click
    If sender Is btnToggleSatNew Then
        Me.NewMap.ToggleSateliteView()
    Else
        Me.OldMap.ToggleSateliteView()
    End If
End Sub

Separate Methods: 

Private Sub btnToggleSatOld_Click(sender As Object, e As EventArgs) Handles btnToggleSatOld.Click
    Me.OldMap.ToggleSateliteView()
End Sub

Private Sub btnToggleSatNew_Click(sender As Object, e As EventArgs) Handles btnToggleSatNew.Click
    Me.NewMap.ToggleSateliteView()
End Sub

What's the overhead of checking if the two objects are the same? Is one version easier to read than the other? Or should I be doing something completely different?

Improve this question
2
  • 1
    In general, avoid 'if' statements. Methods with if statements are more difficult to describe, more difficult to test, and more difficult to read.
    – kevin cline
    Aug 29, 2014 at 16:27
  • 1
    There's one VERY strong reason to use a single method (it might not apply in this case, but I had this happen in a java/GWT app). If the class holding the button handler is constructed anonymously, you will create a new instance for each thing generating events, which if they are done in high numbers or in a loop (table row entries perhaps), consume lots of memory and runtime. Using one method means one object instance. Harder to maintain but the performance is orders of magnitude better (in the case I hit)
    – Daenyth
    Aug 29, 2014 at 16:35

2 Answers 2

Reset to default
7

Use separate methods. It makes the method easier to read, and makes it clearer what the method does. The SingleButtonHandler does two things based on a parameter: it toggles the new map if the sender is btnToggleStatNew, or toggles the old map. While it isn't a horribly messy method right now, it can grow into a large, unwieldy tangle of spaghetti code very quickly.

In addition, it opens up the question of "where should I stick this button's event?" With 1 click handler for two buttons, it sets a bad precedence. There will be the temptation to stick other button's click event into the one, because "this button's click is easy. It doesn't need another whole method" or "this button is kind of related to the others." Don't open up that temptation. Each button gets its own even handler.

On the performance overhead: Don't worry about it. Unless there's code that's causing major headaches, do not attempt to optimize. Premature optimization is the root of all evil. If the profiler says the code isn't the major time sink, then don't try to optimize it. Make it work. Make it readable.

Improve this answer
2

Alternatively, you could add the map to be toggled as a property of the button, and then have one event handler which toggles the map given in the sender's property. This avoids an if/then statement, but gives you one method which does one thing.

Improve this answer

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.