1
\$\begingroup\$

I have two buttons on an ASP.NET page that have separate OnClick methods in the code behind. In one of the methods, if a certain condition is met, the entire process of the other method should be executed.

Here is a simplified example of my code, which works as I intend in my actual implementation:

// OnClick="ThisButton_OnClick" for ThisButton
protected void ThisButton_OnClick(object sender, EventArgs e)
{
    // do stuff
    if(condition)
        ThatButton_OnClick(null, null);
    else
        // do stuff
}

// OnClick="ThatButton_OnClick" for ThatButton
protected void ThatButton_OnClick(object sender, EventArgs e)
{
    // do stuff
}

My question is this: is it good practice to do something like this, where you call a method specifically designed for OnClick without clicking that button, or should I be more explicit (perhaps have a separate helper function that is called by ThatButton_OnClick and in the if)?

\$\endgroup\$

2 Answers 2

8
\$\begingroup\$

I'd suggest something like the following which is simple enough for even a novice like me to understand. Is there a real need to simulate button click? You're not testing buttons, are you?

// OnClick="ThisButton_OnClick" for ThisButton
protected void ThisButton_OnClick(object sender, EventArgs e)
{
    // do stuff
    if(condition)
        DoStuffThatButtonWouldDo(arg);
    else
        // do stuff
}

// OnClick="ThatButton_OnClick" for ThatButton
protected void ThatButton_OnClick(object sender, EventArgs e)
{
    DoStuffThatButtonWouldDo(arg);
}

private static DoStuffThatButtonWouldDo(fancy arguments) 
{ 
    //TODO: do all the stuff that the that button would do 
} 
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I'm not testing buttons. Really, it's just chance that ThatButton_OnClick did what I might want (depending on condition) in ThisButton_OnClick \$\endgroup\$
    – wlyles
    Commented Jul 26, 2013 at 17:43
  • \$\begingroup\$ I think you made the right call in asking yourself if you wanted to make it more explicit. Since the logic does not depend on ThatButton, extracting the logic to its own method would reinforce newcomers like me that there is nothing special happening within ThatButton. Cheers! \$\endgroup\$
    – One
    Commented Jul 26, 2013 at 18:23
0
\$\begingroup\$

I would probably have a seperate method that takes no parameters. Mainly because by passing null, null you are assuming knowledge of the inner workings of the method. What happens if later on down the track your ThatButton_OnClick suddenly decides to do something with sender.

I think it would be fair enough for the ThatButton_OnClick method to assume the objects will not be null and work with them directly. So in that case suddenly your program would fail. You might not even notice that until the program makes production (if the test cases, user doesn't test the ThisButton_onClick functionality).

I would start by making the method private within the code behind. And later move it to the domain or service later later on if required.

protected void ThatButton_OnClick(object sender, EventArgs e)
{
    // Oh no, this will cause a NullReferenceException but would should it when sender should really never be null......

    if(sender.ToString == "Hello") 
    {
       // do something now
    }
}

As for Good practice? I don't think it's either here nor there in this case TBH. However it might help minimize technical debt and possible unintentional future bugs by implementing a seperate method now.

\$\endgroup\$

Your Answer

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

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