Feature Description and Review: CyCommandHandler

Overview

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:

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

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

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

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? 

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

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

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:

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:

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.

SarahKillcoyne

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.

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.

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.

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.

CyCommandHandlers (last edited 2010-02-06 22:21:55 by ScooterMorris)

Funding for Cytoscape is provided by a federal grant from the U.S. National Institute of General Medical Sciences (NIGMS) of the Na tional Institutes of Health (NIH) under award number GM070743-01. Corporate funding is provided through a contract from Unilever PLC.

MoinMoin Appliance - Powered by TurnKey Linux