2

On an application I've been working on I have a function, connecting to a virtual machine, that can be performed two ways. The user can either select a machine from a list and click the "Connect" button. Or can double click on the machine in the list. Currently they are two separate event handlers because they function slightly differently. If a user is already connected, clicking the button will disconnect them, and double clicking will bring the window back up.

///edit The logic is essentially

button
if(connected)
Disconnect()
else
ConnectAndShowWindow()

List
ConnectAndShowWindow()

My question is should I keep the methods separate or combine the two methods even though they are slightly different and then, for the disconnecting/reconnecting switch them based on the sender?

Brian Green
  • 344
  • 3
  • 10
  • 5
    What about two separate methods calling a third common one? Or one doing its specific stuff and calling the other? – Florian Margaine Sep 27 '13 at 17:02
  • The important part is that `ConnectAndShowWindow()` and `Disconnect()` are really functions, not placeholders for similar code blocks, and the code is really not any more complicated than the snippet you showed us above. If this is true, leave everything as it is. – Doc Brown Sep 28 '13 at 07:21

3 Answers3

3

Your features are:

  • Connect
  • Disconnect
  • Bring Window Forward

The logic of the "Connect" button is:

if (connected)
    Disconnect
else
    Connect

The logic of the machine double-click is:

if (connected)
    Bring Window Forward
else
    Connect

To me, these are quite separate handlers and I would leave them so. They should of course call the common Connect method.

Carl Manaster
  • 4,173
  • 18
  • 31
  • Not sure if it changes the answer, but "Connecting" always includes "Bringing the window forward" I will change the logic in my question. – Brian Green Sep 27 '13 at 17:49
  • The answer is still substantially the same. You can represent this in other ways (eg Connect can call Bring Window Forward) but the two controls have different logic and should therefore have different handlers. – Carl Manaster Sep 27 '13 at 19:04
0

An approach that I've used in that type of situation is to refactor the two methods into a single private method with a parameter to indicate which of the paths should be taken.

public void buttonEventHandler() {
    connectHandler(ConnectEvent.BUTTON);
}
public void DoubleClickEventHandler() {
    connectHandler(ConnectEvent.DOUBLE_CLICK);
}
private void connectHandler(ConnectEvent eventOption) {
    // conditional connection logic
}

This keeps the interface simple and allows the connect code to be shared in place.

Mike Partridge
  • 6,587
  • 1
  • 25
  • 39
  • 1
    This is actually not a good idea. See [here](http://programmers.stackexchange.com/questions/147977/is-it-wrong-to-use-a-boolean-parameter-to-determine-behavior) why. – Florian Margaine Sep 27 '13 at 17:34
  • Using a boolean parameter is indeed not the best solution. I'll update the answer, although I think Carl's answer is more specific to the OP's question. – Mike Partridge Sep 27 '13 at 17:43
  • Don't use boolean's, use enumerated values, they are far better at self-explanation. See [Avoiding Booleans](http://www.codinghorror.com/blog/2005/10/avoiding-booleans.html) – Marjan Venema Sep 27 '13 at 17:59
0

So, what you're saying is that the software acts one way when the software is in one state and another thing when the software is in another state. Well, welcome to the State Pattern.

The short story is that you want two classes, each implementing the same interface. the interface should have two methods -- OnMachineConnectSelected and OnMachineDoubleClick.

The first class is called DisconnectedState, the second class is called ConnectedState.

  • DisconnectedState performs Connect and ShowWindow from OnMachineConnectSelected.
  • DisconnectedState performs Connect from OnMachineDoubleClick.
  • ConnectedState performs ShowWindow from both OnMachineConnectSelected and OnMachineDoubleClick.

(It's hard to tell without more specifics, but you can and probably should have Connect and ShowWindow as methods on your form or other application object and pass a reference, probably via another interface, into the constructor of your state class.)

Now your application simply needs an object on which it can call those methods, as the actions occur, and you can switch that object between an instance of DisconnectedState and ConnectedState as the actual state of your application changes.

See what this does for you?

First, it models your application very well. Look at the bullet-points above. You wouldn't have to rearrange the words much to explain what was going on to a user or non-technical manager. You are separating out the concepts of state (software/hardware decisions) and action (user decisions) neatly.

Second, it allows for extensibility. When you realise that you need to act very differently, while connecting and disconnecting, you can simply add two new state classes. Or if you need to add a new action that depends on the state, you can simply add a new method to all your state objects.

Third, it is more maintainable. When a business person says "Hey, currently, when the application is Disconnected and you Select Connect on a machine, it Connects and Shows the Window; but we want it to simply connect," you know EXACTLY where that logic exists in the code and can edit it easily, safe in the knowledge that you aren't breaking any of the other conditions.

Finally, it's more unit-testable. There is only one path through each method. Pass a mock form into your class constructor, call the OnX method and make sure it performs the right actions. And, on the form, you can test that your action calls the right method, no matter what state it's in.

pdr
  • 53,387
  • 14
  • 137
  • 224