2

In Java, given a pair of public methods, enableFoo and disableFoo, that set a boolean field named isEnabledFoo to true or false, respectively, should the method check to see if Foo is already enabled before setting? If yes, should an exception be thrown? If yes, should it be checked or unchecked? If unchecked, what should it throw? IllegalStateException?

A twist: Although the current implementation simply sets a boolean field, I'm purposely not implementing it as one setFoo "setter" method with a boolean parameter because the implementation might be changed later on to include side effects (maybe even not setting a field at all). Keeping it as enableFoo/disableFoo seemed the best way to guarantee encapsulation.

Should I do this:

public void enableFoo() throws myCheckedException
{
  if (! this.isEnabledFoo)
  {
    this.isEnabledFoo = true;
  }
  // Is this overkill?
  else
  {
    // Foo is already enabled...Should a checked exception be thrown?
    throw new myCheckedException ("Foo is already enabled.");

    // What about an unchecked exception?
    // throw new IllegalStateException ("Foo is already enabled.");
  }
}

or simply like this:

public void enableFoo()
{
  // I guess we don't care if foo is already enabled...
  this.isEnabledFoo = true;
}

or even:

public void enableFoo()
{
  // Is this just code bloat?
  if (! this.isEnabledFoo)
  {
    this.isEnabledFoo = true;
  }
}

I think that while the last two versions are simpler, they would hide a possible bug where the developer was calling enableFoo thinking that it was disabled, when in fact it was already enabled (but would that really matter?). What's the best design here?

5 Answers5

4

I don't really think there's an answer to your question. all of them seem valid, it depends on the operation you are doing and on the consistent state of the object following the operation.

For instance: I have a method which enables syntax highlighting, if you choose to enable it and it is already enabled what would you care. In such a case you would simply set the boolean flag regardless of the previous state (option 2).

If on the other hand I don't just change a boolean field but rather perform some complex logic which may be expensive to re-run such as go over the DOM of the document and color all the right places I wouldn't want to do it again just because someone "rechecked" the enable flag.

Something like this:

public void enableHighlighting() {
  if (!isHighlighting) {
    isHighlighting = true;
    colorView();  // very expensive method
  }
}

Now let's assume you have a case where double enabling reflects an inconsistent state of the machine (something like what you would use assert for. In this case you would want to throw an exception to denote that the user did something illegal.

For instance let's say syntax highlighting only affects certain types of files such as XML or Java code, and the user tries to apply it to PHP code. Now the highlighting is already enabled but the frustrated user tries to enable it because he doesn't see any change. In such a case you might want to print a message to the user saying that highlighting is already enabled. In such a case a return value for whether a change occurred would probably make more sense, but you could use an exception as well.

Return value:

public boolean enableHighlighting() {
  prevState = isHighlighting;
  isHighlighting = true;
  return prevState != isHighlighting; //return true if value changed
}
Asaf
  • 6,384
  • 1
  • 23
  • 44
  • Ok, so as I understand your answer, setters should in general be indempotent, unless there is very good reason for it not to be (expensive, etc). I'm thinking a developer would probably assume it was indempotent if there were no documentation to indicate otherwise. –  Apr 17 '11 at 06:12
  • @Aaron nice use of the word "idempotent". For future reference you may want to note that it is spelled without an 'n' in the second position. – ChrisH Apr 17 '11 at 06:37
0

Nope. Why should you care? enableFoo and disableFoo are very explicit - one enables, the other disables. No need to check for the current state.

If you really wanted, you could return a boolean which would indicate the previous state. You could also have an public boolean isEnabled() method.

Brian Roach
  • 76,169
  • 12
  • 136
  • 161
  • Yes, but would anyone take the time to check the previous state before calling? I personally don't think they would, especially if calling enableFoo twice wouldn't throw any exceptions. –  Apr 17 '11 at 05:45
  • That's the point though - if calling it twice isn't a problem (therefore the need to know isn't required) then throwing an exception is also not needed. Your method names convey functionality. As an example, look at [AbstractAction](http://download.oracle.com/javase/6/docs/api/javax/swing/AbstractAction.html) - the setter (`setEnabled(boolean)`) doesn't care what the current state is, and there's an `isEnabled()` if you want to check. – Brian Roach Apr 17 '11 at 05:51
0

There is not straight answer. It depends on requiremetns. Specifically whether your operation is idempotent.

http://en.wikipedia.org/wiki/Idempotence

Dakshinamurthy Karra
  • 5,353
  • 1
  • 17
  • 28
  • Thank you for the enlightening article. Should setters in general be idempotent? –  Apr 17 '11 at 05:58
  • I would like to think of attributes (member variables) of two types: those that provide identity to an object and those that provide behavior. The setters for identity are in general can be idempotent, but the setters for behavioral attribute need not be. As a thumb rule if the documentation does not say do not assume any method (including setters) to be idempotent. – Dakshinamurthy Karra Apr 17 '11 at 06:27
  • Could you provide an example of a behavioral vs identity attribute? I'm not familiar with this terminology. –  Apr 17 '11 at 06:35
  • To clarify, I thought that behavior == methods and identity == attributes. –  Apr 17 '11 at 06:41
  • The terminology is something I made up ;-). – Dakshinamurthy Karra Apr 17 '11 at 06:58
  • The terminology is something I made up ;-). Take regular swing components themselves. A JComponent has a name attribute - identity - setting it is idempotent. That does not change the way the component behaves. Now enabled attribute, changes the behavior of the component and usually the setEnabled is also an idempotent operation. The setFocusTraversalKeys method changes the behavior and also not idempotent - throws an exception if the key is already used by the component. – Dakshinamurthy Karra Apr 17 '11 at 07:25
0

In case your program is multithreaded, locking the enable and disable functions is advisable and the checks might help in case locks are not atomic. Otherwise you should not need to worry about checking state.

dreamer13134
  • 471
  • 1
  • 6
  • 19
  • In modern Java you can accomplish this with [java.util.concurrent.atomic.AtomicBoolean](http://download.oracle.com/javase/1.5.0/docs/api/java/util/concurrent/atomic/AtomicBoolean.html) – Brian Roach Apr 17 '11 at 06:00
0

You can use Facet for enabling/disabling features.

Disclaimer: I am one of the authors of the platform

You can think of facets as all the methods of your application. You can enable/disable methods during runtime.

How to integrate:

  • Grab the package from maven central
  • Add it to gradle config: implementation 'run.facet.agent.java:facet-agent:0.0.8'
  • Create a facet.yml:
workspaceId: WORKSPACE~ID
name: My-Application
environment: dev
apiKey: API_KEY

Note: you can grab these keys from the Facet dashboard

  • After the integration is complete, you can enable/disable methods live from the system. Note that there's no need of redeploying your application or restarting it.

To give you a perspective of how's that different from feature flags that others have mentioned, you can think that facets are applied automatically to your application, rather than declared programmatically every time you develop features.

Note: this is not production-ready yet. I am happy to troubleshoot with you!

Menelaos Kotsollaris
  • 5,776
  • 9
  • 54
  • 68