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.

• Multiple 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 approach, 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 being objects used for labels, descriptions, and other UI purposes.

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.

• ScooterMorris: while I appreciate the argument for a relatively "pure" approach, I, and several other plugin implementers that I've discussed this with find that being forced to refactor existing plugins would significantly inhibit adoption. Our primary goal for this is to provide an easy mechanism for plugins to take advantage of the functionality in other plugins in the Cytoscape 2.x code base. The proposed API allows for providers to implement each command as a separate class, it just doesn't mandate it, at the cost of an extra string argument. Given the state of 2.x, which has many more egregious violations of currently accepted Java practices, I believe erring on the side of easing the adoption for existing plugin authors is the correct approach.

SarahKillcoyne

• ScooterMorris: Thanks, Sarah, and friend!

I did not review this myself. I read through the wiki, looked over the code then asked a senior software engineer and architect to take a look at it and discussed it with him. This is his review:

I think that the implementation is clean and does the job fine. It will potentially lead to really long methods (and switch statements), but the routing of commands is a fairly standard pattern.

Two points on the current implementation.

1. Unregistering is a bad idea - clients will only check, if they check at all, that you implement the commands they expect at load time (or just before using them the first time). If you later unregister it will likely case clients to crash. Assume programmers are lazy (we all are).

2. ScooterMorris: I think you are probably right. I think it makes sense to remove this.

3. I don’t see the utility of registering the names of commands as clients will always have to look at documentation to understand what they need. Dynamic discovery is not useful in that case so I would just implement the arguments: CyCommandManager.execute(NameSpace, “string matching command as documented”, Object) In this case the untyped Object is some data that is expected by the plugin providing the service. The provider will have to cast it appropriately after CyCommandManager.execute(…) is called. The result will also need to be untyped and recast by the client plugin. Either way this depends on the service being documented though.

4. ScooterMorris: I understand your perspective, but turns out that dynamic discovery is extremely useful for some applications. commandTool shows an example of how a simple command parser could be written to provide a user interface for Cytoscape interaction, which, has some significant advantages. This would be possible, but difficult without the simple dynamic discovery.

That said, I think the current implementation will be overcomplicated for the service providers (and I disagree that adding a class for every command is the way to go, far too much complication for the plugin writer). You want to keep it as simple as possible for the plugin service providers or few will write them.

• ScooterMorris: I agree that our goal should be to have an implementation that is reasonable and moderately simple for plugin service providers.

Instead I would suggest that the CyCommandManager uses reflection to avoid the switch statements in plugin code. It will be more complicated for the manager, and potentially slower, but far simpler for the plugin writer (no switch statements) and allow for more error checking in the manager itself rather than the plugin.

• ScooterMorris: interesting idea, and one that I hadn't considered. I would be a little uncomfortable with this in general (java reflection can lead to unfortunate issues if the implementer isn't careful with what methods are made public and which are made private.

This case also requires that the service be documented, dynamic discovery just isn’t going to work here. So again, the client would use the three arguments listed above. The string argument would literally refer to a method name that is accessible and documented. When the manager gets the execute command it would use reflection on the namespaced class to find the matching method name, cast the untyped Object appropriately and call it. The returned data would have to be cast by the client as before.

• ScooterMorris: While this might be easier, I worry about the robustness of using a straight Object. As currently implemented, the service provider can add commands and new arguments without any changes to potential clients. Using a simple Object for the argument object and the return value would be really problematic in some cases -- consider the arguments necessary for the Force Directed Layout, for example. Very complex, but using the Tunable mechanism, easily passed through the existing API.

This is a tradeoff in performance, but much simpler for the service providers. In all cases you need a “best practices” write up for the providers and clients.

• ScooterMorris: I certainly agree with this! In part, the two plugins I've already written: commandTool and the coreCommands should provide the foundation to a "best practices" by example. These will need to be augmented by the API documentation.