Feature Description and Review: CyCommandHandler

Overview

• One of the strengths of Cytoscape is it's plugin architecture and the wealth of plugins that are available. One of the weaknesses of Cytoscape is that plugins have no easy way to take advantage of functionality available in other plugins. As a result, the user must act as the "inter-plugin" communications agent, or plugin authors must use the CyAttributes mechanism to exchange data back and forth, which entails a certain level of coordination and agreement that can sometimes be difficult. The goal of this new API within Cytoscape is to partially solve this weakness by providing a mechanism through which a plugin can publish a set of functions that it will perform. Plugins that wish to make use of that functionality can do without directly accessing the API (which means you don't need to include the plugin in your classpath).

  The general approach taken by the CyCommandHandler mechanism is similar to the pattern used by CyLayouts. A plugin registers it's commands when it is loaded, and plugins can check to see if a given command has been registered. To avoid multiple plugins attempting to register the same command (e.g. 'open'), each plugin can reserve a namespace for itself. Once a plugin has reserved a namespace, no other plugin may reserve the same namespace. In this way, a plugin might reserve the IDMapper namespace and any commands registered by that plugin would be part of the IDMapper namespace. A plugin may reserve multiple namespaces, but this should be rare. Arguments to commands are given as a Map of name, value pairs or a list of Tunables. In any case, the list of possible arguments may be determined through calls to the API.

Example

Consider the example below. We have two plugins: MyPlugin and YourPlugin. Hypothetically, MyPlugin provides some sort of analysis algorithm that takes a network, a list of nodes, and an iteration parameter and returns a score and a value for each node. YourPlugin wants to use that algorithm and visualize the results in some way. Here is the code for MyPlugin:

public class MyPlugin extends CytoscapePlugin {
   public MyPlugin() {
      // Plugin initialization
      try {
        // You must reserve your namespace first
        CyCommandNamespace ns = CyCommandManager.reserveNamespace("my algorithm");

        // Now register this handler as handling "analyze"
        CyCommandHandler myHandler = new MyHandler(ns);
      } catch (RuntimeException e) {
        // Handle already registered exceptions
      }
   }

   // Use a separate class so we can extend AbstractCommandHandler
   class MyHandler extends AbstractCommandHandler {
      protected MyHandler(CyCommandNamespace ns) {
          super(ns);
          addArgument("analyze", "network", "current");
          addArgument("analyze", "nodeList");
          addArgument("analyze", "iterations", new Integer(2));
      }
      public String getHandlerName() { return "analyze"; }
      public CyCommandResult execute(String command, Map<String, Object>args) 
                             throws CyCommandException {
          // Assuming we've implemented arguments as Tunables
          return execute(command, createTunableCollection(args));
      }
      public CyCommandResult execute(String command, Collection<Tunable>args)
                             throws CyCommandException {
          // Execution code goes here...
      }
   }
 }

The code for YourPlugin is even easier...

public class YourPlugin extends CytoscapePlugin {
    public YourPlugin {
        // Plugin initialization.  Note: we don't want to look for MyPlugin yet.  That should
        // wait until we actually want to use it.  This avoids errors that result from the
        // arbitrary loading order of plugins.
    }

    public void doWork() {
        Map<String, Object> args = new HashMap();
        args.put("iterations", new Integer(10));
        try {
            CyCommandResult result = CyCommandManager.execute("my algorithm", "analyze", args);
            // Visualize data from result
        } catch (CyCommandException e) {
            // Handle exception
        }
    }
}

Code Layout

All code for the CyCommands implementation is in src/cytoscape/command and is part of the cytoscape.command package. There are no hooks anywhere else in the Cytoscape core that depend on this package. The package provides two interfaces:

• CyCommandHandler: This is the main interface for implementing a command handler. Generally, it is better to extend AbstractCommandHandler, which provides defaults for most methods and a number of convenience functions that significantly simplify implementing a new handler.

• CyCommandNamespace: This is the namespace interface. This should only be implemented by the package itself.

In addition to the two interfaces, there are four classes:

• AbstractCommandHandler: This is an abstract base class that provides a number of convenience routines for implementing command handlers.

• CyCommandException: The exception that get's generated when something goes wrong with an execute.

• CyCommandResult: The results object that is passed back. This includes any error strings, messages, and a map with the actual results.

• CyCommandManager: The main "hook" into the Cytoscape core. This is the singleton class that holds all of the CyCommandNamespace information as well as all of the CyCommandHandlers.

And finally, there is a single command implementation for the help function: HelpCommand.

Code Review Comments

Piet

Looks good, something to look forward to for plugin developers (although this should be covered for 3.0 also). I especially like the Tunables mechanism as a model for this

• Is there some kind of mechanism that would expose the names of the the functions also; eg. require an Enum with names describing the functionality. These could be exposed and documented through the API also...

  • ScooterMorris: There is -- you can get the names of all namespaces with CyCommandManager.getNamespaceList() and a list of all the commands for a single namespace with CyCommandManager.getCommandList(String namespace).

• Is there some flag that can be set to check whether the plugin has been loaded? Or an event listener, fire the event when loaded...

  • ScooterMorris: No, but that shouldn't be needed. The idea is that a client would only attempt to execute a command after all initialization has completed. On the other hand, you could always listen for CYTOSCAPE_INITIALIZED if you wanted to execute a command right after initialization was complete.

Even more impressive when I studied the actual code (made my remarks above obsolete Some points about the code:

• Should't an addArgument method be part of the interface CyCommandHandler also? And addDescription for that matter.

  • ScooterMorris: I don't think so. Those are really convenience functions for handlers and should never be referenced externally. Note that both addArgument and addDescription are protected methods...

I'm not an architect but imho the CyCommandHandler interface should be pretty strictly defined to force communication between plugins. Or am I missing something?

• ScooterMorris: Not sure what you mean here -- that is certainly the goal of the CyCommandHandler interface and it's only real function (although I eventually would like to see the coreCommands as a core plugin). PietMolenaar; a not so smart comment from me; of course only getEtc and execute should be in the interface. My only concern is that you ideally want the names of commands available through the plugin API also; the way it works now for Layouts (for example) always requires the developer to look into the actual code for the names of the layouts in order to execute them...

• In AbstractCommandHandler the value null is passed to a Tunable object. Doesn't this give rise to a nullpointer exception in Tunable?

protected void addArgument(String command, String vKey) {
                addArgument(command, vKey, null);
        }

• ScooterMorris: No, Tunable explicitly accepts "null" as a value to indicate that there is no initial value for this Tunable PietMolenaar: Ok, for some reason I always thought this was necessary when using Tunable...

* I think you're right whereas the "help" functionality should be subsumed by the "shell"; so lift the HelpCommand out of command

• ScooterMorris: Cool, this is now done and checked in.

MikeSmoot

In general I think the concept of CyCommands is a reasonable solution to facilitate plugin communication. I do, however, have serious problems the design of CyCommandHandlers.

The fundamental problem is that the CyCommandHandler interface couples two distinct functions: a command container that manages independent commands and the implementation of individual command behavior (e.g. load network, destroy network). This is to say that CyCommandHandlers violate the single responsibility principle and do not cleanly separate concerns. These classes are forced to do many things that are unrelated and should not be grouped into the same class, which is a problem for several reasons:

• It leads to single methods that are hundreds of lines long.
• It introduces a ton of unnecessary inter-dependencies (e.g. a network destroy command doesn't need to know about XGMML, which is necessary for network import).
• It makes the code much harder to unit test because of the myriad dependencies needed to instantiate the command.
• It increases the cyclomatic complexity of the methods, which provides yet another way for bugs to wiggle into our code.
• It makes the code much harder to understand since you have to read through dozens of "if" statements and remember what args and commands are being operated on.

• Mulitple commands per handler necessitate an extra argument for each method in the CyCommandHandler interface, which forces methods to become more complicated.

A slightly more esoteric concern is that the CyCommandHandler approach also violates Effective Java guideline #50, which recommends avoiding the use of Strings when other types are more appropriate. In the current CyCommandHandler design, the only thing binding the behavior of the CyCommandHandler methods (e.g. execute, getDescription, getTunables, etc.) is the String command name. This means that the command string is only coupled to the actual command behavior by convention and the diligence of the plugin writer. From a practical perspective, plugin writers must go to great lengths to ensure that their use of string commands is consistent throughout the CyCommandHandler class. This is not difficult, just tedious and error prone.

All of this goes towards making the code less modular, harder to maintain, more brittle with respect to change.

The solution I'm proposing is instead of having one CyCommandHandler combine multiple commands into a single class, create a separate CyCommand class for each command. Instead of a command class serving as both a container of multiple commands and the commands themselves, a class simply represents one command and all of the container behavior is moved to the CyCommandManager. When commands do need to share code, an abstract base class can capture similar behavior. This apprach, which we've been calling "One Command, One Class" (OCOC), adheres to the single responsibility principle, Effective Java recommendation #50 and also more closely mirrors the intent of the Command design pattern. OCOC alleviates all of the points mentioned above:

• It reduces the length of methods, reduces the cyclomatic complexity, and makes the code easier to read since the chained IF statements disappear.
• It reduces inter-dependencies because the behavior of a given class if focused on a single task.
• It makes the code easier to unit test because there will be few dependencies to be stubbed or mocked.
• It simplifies the API because there are fewer arguments.
• Commands are distinct objects and strings are demoted from unifying principle to .

Each point helps create code that is more modular, easier to maintain, and more robust to change.

Finally, an implementation of this approach is available in the attached tar files. command.tgz contains the core API and coreCommands2.tgz shows how some some of the core commands could be ported.