146

I have a class where every method starts the same way:

class Foo {
  public void bar() {
    if (!fooIsEnabled) return;
    //...
  }
  public void baz() {
    if (!fooIsEnabled) return;
    //...
  }
  public void bat() {
    if (!fooIsEnabled) return;
    //...
  }
}

Is there a nice way to require (and hopefully not write each time) the fooIsEnabled part for every public method in the class?

kris
  • 23,024
  • 10
  • 70
  • 79
  • Do you want to be like every time start will be same and then the code differs based on condition? – Nakul91 Jun 29 '15 at 17:09
  • 46
    Look into Aspect Oriented Programming (specifically _before advice_). – Sotirios Delimanolis Jun 29 '15 at 17:10
  • 5
    If you have particular condition based on which your further code is decided then look for strategy design pattern – Nakul91 Jun 29 '15 at 17:14
  • @Nakul91 each method does different things, they just all need to return immediately if Foo isn't "turned on." – kris Jun 29 '15 at 17:14
  • 15
    How many methods do you have to use this boilerplate for? Before you go down the route of introducing AOP you might want to consider just having this little bit of repeated code. Sometimes a little copy and paste is the simplest solution. – bhspencer Jun 29 '15 at 17:15
  • 2
    Contrary to what others have said here I don't think that the enabled-ness of a class is a "cross cutting concern" and is not a good fit for AOP. – bhspencer Jun 29 '15 at 17:18
  • 2
    @bhspencer a dozen methods. I could certainly copy/paste, just worried a future maintainer won't notice that they need to. – kris Jun 29 '15 at 17:20
  • 52
    I suspect your future maintainer would be happier with the extra boiler plate than having to learn an AOP framework. – bhspencer Jun 29 '15 at 17:22
  • 3
    I could imagine a solution that had an abstract base class and two classes that extend it, one the enabled version and one the disabled version. You also need some container class that has a getInstance method that returns you the enabled or disabled version. The disabled class would just have empty method stubs. – bhspencer Jun 29 '15 at 17:25
  • 3
    This is kind of a code smell in my opinion. It looks like there is some mutable state that you want to test against. Without knowing more about the design and flow of your program, I don't think an elegant suggestion will be made. – Amir Afghani Jun 29 '15 at 17:32
  • 2
    You don't need a fancy AOP framework. Spring provides very simple AOP functionality. – Sotirios Delimanolis Jun 29 '15 at 17:56
  • Having looked at the proposed solutions I get the sense that they all add more work than is worth it. I say stick with the copy and paste and write some unit tests. Your tests will serve as documentation and show intention for your future maintainer. – bhspencer Jun 29 '15 at 17:57
  • 1
    There are two reasons you might want to automate this; to make the code prettier, and to prevent errors such as accidentally forgetting to add it to a method. Most people focus on the former, but most of the real value is in the latter. If you manually insert the snippet, you can use an annotation processor or other tooling to *verify* that you've inserted the correct block of code at the beginning of the method, so you can early detect when someone has failed to follow the pattern. – Brian Goetz Jun 29 '15 at 18:17
  • 7
    If every method of the class has to do the very same thing in their first lines of code then we have a bad design. – Tulains Córdova Jun 29 '15 at 19:10
  • Shouldn't this question be migrated to programmers? – Tulains Córdova Jun 29 '15 at 19:16
  • 7
    @user1598390: The question is not off-topic here, and there's nothing about Programmers' scope that makes the question especially meaningful there. – Robert Harvey Jun 29 '15 at 19:19
  • 2
    @user1598390: how would you implement something like a [Freezable](https://msdn.microsoft.com/en-us/library/system.windows.freezable%28v=vs.110%29.aspx) class then? – Thomas Weller Jun 30 '15 at 12:30
  • @user1598390 I think that's the point of the question. How would you fix this bad design? – Ian Goldby Jun 30 '15 at 12:31
  • @bhspencer - So you think this is a good fit for aspect-oriented programming? Double negation, hard to follow, I think you confused yourself. – Nicolas Barbulesco Jul 01 '15 at 21:55
  • @Amir - The comment just before yours, by bhspencer, is an elegant solution in my opinion. – Nicolas Barbulesco Jul 01 '15 at 22:52
  • @Brian - Can you develop on the annotation solution? – Nicolas Barbulesco Jul 01 '15 at 22:55
  • 1
    @NicolasBarbulesco Annotation processors, which run during compilation, can gain access to the trees used by the compiler to represent programs. So an annotation processor can inspect the syntax tree of the program being compiled and do some tree-similarity matching against the desired prologue code. APs can all feed diagnostics back into the compiler output, so they can issue warnings or errors when the tree-similarity match fails. – Brian Goetz Jul 01 '15 at 23:13
  • @NicolasBarbulesco I agree my comment was ambiguous. I should have written: I don't think that the enabled-ness of a class is a "cross cutting concern" and is therefore not a good fit for AOP. – bhspencer Jul 02 '15 at 13:11
  • @NicolasBarbulesco can you really not glean from that statement that the enabled-ness of a class is not a good fit for AOP? – bhspencer Jul 02 '15 at 16:30

14 Answers14

93

I don't know about elegant, but here is a working implementation using Java's built-in java.lang.reflect.Proxy that enforces that all method invocations on Foo begin by checking the enabled state.

main method:

public static void main(String[] args) {
    Foo foo = Foo.newFoo();
    foo.setEnabled(false);
    foo.bar(); // won't print anything.
    foo.setEnabled(true);
    foo.bar(); // prints "Executing method bar"
}

Foo interface:

public interface Foo {
    boolean getEnabled();
    void setEnabled(boolean enable);

    void bar();
    void baz();
    void bat();

    // Needs Java 8 to have this convenience method here.
    static Foo newFoo() {
        FooFactory fooFactory = new FooFactory();
        return fooFactory.makeFoo();
    }
}

FooFactory class:

import java.lang.reflect.InvocationHandler;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;

public class FooFactory {

    public Foo makeFoo() {
        return (Foo) Proxy.newProxyInstance(
                this.getClass().getClassLoader(),
                new Class[]{Foo.class},
                new FooInvocationHandler(new FooImpl()));
    }

    private static class FooImpl implements Foo {
        private boolean enabled = false;

        @Override
        public boolean getEnabled() {
            return this.enabled;
        }

        @Override
        public void setEnabled(boolean enable) {
            this.enabled = enable;
        }

        @Override
        public void bar() {
            System.out.println("Executing method bar");
        }

        @Override
        public void baz() {
            System.out.println("Executing method baz");
        }

        @Override
        public void bat() {
            System.out.println("Executing method bat");
        }

    }

    private static class FooInvocationHandler implements InvocationHandler {

        private FooImpl fooImpl;

        public FooInvocationHandler(FooImpl fooImpl) {
            this.fooImpl = fooImpl;
        }

        @Override
        public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
            if (method.getDeclaringClass() == Foo.class &&
                !method.getName().equals("getEnabled") &&
                !method.getName().equals("setEnabled")) {

                if (!this.fooImpl.getEnabled()) {
                    return null;
                }
            }

            return method.invoke(this.fooImpl, args);
        }
    }
}

As others have pointed out, it does seem like overkill for what you need if you only have a handful of methods to worry about.

That said, there certainly are benefits:

  • A certain separation of concerns is achieved, because Foo's method implementations don't have to worry about the enabled check cross-cutting concern. Instead, the method's code only needs to worry about what the method's primary purpose is, nothing more.
  • There is no way for an innocent developer to add a new method to the Foo class and mistakenly "forget" to add the enabled check. The enabled check behavior is automatically inherited by any newly added method.
  • If you need to add another cross-cutting concern, or if you need to enhance the enabled check, it's very easy to do so safely and in one place.
  • It is kind of nice that you can get this AOP-like behavior with built-in Java functionality. You are not forced into having to integrate some other framework like Spring, though they can definitely be good options too.

To be fair, some of the downsides are:

  • Some of the implementation code that handles the proxy invocations is ugly. Some would also say that having inner classes to prevent instantiation of the FooImpl class is ugly.
  • If you want to add a new method to Foo, you have to make a change in 2 spots: the implementation class and the interface. Not a big deal, but it's still a bit more work.
  • Proxy invocations are not free. There is a certain performance overhead. For general use though, it won't be noticeable. See here for more information.

EDIT:

Fabian Streitel's comment got me thinking about 2 annoyances with my above solution that, I'll admit, I'm not happy about myself:

  1. The invocation handler uses magic strings to skip the "enabled-check" on the "getEnabled" and "setEnabled" methods. This can easily break if the method names are refactored.
  2. If there was a case where new methods need to be added that should not inherit the "enabled-check" behavior, then it can be pretty easy for the developer to get this wrong, and at the very least, it would mean adding more magic strings.

To resolve point #1, and to at least ease the problem with point #2, I would create an annotation BypassCheck (or something similar) that I could use to mark the methods in the Foo interface for which I don't want to perform the "enabled check". This way, I don't need magic strings at all, and it becomes a lot easier for a developer to correctly add a new method in this special case.

Using the annotation solution, the code would look like this:

main method:

public static void main(String[] args) {
    Foo foo = Foo.newFoo();
    foo.setEnabled(false);
    foo.bar(); // won't print anything.
    foo.setEnabled(true);
    foo.bar(); // prints "Executing method bar"
}

BypassCheck annotation:

import java.lang.annotation.*;

@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
public @interface BypassCheck {
}

Foo interface:

public interface Foo {
    @BypassCheck boolean getEnabled();
    @BypassCheck void setEnabled(boolean enable);

    void bar();
    void baz();
    void bat();

    // Needs Java 8 to have this convenience method here.
    static Foo newFoo() {
        FooFactory fooFactory = new FooFactory();
        return fooFactory.makeFoo();
    }
}

FooFactory class:

import java.lang.reflect.InvocationHandler;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;

public class FooFactory {

    public Foo makeFoo() {
        return (Foo) Proxy.newProxyInstance(
                this.getClass().getClassLoader(),
                new Class[]{Foo.class},
                new FooInvocationHandler(new FooImpl()));
    }

    private static class FooImpl implements Foo {

        private boolean enabled = false;

        @Override
        public boolean getEnabled() {
            return this.enabled;
        }

        @Override
        public void setEnabled(boolean enable) {
            this.enabled = enable;
        }

        @Override
        public void bar() {
            System.out.println("Executing method bar");
        }

        @Override
        public void baz() {
            System.out.println("Executing method baz");
        }

        @Override
        public void bat() {
            System.out.println("Executing method bat");
        }

    }

    private static class FooInvocationHandler implements InvocationHandler {

        private FooImpl fooImpl;

        public FooInvocationHandler(FooImpl fooImpl) {
            this.fooImpl = fooImpl;
        }

        @Override
        public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
            if (method.getDeclaringClass() == Foo.class
                    && !method.isAnnotationPresent(BypassCheck.class) // no magic strings
                    && !this.fooImpl.getEnabled()) {

                return null;
            }

            return method.invoke(this.fooImpl, args);
        }
    }
}
Community
  • 1
  • 1
sstan
  • 35,425
  • 6
  • 48
  • 66
  • 11
    I get that this is a clever solution but would you really use this? – bhspencer Jun 29 '15 at 18:03
  • 1
    is a nice workaround, you are using the dynamic proxy pattern to decorate the object with this common behavior found at the start of each method. – Victor Jun 29 '15 at 18:07
  • 12
    @bhspencer: A very legitimate question. I have actually used it many times to do exception handling, logging, transaction handling, etc. I'll admit that for smaller classes, it seems like overkill, and may very well be. But if I expect the class to grow a lot in complexity, and want to ensure consistent behavior across all methods as I do that, I don't mind this solution. – sstan Jun 29 '15 at 18:10
  • 1
    Not to be part of the 97% of evil here, but what are the performance implications of this proxy class? – corsiKa Jun 29 '15 at 21:29
  • 5
    @corsiKa: Good question. There is no doubt that using dynamic proxies is slower than direct method calls. For general use though, the performance overhead will be imperceptible. Related SO thread if you're interested: [Performance cost of Java dynamic proxy](http://stackoverflow.com/questions/1856242/performance-cost-of-java-dynamic-proxy) – sstan Jun 29 '15 at 23:51
  • 1
    To make the implementation of the `InvocationHandler` simpler you should separate the get/setFooEnabled methods from the `Foo` interface, because in OPs code they are also an implementation detail. So this saves you the check if that method is actually called on the proxy. – SpaceTrucker Jun 30 '15 at 10:50
  • 1
    @corsiKa You could start with the proxy and if you find that a proxy to a specific implementation is slowing your app down, you can always implement the interface with the checks put in and just wire up your app to use that implementation. – SpaceTrucker Jun 30 '15 at 10:52
  • @SpaceTrucker: Depends on who OP expects will be setting the `enabled` flag. I assumed that it would be the consumer of the `Foo` class. But if not, then I agree with your suggestion + maybe it also means that `FooImpl` doesn't need to be a private class. Thanks for the feedback. – sstan Jun 30 '15 at 12:05
  • @SpaceTrucker For sure. That would be following best practices and not premature-optimizing as my comment implied. I was just curious (for those too lazy to click the link, the cost is almost negligible, apparently...) – corsiKa Jun 30 '15 at 14:02
  • I think a big downside would be confusing the programmer that will read this. – MaKri Jul 01 '15 at 10:59
  • Plus it's not "maintenance proof". just adding another simple getter/setter that's not supposed to be guarded will result in broken code unless you remember to add it to the check in the invocation handler – Fabian Streitel Jul 02 '15 at 07:21
  • 1
    @Fabian: You make a good point. Making changes that are _not_ supposed to inherit the default "enabled-check" behavior do require more care. At that point, I might consider having an annotation that defines which methods opt-out of the default behavior, and have the invocation handler check for the presence of the annotation on a method rather than checking for specific method names. It would probably be easier for the maintainer to get the change right that way. In any case, I edited out the "maintenance proof" part, because you're right, that would be a false promise. – sstan Jul 02 '15 at 17:33
51

There is a lot of good suggestions.. what you can do to strike your problem is think in the State Pattern and implement it.

Take a look at this code snippet.. perhaps it will get you to an idea. In this scenario looks like you want to modify the entire methods implementation based on the internal state of the object. Please recall that the sum of the methods in a object is knows as behavior.

public class Foo {

      private FooBehaviour currentBehaviour = new FooEnabledBehaviour (); // or disabled, or use a static factory method for getting the default behaviour

      public void bar() {
        currentBehaviour.bar();
      }
      public void baz() {
        currentBehaviour.baz();
      }
      public void bat() {
        currentBehaviour.bat();
      }

      public void setFooEnabled (boolean fooEnabled) { // when you set fooEnabel, you are changing at runtime what implementation will be called.
        if (fooEnabled) {
          currentBehaviour = new FooEnabledBehaviour ();
        } else {
          currentBehaviour = new FooDisabledBehaviour ();
        }
      }

      private interface FooBehaviour {
        public void bar();
        public void baz();
        public void bat();
      }

      // RENEMBER THAT instance method of inner classes can refer directly to instance members defined in its enclosing class
      private class FooEnabledBehaviour implements FooBehaviour {
        public void bar() {
          // do what you want... when is enabled
        }
        public void baz() {}
        public void bat() {}

      }

      private class FooDisabledBehaviour implements FooBehaviour {
        public void bar() {
          // do what you want... when is desibled
        }
        public void baz() {}
        public void bat() {}

      }
}

Hope you like it!

P.D: Is an implementation of the State Pattern (also knows as Strategy depending on the context.. but the principles are just the same).

Victor
  • 3,841
  • 2
  • 37
  • 63
  • 2
    OP wants not having to repeat the same line of code at the beginning of every method and your solution involves repeating the same line of code at the beginning of every method. – Tulains Córdova Jun 29 '15 at 19:14
  • 2
    @user1598390 there is no need of repeting the evalution, inside the FooEnabledBehaviour you are assuming that the client of this object has set fooEnabled to true, so there is no need of cheking. The same goes to FooDisabledBehaviour class. Check again, code in it. – Victor Jun 29 '15 at 19:21
  • 2
    Thanks @bayou.io, let's wait until the OP answer. I think that the community is doing a hell of a job here, there is a lot of good tips around here! – Victor Jun 29 '15 at 19:21
  • Compared to the top answer, I guess this one would actually be expensive on a big class. Good example for a small one though. – dyesdyes Jun 30 '15 at 08:26
  • 2
    Agreed with @dyesdyes I can't imagine implementing this for anything but a really trivial class. It's just too problematic, considering that `bar()` in `FooEnabledBehavior` and `bar()` in `FooDisabledBehavior` might share a lot of the same code, with perhaps even a single line different between the two. You could very easily, especially if this code were to be maintained by junior devs (such as myself), end up with a giant mess of crap that is unmaintainable and untestable. That can happen with any code, but this just seems so easy to screw up really fast. +1 though, because good suggestion. – Chris Cirefice Jul 01 '15 at 14:18
  • 1
    Mmmmm... i don't guys.. but first, thanks for comments. For me the size of the code is not a problem as long as it is "clean" and "readable". In my favor i should argue that i am not using any external class.. that should make things more accessible. And if there is some common behavior, let's encapsulate it in a CommonBehaviourClass and delegate to where it need. In the GOF book (not my favorite book for learning, but has good recipes) you found this example : https://en.wikipedia.org/wiki/Talk:State_pattern#TCPConnection_class_the_GoF_book. Is more or less the same thing i do here. – Victor Jul 01 '15 at 14:59
  • 1
    This would be my preferred way of doing this. Nice answer! – Ky - Jul 23 '15 at 19:33
  • @ChrisCirefice The OP's disabled 'behavior' is to return without doing anything, so why would it share any code with enabled behavior? – Ky - Jul 23 '15 at 19:35
14

Yes, but it's a bit of work, so it depends how important it is to you.

You can define the class as an interface, write a delegate implementation, and then use java.lang.reflect.Proxy to implement the interface with methods that do the shared portion and then conditionally call the delegate.

interface Foo {
    public void bar();
    public void baz();
    public void bat();
}

class FooImpl implements Foo {
    public void bar() {
      //... <-- your logic represented by this notation above
    }

    public void baz() {
      //... <-- your logic represented by this notation above
    }

    // and so forth
}

Foo underlying = new FooImpl();
InvocationHandler handler = new MyInvocationHandler(underlying);
Foo f = (Foo) Proxy.newProxyInstance(Foo.class.getClassLoader(),
     new Class[] { Foo.class },
     handler);

Your MyInvocationHandler can look something like this (error handling and class scaffolding omitted, assuming fooIsEnabled is defined somewhere accessible):

public Object invoke(Object proxy, Method method, Object[] args) {
    if (!fooIsEnabled) return null;
    return method.invoke(underlying, args);
}

It's not incredibly pretty. But unlike various commenters, I'd do it, as I think repetition is a more important risk than this kind of density, and you'll be able to produce the "feel" of your real class, with this somewhat inscrutable wrapper added on very locally in just a couple of lines of code.

See the Java documentation for details on dynamic proxy classes.

David P. Caldwell
  • 3,394
  • 1
  • 19
  • 32
14

This question is closely related to aspect-oriented programming. AspectJ is an AOP extension of Java and you may give it a look to get some ispiration.

As far as I know there is no direct support for AOP in Java. There are some GOF patterns that relate to it, like for instance Template Method and Strategy but it will not really save you lines of code.

In Java and most other languages you could define the recurrent logic you need in functions and adopt a so-called disciplined coding approach in which you call them at the right time.

public void checkBalance() {
    checkSomePrecondition();
    ...
    checkSomePostcondition();
}

However this would not fit your case because you would like the factored-out code to be able to return from checkBalance. In languages that support macros (like C/C++) you could define checkSomePrecondition and checkSomePostcondition as macros and they would simply be replaced by the preprocessor before the compiler is even invoked:

#define checkSomePrecondition \
    if (!fooIsEnabled) return;

Java does not have this out of the box. This may offend someone but I did use automatic code generation and template engines to automate repetitive coding tasks in the past. If you process your Java files before compiling them with a suitable preprocessor, for instance Jinja2, you could do something similar to what is possible in C.

Possible pure Java approach

If you are looking for a pure Java solution, what you may find is probably not going to be concise. But, it could still factor out common parts of your program and avoid code duplication and bugs. You could do something like this (it's some sort of Strategy-inspired pattern). Note that in C# and Java 8, and in other languages in which functions are a little easier to handle, this approach may actually look nice.

public interface Code {
    void execute();
}

...

public class Foo {
  private bool fooIsEnabled;

  private void protect(Code c) {
      if (!fooIsEnabled) return;
      c.execute();
  }

  public void bar() {
    protect(new Code {
      public void execute() {
        System.out.println("bar");
      }
    });
  }

  public void baz() {
    protect(new Code {
      public void execute() {
        System.out.println("baz");
      }
    });
  }

  public void bat() {
    protect(new Code {
      public void execute() {
        System.out.println("bat");
      }
    });
  }
}

Kinda of a real-world scenario

You are developing a class to send data frames to an industrial robot. The robot takes time to complete a command. Once the command is completed, it sends you a control frame back. The robot may get damaged if it receives a new command while the previous is still being executed. Your program uses a DataLink class to send and receive frames to and from the robot. You need to protect access to the DataLink instance.

The user interface thread calls RobotController.left, right, up or down when the user clicks the buttons, but also calls BaseController.tick at regular intervals, in order to reenable command forwarding to the private DataLink instance.

interface Code {
    void ready(DataLink dataLink);
}

class BaseController {
    private DataLink mDataLink;
    private boolean mReady = false;
    private Queue<Code> mEnqueued = new LinkedList<Code>();

    public BaseController(DataLink dl) {
        mDataLink = dl;
    }

    protected void protect(Code c) {
        if (mReady) {
            mReady = false;
            c.ready(mDataLink);
        }
        else {
            mEnqueue.add(c);
        }
    }

    public void tick() {
        byte[] frame = mDataLink.readWithTimeout(/* Not more than 50 ms */);

        if (frame != null && /* Check that it's an ACK frame */) {
          if (mEnqueued.isEmpty()) {
              mReady = true;
          }
          else {
              Code c = mEnqueued.remove();
              c.ready(mDataLink);
          }
        }
    }
}

class RobotController extends BaseController {
    public void left(float amount) {
        protect(new Code() { public void ready(DataLink dataLink) {
            dataLink.write(/* Create a byte[] that means 'left' by amount */);
        }});
    }

    public void right(float amount) {
        protect(new Code() { public void ready(DataLink dataLink) {
            dataLink.write(/* Create a byte[] that means 'right' by amount */);
        }});
    }

    public void up(float amount) {
        protect(new Code() { public void ready(DataLink dataLink) {
            dataLink.write(/* Create a byte[] that means 'up' by amount */);
        }});
    }

    public void down(float amount) {
        protect(new Code() { public void ready(DataLink dataLink) {
            dataLink.write(/* Create a byte[] that means 'down' by amount */);
        }});
    }
}
damix911
  • 4,165
  • 1
  • 29
  • 44
  • 4
    Doesn't this just kick the can down the road. Which is to say the future maintainer used to have to remember to if (!fooIsEnabled) return; at the start of every function and now they need to remember to protect(new Code {... at the start of every function. How does this help? – bhspencer Jun 29 '15 at 17:59
  • 1
    I like you analysis and background context damix911... i would build the new Code instances at compile time (using private static members) assuming that the code won't change over time and rename the change to "executeIf" passing as argument a Condition (Like Predicate class) and the Code. But that is more personal felling and taste. – Victor Jun 29 '15 at 18:59
  • 1
    @bhspencer It looks rather clumsy in Java, and in most cases this strategy is really an overengineering of otherwise simple code. Not many programs can benefit from such a pattern. A cool thing however is that we created a new symbol `protect` which is easier to reuse and document. If you tell the future maintainer that critical code should be protected with `protect`, you are already telling him what to do. If the protection rules change, the new code is still protected. This is exactly the rationale behind defining functions but the OP needed to "return a `return`" which functions can't do. – damix911 Jul 03 '15 at 13:40
11

I would consider refactoring. This pattern is heavily breaking DRY pattern (Don't repeat yourself). I believe this break this class responsibility. But this depends on your control of code. Your question is very open - where are you calling Foo instance?

I suppose you have code like

foo.bar(); // does nothing if !fooEnabled
foo.baz(); // does also nothing
foo.bat(); // also

maybe you should call it something like this way:

if (fooEnabled) {
   foo.bat();
   foo.baz();
   ...
}

And keep it clean. For example, logging:

this.logger.debug(createResourceExpensiveDump())

a logger is not asking himself, if debug is enabled. It just logs.

Instead, calling class need to check this:

if (this.logger.isDebugEnabled()) {
   this.logger.debug(createResourceExpensiveDump())
}

If this is a library and you cannot control calling of this class, throw an IllegalStateException which explains why, if this calling is illegal and cause trouble.

Gondy
  • 4,925
  • 4
  • 40
  • 46
  • Ey Downvoters.. Gondy just share an idea... and a very good one. The question is very open. Please consider to leave comments if you know how to improve. For this case, Gondy and Kristina, i guess that the state pattern will works well. – Victor Jun 29 '15 at 17:50
  • 6
    It's definitely simpler and easier on the eyes. But if OP's goal is to make sure that, as new methods are added, that the enabled logic is never bypassed, then this refactoring does not make it easier to enforce that. – sstan Jun 29 '15 at 17:57
  • 4
    Also for your log example, I would say this involves a lot more repetition - every time you want to log, you have to check the logger is enabled. I tend to log more lines than the number of methods on any class... – T. Kiley Jun 30 '15 at 11:49
  • 4
    This breaks modularity, because now the caller has to know something about the internals of foo (in this case whether it is fooEnabled). This is a classic example where following the best practice rules won't solve the problem because the rules conflict. (I'm still hoping someone will come up with a "why didn't I think of that?" answer that.) – Ian Goldby Jun 30 '15 at 12:35
  • Ian that is true. But for me, it does not make sense, when foo methods does nothing without throwing exception or any feedback. – Gondy Jun 30 '15 at 12:38
  • 2
    Well, it depends heavily on the context as to whether it makes sense. – Ian Goldby Jun 30 '15 at 12:42
  • 2
    This takes the logic of how a Foo behaves outside of the Foo class. That seems all wrong.... – GreenAsJade Jul 01 '15 at 10:33
  • 3
    Logging is exactly the example, where I want no repetition in my code. I just want to write LOG.debug("...."); - And the logger should check if I really want to debug. -- Another example is closing/cleanup. - If I use AutoClosable I don't want an exception if it is already closed, it should just do nothing. – Falco Jul 01 '15 at 11:11
  • Gondy, a method can do nothing without throwing any exception, this can make sense. For example, we have a manager of connections: if `connectionManager` is not *enabled*, then `connectionManager.closeAllConnections()` does nothing because there is nothing to close. – Nicolas Barbulesco Jul 04 '15 at 00:09
  • I fully agree with @Falco regarding logging. And `LOG.debug("…")` is still too long; `debug("…")` should be enough. – Nicolas Barbulesco Jul 04 '15 at 00:20
6

IMHO the most elegant and best performing solution to this is to have more than one implementation of Foo, together with a factory method for creating one:

class Foo {
  protected Foo() {
    // Prevent direct instantiation
  }

  public void bar() {
    // Do something
  }

  public static void getFoo() {
    return fooEnabled ? new Foo() : new NopFoo();
  }
}

class NopFoo extends Foo {
  public void bar() {
    // Do nothing
  }
}

Or a variation:

class Foo {
  protected Foo() {
    // Prevent direct instantiation
  }

  public void bar() {
    // Do something
  }

  public static void getFoo() {
    return fooEnabled ? new Foo() : NOP_FOO;
  }

  private static Foo NOP_FOO = new Foo() {
    public void bar() {
      // Do nothing
    }
  };
}

As sstan points out, even better would be to use an interface:

public interface Foo {
  void bar();

  static Foo getFoo() {
    return fooEnabled ? new FooImpl() : new NopFoo();
  }
}

class FooImpl implements Foo {
  FooImpl() {
    // Prevent direct instantiation
  }

  public void bar() {
    // Do something
  }
}

class NopFoo implements Foo {
  NopFoo() {
    // Prevent direct instantiation
  }

  public void bar() {
    // Do nothing
  }
}

Adapt this to the rest of your circumstances (are you creating a new Foo every time or reusing the same instance, etc.)

Pepijn Schmitz
  • 2,143
  • 1
  • 17
  • 18
  • 1
    This is not the same as Konrad's answer. I like this, but I think it would be safer if, instead of using class inheritance, you used an interface as others have suggested in their answers. The reason is simple: It is too easy for a developer to add a method to `Foo`, and forget to add the no-op version of the method in the extended class, thus bypassing the desired behavior. – sstan Jun 30 '15 at 12:38
  • 2
    @sstan You're right, that would be better. I did it like this to modify kristina's original example as little as possible in order to avoid distractions, but this is relevant. I'll add your suggestion to my answer. – Pepijn Schmitz Jun 30 '15 at 12:59
  • 1
    I think you miss a point. You determine whether `isFooEnabled` at the instantiation of `Foo`. It is too early. In the original code, this is done when a method is run. The value of `isFooEnabled` can change in the meantime. – Nicolas Barbulesco Jul 04 '15 at 09:40
  • 1
    @NicolasBarbulesco You don't know that, fooIsEnabled could be a constant. Or it could be used in a way that makes it effectively constant. Or it could be set in a few well defined places so that it would be easy to get a new instance of Foo every time. Or it could be acceptable to get a new instance of Foo every time it is used. You don't know, that's why I wrote: "adapt this to the rest of your circumstances." – Pepijn Schmitz Jul 04 '15 at 09:52
  • @PepijnSchmitz - Of course, `fooIsEnabled` *may* be constant. But nothing tells us that it be constant. So I consider the general case. – Nicolas Barbulesco Jul 04 '15 at 10:30
5

I have another approach: have a

interface Foo {
  public void bar();
  public void baz();
  public void bat();
}

class FooImpl implements Foo {
  public void bar() {
    //...
  }
  public void baz() {
    //...
  }
  public void bat() {
    //...
  }
}

class NullFoo implements Foo {
  static NullFoo DEFAULT = new NullFoo();
  public void bar() {}
  public void baz() {}
  public void bat() {}
}

}

and then you can do

(isFooEnabled ? foo : NullFoo.DEFAULT).bar();

Maybe you can even replace the isFooEnabled with a Foo variable which either holds the FooImpl to be used or the NullFoo.DEFAULT. Then the call is simpler again:

Foo toBeUsed = isFooEnabled ? foo : NullFoo.DEFAULT;
toBeUsed.bar();
toBeUsed.baz();
toBeUsed.bat();

BTW, this is called the "Null pattern".

glglgl
  • 89,107
  • 13
  • 149
  • 217
  • The overal approach is good, but using the expression `(isFooEnabled ? foo : NullFoo.DEFAULT).bar();` seems a bit clumsy. Have a third implementation that delegates to one of the existing implementations. Instead of changing the value of a field `isFooEnabled` the target of delegation could be changed. This reduces the number of branches in the code – SpaceTrucker Jun 30 '15 at 09:27
  • 1
    But you are deporting the internal cooking of the class `Foo` in the calling code! How could we know whether `isFooEnabled`? This is an *internal* field in the class `Foo`. – Nicolas Barbulesco Jul 04 '15 at 00:49
3

In a similar functional approach to @Colin's answer, with Java 8's lambda functions, it is possible to wrap the conditional feature toggle enable / disable code into a guard method (executeIfEnabled) which accepts the action lambda, to which code to be conditionally executed can be passed.

Although in your case, this approach won't save any lines of code, by DRYing this up, you now have the option to centralize other feature toggle concerns, plus AOP or debugging concerns like logging, diagnostics, profiling et al.

One benefit of using lambdas here is that closures can be used to avoid the need to overload the executeIfEnabled method.

For example:

class Foo {
    private Boolean _fooIsEnabled;

    public Foo(Boolean isEnabled) {
        _fooIsEnabled = isEnabled;
    }

    private void executeIfEnabled(java.util.function.Consumer someAction) {
        // Conditional toggle short circuit
        if (!_fooIsEnabled) return;

        // Invoke action
        someAction.accept(null);
    }

    // Wrap the conditionally executed code in a lambda
    public void bar() {
        executeIfEnabled((x) -> {
            System.out.println("Bar invoked");
        });
    }

    // Demo with closure arguments and locals
    public void baz(int y) {
        executeIfEnabled((x) -> {
            System.out.printf("Baz invoked %d \n", y);
        });
    }

    public void bat() {
        int z = 5;
        executeIfEnabled((x) -> {
            System.out.printf("Bat invoked %d \n", z);
        });
    }

With a test:

public static void main(String args[]){
    Foo enabledFoo = new Foo(true);
    enabledFoo.bar();
    enabledFoo.baz(33);
    enabledFoo.bat();

    Foo disabledFoo = new Foo(false);
    disabledFoo.bar();
    disabledFoo.baz(66);
    disabledFoo.bat();
}
StuartLC
  • 104,537
  • 17
  • 209
  • 285
  • Also similar to Damix's approach, without the need for an interface and anonymous class implementations with method override. – StuartLC Jul 17 '15 at 10:12
2

As is pointed out in other answers, the Strategy Design Pattern is an appropriate design pattern to follow to simplify this code. I've illustrated it here using method invocation through reflection, but there are any number of mechanisms that you could use to get the same effect.

class Foo {

  public static void main(String[] args) {
      Foo foo = new Foo();
      foo.fooIsEnabled = false;
      foo.execute("bar");
      foo.fooIsEnabled = true;
      foo.execute("baz");
  }

  boolean fooIsEnabled;

  public void execute(String method) {
    if(!fooIsEnabled) {return;}
    try {
       this.getClass().getDeclaredMethod(method, (Class<?>[])null).invoke(this, (Object[])null);
    }
    catch(Exception e) {
       // best to handle each exception type separately
       e.printStackTrace();
    }
  }

  // Changed methods to private to reinforce usage of execute method
  private void bar() {
    System.out.println("bar called");
    // bar stuff here...
  }
  private void baz() {
    System.out.println("baz called");
    // baz stuff here...
  }
  private void bat() {
    System.out.println("bat called");
    // bat stuff here...
  }
}
LJ2
  • 593
  • 5
  • 11
  • Having to deal with reflection is a bit awkward, if there are already classes that do this for you, like the already mentioned `Proxy`. – SpaceTrucker Jun 30 '15 at 09:30
  • How could you do `foo.fooIsEnabled ...` ? A priori, this is an internal field of the object, we cannot, and do not want to, see it outside. – Nicolas Barbulesco Jul 04 '15 at 10:05
2

If only java was a bit better at being functional. It think the most OOO solution is to create class that wraps a single function so it is called only when foo is enabled.

abstract class FunctionWrapper {
    Foo owner;

    public FunctionWrapper(Foo f){
        this.owner = f;
    }

    public final void call(){
        if (!owner.isEnabled()){
            return;
        }
        innerCall();
    }

    protected abstract void innerCall();
}

and then implement bar, baz and bat as anonymous classes that that extend FunctionWrapper.

class Foo {
    public boolean fooIsEnabled;

    public boolean isEnabled(){
        return fooIsEnabled;
    }

    public final FunctionWrapper bar = new FunctionWrapper(this){
        @Override
        protected void innerCall() {
            // do whatever
        }
    };

    public final FunctionWrapper baz = new FunctionWrapper(this){
        @Override
        protected void innerCall() {
            // do whatever
        }
    };

    // you can pass in parms like so 
    public final FunctionWrapper bat = new FunctionWrapper(this){
        // some parms:
        int x,y;
        // a way to set them
        public void setParms(int x,int y){
            this.x=x;
            this.y=y;
        }

        @Override
        protected void innerCall() {
            // do whatever using x and y
        }
    };
}

Another Idea

Use glglgl's nullable solution but make FooImpl and NullFoo inner classes (with private constructors) of the below class:

class FooGateKeeper {

    public boolean enabled;

    private Foo myFooImpl;
    private Foo myNullFoo;

    public FooGateKeeper(){
        myFooImpl= new FooImpl();
        myNullFoo= new NullFoo();
    }

    public Foo getFoo(){
        if (enabled){
            return myFooImpl;
        }
        return myNullFoo;
    }  
}

this way you don't have to worry about remembering to use (isFooEnabled ? foo : NullFoo.DEFAULT).

Community
  • 1
  • 1
Colin
  • 588
  • 6
  • 9
1

It seems like the class does nothing when Foo is not enabled so why not express this at a higher level where you create or get the Foo instance?

class FooFactory
{
 static public Foo getFoo()
 {
   return isFooEnabled ? new Foo() : null;
 }
}
 ...
 Foo foo = FooFactory.getFoo();
 if(foo!=null)
 {
   foo.bar();
   ....
 }     

This only works if isFooEnabled is a constant though. In a general case, you could create your own annotation.

Konrad Höffner
  • 11,100
  • 16
  • 60
  • 118
1

I am not familiar with Java syntax. Assumption that in Java, there is polymorphism, static property, abstract class & method:

    public static void main(String[] args) {
    Foo.fooIsEnabled = true; // static property, not particular to a specific instance  

    Foo foo = new bar();
    foo.mainMethod();

    foo = new baz();
    foo.mainMethod();

    foo = new bat();
    foo.mainMethod();
}

    public abstract class Foo{
      static boolean fooIsEnabled;

      public void mainMethod()
      {
          if(!fooIsEnabled)
              return;

          baMethod();
      }     
      protected abstract void baMethod();
    }
    public class bar extends Foo {
        protected override baMethod()
        {
            // bar implementation
        }
    }
    public class bat extends Foo {
        protected override baMethod()
        {
            // bat implementation
        }
    }
    public class baz extends Foo {
        protected override baMethod()
        {
            // baz implementation
        }
    }
ehh
  • 3,412
  • 7
  • 43
  • 91
1

Basically you have a flag that if it's set, the function call should be skipped. So I think my solution would be silly, but here it is.

Foo foo = new Foo();

if (foo.isEnabled())
{
    foo.doSomething();
}

Here's the implementation of a simple Proxy, in case you want to execute some code before executing any function.

class Proxy<T>
{
    private T obj;
    private Method<T> proxy;

    Proxy(Method<T> proxy)
    {
        this.ojb = new T();
        this.proxy = proxy;
    }

    Proxy(T obj, Method<T> proxy)
    {
        this.obj = obj;
        this.proxy = proxy;
    }

    public T object ()
    {
        this.proxy(this.obj);
        return this.obj;
    }
}

class Test
{
    public static void func (Foo foo)
    {
        // ..
    }

    public static void main (String [] args)
    {
        Proxy<Foo> p = new Proxy(Test.func);

        // how to use
        p.object().doSomething();
    }
}

class Foo
{
    public void doSomething ()
    {
        // ..
    }
}
Khaled.K
  • 5,828
  • 1
  • 33
  • 51
0

There is another solution, using delegate (pointer to function). You can have a unique method that first is doing the validation and then is calling to the relevant method according to the function (parameter) to be called. C# code:

internal delegate void InvokeBaxxxDelegate();

class Test
{
    private bool fooIsEnabled;

    public Test(bool fooIsEnabled)
    {
        this.fooIsEnabled = fooIsEnabled;
    }

    public void Bar()
    {
        InvokeBaxxx(InvokeBar);
    }

    public void Baz()
    {
        InvokeBaxxx(InvokeBaz);
    }

    public void Bat()
    {
        InvokeBaxxx(InvokeBat);
    }

    private void InvokeBaxxx(InvokeBaxxxDelegate invoker)
    {
        if (!fooIsEnabled) return;
        invoker();
    }

    private void InvokeBar()
    {
        // do Invoke bar stuff
        Console.WriteLine("I am Bar");
    }

    private void InvokeBaz()
    {
        // do Invoke bar stuff
        Console.WriteLine("I am Baz");
    }

    private void InvokeBat()
    {
        // do Invoke bar stuff
        Console.WriteLine("I am Bat");
    }
}
ehh
  • 3,412
  • 7
  • 43
  • 91
  • 2
    Correct, it is marked Java and this why I emphasize and wrote "Code in C#" since I don't know Java. Since it is a Design Pattern question so the language is not important. – ehh Jun 30 '15 at 17:23
  • Oh! I understand, sorry for that, only tried to help and find a solution. Thank you – ehh Jun 30 '15 at 19:02