views:

695

answers:

8

I have this interface:

public interface Command<T> {
    T execute(String... args);
}

it works fine for most uses. But when I try to model a command that have only side effects (e.g. without return value) I'm tempted to write:

public class SideEffectCommand implements Command<Void> {

    @Override
    public Void execute(String... args) {
        return null; // null is fine?
    }
}

Is this a common problem? Are there best practices to model Commands with and without return value?

I've tried with this adapter but I think this is not optimal for several reasons:

public abstract class VoidCommand implements Command<Void> {

    @Override
    public Void execute(String... args) {
       execute2(args);
       return null;
    }

    public abstract void execute2(String... args);
}
+2  A: 

Keep the interface as it is: this is why Void is in the Standard Library. Just as long as whatever is calling the Command expects nulls to come back out.

And yes, null is the only value you can return for Void.

Michael Deardeuff
I think the main reasson for Void is to represent the void return type of methods (reflection). At least back to JDK 1.1 when it was included.
Carlos Heuberger
Yeah, it predates generics. Indeed, it's use in generics is only a convention, albeit an obvious one.
Tom Hawtin - tackline
+2  A: 

What is interesting in your example is the use of parameterized types. Usually you'd have

interface Command<T> {
    public T execute(T someObject);
}

In your case you only have T as a return value. Nevertheless using Void in this case is a good solution. The return value should be null.

Marcin
+1  A: 

That problem is not that common, but neither that rare... I think I've seen a discussion about that some time ago, about Callables that return nothing.

I agree with the other posters that this is a good solution, much better than using Object or some other dummy placeholder.

Carlos Heuberger
can you link the discussion please?
dfa
+9  A: 

I would stick to using Void explicitly. It's easy to see what is going on without another class involved. It'd be nice if you could override a Void return with void (and Integer with int, etc), but that is not a priority.

Tom Hawtin - tackline
Reading the answers i have to say that this is the only one that actually engages the problem instead of commenting on life in general.
Stroboskop
+1 for this answer. Don't get hung-up on Java's type system being a bit awkward with the interplay between Object-based and primitive types.
hbunny
This post doesn't address what happens when code expecting a return happens upon a Command that returns null.
Kelly French
+4  A: 

This is not a common problem. The issue you need to address is the expectation of your interface. You're combining the behavior of a non-side effect interface with an interface that allows side effects.

Consider this:

public class CommandMonitor {

    public static void main(String[] args) {  
     Command<?> sec = new SideEffectCommand();  
     reportResults(sec);  
    } 

    public static void reportResults(Command<?> cmd){

     final Object results = cmd.execute("arg");
     if (results != null ) {
      System.out.println(results.getClass());
     }
    }
}

There is nothing wrong with using <Void> as the template type but allowing it to mix with implementations for "Command<T>" means some clients of the interface may not expect a void result. Without changing the interface, you've allowed an implementation to create an unexpected result.

When we pass around sets of data using the Collection classes, my team agreed to never return null even though it's fine syntactically. The problem was that classes that used the return values would constantly have to check for a void to prevent a NPE. Using the code above, you would see this everywhere:

 if (results != null ){

Because there is now way to know whether the implementation actually has an object or is null. For a specific case, sure you'd know because your familiar with the implementation. But as soon as you start to aggregate them or they pass beyond your coding horizon (used as a library, future maintenance, etc.) the null problem will present itself.

Next, I tried this:

public class SideEffectCommand implements Command<String> {

    @Override
    public String execute(String... args) {
        return "Side Effect";
    }

}

public class NoSideEffectCommand implements Command<Void>{
    @Override
    public Void execute(String... args) {
        return null;
    }   
}
public class CommandMonitor {

    public static void main(String[] args) {  
     Command<?> sec = new SideEffectCommand();
     Command<?> nsec = new NoSideEffectCommand();

     reportResults(sec.execute("args"));
     reportResults(nsec.execute("args")); //Problem Child
    } 

    public static void reportResults(Object results){
     System.out.println(results.getClass());
    }

    public static void reportResults(Void results){
     System.out.println("Got nothing for you.");
    }
}

Overloading didn't work because the second call to reportResults still called the version expecting an Object (of course). I contemplated changing to

public static void reportResults(String results){

But this illustrated the root of the problem, your client code starts having to know implementation details. Interfaces are supposed to help isolate code dependencies when possible. To add them at this point seems like bad design.

The bottom line is you need to use a design which makes clear when you expect a command to have a side effect and to think through how you would handle a set of commands, i.e. an array of unknown commands.

This may be a case of leaky abstractions.

Kelly French
+2  A: 

It looks fine to me. As others said, Void was originally designed for the reflection mechanism, but it is now used in Generics quite often to describe situations such as yours.

Even better: Google, in their GWT framework, use the same idea in their examples for a void-returning callback (example here). I say: if Google does it, it must be at least OK.. :)

Aviad Ben Dov
+2  A: 

Here's a best-of-multiple-worlds implementation.

// Generic interface for when a client doesn't care
// about the return value of a command.
public interface Command {
    // The interfaces themselves take a String[] rather
    // than a String... argument, because otherwise the
    // implementation of AbstractCommand<T> would be
    // more complicated.
    public void execute(String[] arguments);
}

// Interface for clients that do need to use the
// return value of a command.
public interface ValuedCommand<T> extends Command {
    public T evaluate(String[] arguments);
}

// Optional, but useful if most of your commands are ValuedCommands.
public abstract class AbstractCommand<T> implements ValuedCommand<T> {
    public void execute(String[] arguments) {
        evaluate(arguments);
    }
}

// Singleton class with utility methods.
public class Commands {
    private Commands() {} // Singleton class.

    // These are useful if you like the vararg calling style.
    public static void execute(Command cmd, String... arguments) {
        cmd.execute(arguments);
    }

    public static <T> void execute(ValuedCommand<T> cmd, String... arguments) {
        return cmd.evaluate(arguments);
    }

    // Useful if you have code that requires a ValuedCommand<?>
    // but you only have a plain Command.
    public static ValuedCommand<?> asValuedCommand(Command cmd) {
        return new VoidCommand(cmd);
    }

    private static class VoidCommand extends AbstractCommand<Void> {
        private final Command cmd;

        public VoidCommand(Command actual) {
            cmd = actual;
        }

        public Void evaluate(String[] arguments) {
            cmd.execute(arguments);
            return null;
        }
    }
}

With this implementation, clients can talk about a Command if they don't care about the return value, and a ValuedCommand<T> if the need a command that returns a certain value.

About the only reason not to go with using Void straight up is all the unsightly return null; statements that you will be forced to insert.

John Calsbeek
What about all the "if (results != null ){}" you'd have to insert in the calling methods?
Kelly French
If you don't know that your command returns `Void`, you probably shouldn't be testing the result for `null`, because you wouldn't have anything to do with it besides pass it around anyway.
John Calsbeek
The reason you test for null is to prevent a NullPointerException which will happen "If you don't know that your command returns Void". Ignoring it by not putting in the if(results != null){} almost guarantees a NPE in the future. That's why you don't mix the interfaces.
Kelly French
A: 

What if instead of having an interface like:

public interface Command<T> {
    T execute(String... args);
}

You instead had:

public interface Command<T> {
    void execute(String... args);
    T getResult();
    bool hasResult();
}

Then callers would do:

public void doSomething(Command<?> cmd) {
    cmd.execute(args);
    if(cmd.hasResult()) {
        // ... do something with cmd.getResult() ...
    }
}

You could also create an interface VoidCommmand that extends Command, if you like.

This seems like the cleanest solution to me.

Daniel Pryden
That has the unfortunate problem of complicating the implementations of Command<T> and not being thread-safe.
John Calsbeek
And also, it complicates things for the caller.
Stephen C
It's perfectly thread-safe as long as long as Command<T>.execute is idempotent, which is usually how I've seen the Command design pattern implemented. It also enforces command-query separation (see http://en.wikipedia.org/wiki/Command-query_separation). But you're right, it does mean that you can't call Command<T>.execute() from multiple threads. Although if that's what you needed, I would still make Command<T>.execute() synchronized (Command<T>.getResult() alone should be threadsafe, although that depends on exactly what kind of result you're returning...)
Daniel Pryden
getResult has the **same** problem of my execute: if T is Void, should I return null?
dfa
@dfa: if T is Void, throw an exception. Callers should never call getResult() without checking hasResult() first, so any code that hits that case is an error. At least, that's how I would do it, but it appears everyone in this thread disagrees with me.
Daniel Pryden
@Daniel Pryden: I don't disagree with you. The issue isn't how could you code it, it's what should the code expect. Checking hasResult() would work but it complicates the relationship between Command<T> and its clients by making the clients (or children) act explicitly on behavior of Command<T>. @dfa is asking about ways to make the coupling more implicit so the client code always knows whether to return/check for null. Always checking hasResult() everywhere is equivalent to having the following code everwhere: if(response != null) {}
Kelly French