Undoable Undo Commands and the Singularity

Anders Holmgren suggested a nifty implementation for the command pattern in Dart. Part of his solution has me flummoxed.

I do not think the problem is with Anders' solution. Having spent two days exploring it, I think it is solid. After last night, I do not believe the problem is with Dart either. I think the problem is mostly between my ears. That or I am experiencing the second of the two hardest things in programming:

1. cache invalidation
2. naming things
3. off-by-one errors

The naming problem that I may be experiencing is that I have been working with objects of type Command. Anders solution used functions named Command while his classes were named CommandClass. So whenever his solution referenced Command, it meant the function, but I kept thinking the object.

This brings me to a question. When working with functions and function-like objects in Dart, how should I name them? Normally, I do not care for Hungarian notation. But I cannot think of anything better than calling one Command while the other gets the Hungarian treatment (either CommandFunction or CommandClass). I am not sure either is a good approach. Having already explored the other option, tonight I will explore how things look with a Command class and a CommandFunction typedef.

I should stress that I am not attempting to argue with Anders' solution. He adapted a working solution to my existing code, which I hugely appreciate. The working solution neatly sidesteps this naming issue. I may adopt that solution, but I first would like to see how this plays out. I also note that I strongly risk confirmation bias here—CommandClass confused me when I first looked at it and I named my class my original class Command. That said...

I start with a CommandFunction typedef:

typedef void CommandFunction();

My commands will all have void return type and take no arguments. Thus, any functions with that signature are "Command Functions". For example, the functions supplied to the "Hi!" and "Scare" buttons are both a CommandFunction:

  var btnSayHi = new Button("Hi!", (){ robot.say("Hi!"); });
  var btnScare = new Button("Scare Robot", (){ robot.say("Ahhhhh!"); });

In Dart, any class that declares a call() method is also a function. So any class that implements the abstract Command class will be a CommandFunction:

abstract class Command {
  void call();
}

The MoveNorthCommand, for instance, should be a CommandFunction:

class MoveNorthCommand implements Command {
  Robot robot;
  MoveNorthCommand(this.robot);
  void call() { robot.move(Direction.NORTH); }
  void undo() { robot.move(Direction.SOUTH); }
}

And indeed, when I check an instance of this class, it does report that it is a CommandFunction:

  var moveNorth = new MoveNorthCommand(robot);
  print("Is moveNorth and CommandFunction? ${moveNorth is CommandFunction}.");

This results in:

Is moveNorth and CommandFunction? true.

There is not much that I can do with the CommandFunction in my current implementation. The Command class cannot implement a typedef. Classes in Dart, even Function classes, can only implement other classes, not typedefs. So this means that Command is implicitly a CommandFunction, but there is no way to make that relationship explicit:

typedef void CommandFunction();

abstract class Command implements Function {
  void call();
}

What I can do, as I have done with the CommandClass approach, is replace references to Function with the CommandFunction typedef. Thus, a command in the Button class must be a CommandFunction:

class Button {
  String name;
  CommandFunction command;
  Button(this.name, this.command);

  void press() {
    print("[pressed] $name");
    command.call();
    History.add(command);
  }
}

I can also constrain History so that it can only add CommandFunction objects:

class History {
  // ...
  static void add(CommandFunction c) {
   // ...
    _h._undoCommands.add(c);
  }
  // ...
}

While I am writing those, I have to admit that they do feel a little awkward. I want to talk about "command objects" or "command interface objects" instead of "command function objects." I can rationalize a little bit that these used to be "function objects" and the addition of "command" make the type clearer. But I honestly do not know if that stands up to the 6-months-later smell test. Regardless, I push on.

Currently, were I declare an UndoableCommand, it would implement the Command class (making it a CommandFunction) and add an undo() method:

abstract class Command implements Function {
  void call();
}

abstract class UndoableCommand implements Command {
  void undo();
}

I can replace Command as the implemented class in most of my commands with UndoableCommand without missing a beat. There are some other benefits as well, but where things get really interesting is with last night's typedef-as-a-generic-upper-bound solution.

This comes directly from Anders' solution and is pretty slick. Instead of declaring an undo() method in UndoableCommand, I make undo another command object:

abstract class UndoableCommand<C extends CommandFunction> implements Command {
  C get undoCommand;
}

As I found last night, I may not be able to extend a typedef in a class declaration, but I can do so in a generic. What this does is declare that the upper bound of the undoCommand has to be a CommandFunction. That is, I have explicitly constrained undoCommand to be a CommandFunction.

This allows me to rewrite my commands with commands that are themselves undoable commands:

class MoveNorthCommand implements UndoableCommand {
  Robot robot;
  MoveNorthCommand(this.robot);
  void call() { robot.move(Direction.NORTH); }
  UndoableCommand get undoCommand => new MoveSouthCommand(robot);
}

Just like replacing callbacks with objects in the original implementation the command pattern, this gives me more control and options with undo commands. Plus I can explicitly constrain these beasties to be CommandFunction thanks to the extended typedef. That remains small comfort since I cannot explicitly constrain the original Command class to be a CommandFunction. Still it's something.

As for the what gets the Command name question, I think I am going to give into confirmation bias here. I admit that CommandFunction got awkward in the History class, but I think that awkwardness was better than naming the typedef as Command and then mistaking it 6 months later for a command pattern object instead of a typedef. In the end, it is likely advisable to come up with different names altogether. Anders calls the typedef Callable in the working code, which is just another name for Function which might make it a bit too generic. Still, the resulting code reads nicely.

In the end, I think I finally have a handle on this. Big thanks again to Anders for the suggestion—undoable undo actions was a very worthwhile exploration. The typedef thing I could have done without (brain hurts!), but I should know it better... and now I do!

Play with the code in DartPad: https://dartpad.dartlang.org/61b96c6edfb1074c77f4.


Day #47

‹prev | My Chain | next›

Compile Time Constant Factory Method

Tonight I continue to poke and prod the first design pattern destined for Design Patterns in Dart...

I really like yesterday's map-of-factories approach to the Factory Method pattern in Dart. The approach, first suggested to me by Vadim Tsushko removes the immediate responsibility of Factory Methods from subclasses of the creator class and instead places them in a Map of factories. I think I may have left some room for improvement though...

The classic Factory Method pattern starts with an abstract creator class that includes an abstract creator method:

abstract class Creator {
  Product productMaker();
  // ...
}

The Product is similarly abstract—the concrete subclass of Product is deferred to the concrete subclass of Creator. Leaving the productMaker() method abstract says just that: the subclass needs to define a productMaker() method that returns a subclass of Product:

class ConcreteCreator extends Creator {
  productMaker()=> new ConcreteProduct();
}

Yesterday's variation does away with the creator subclass, which is quite nice if the factory method is the only reason to define a subclass. Instead, the top-level creator class now looks up the factory method in a simple Map:

typedef Product productFactory();
class Creator {
  static Map<Type, productFactory> factory = new Map();
  Product productMaker(type) => factory[type]();
  // ...
}

Here, I typedef a function that takes no arguments and returns some kind of concrete Product (just like the original productMaker() factory method). The static factory map is then a key/value store in which the keys are types and the values are product factories.

Either approach (subclass or map-of-factories) will work as a Factory Method implementation. I have yet to benchmark the two. In the absence of that admittedly useful information, the choice between the two comes down to context or personal preference. In the web MVC framework example that I used yesterday, it probably makes more sense to use the subclass approach as I have to create subclasses anyway and the map-of-factories was a little awkward. If the concrete classes are all part of the same codebase or if a parallel class hierarchy needs factories, then map-of-factories seems the better approach.

I do not want to dwell on those question too much just yet. Instead, I am wondering if including the map-of-factories in the creator class is the right approach. Perhaps this was just the codebase upon which I was experimenting last night, but it felt a little messy having it in the same class.

Instead, I would like an entirely separate Factory class to hold this information. Something along the lines of:

typedef Product productFactory();
class Factory {
  static Map<Type, productFactory> factory = new Map();
}

So I give that a try in my Hipster MVC (abstract creator and product) and Dart Comics (concrete creator and product) codebases. In Hipster MVC, the HipsterCollection class had served as the abstract creator, creating HipsterModel objects from attributes fetched via a RESTful backend. Last night it got the map-of-factories treatment. Tonight, I move that out into a separate Factory class:

typedef HipsterModel modelMaker(Map attrs);

class Factory {
  static Map factory = new Map();
}

This works just fine. HipsterCollection is now capable of creating concrete instances of HipsterModel (e.g. ComicBook from Dart Comics) using this Factory:

class HipsterCollection extends IterableBase {
  // ...
  HipsterModel modelMaker(attrs) => Factory.factory[this.runtimeType](attrs);
  // ...
}

But you know what? Factory.factory looks ugly. It would be much cooler if that were just:

  HipsterModel modelMaker(attrs) => Factory[this.runtimeType](attrs);

Unfortunately operators, like the square bracket lookup operator, cannot be static:

class _Factory {
  static Map<Type, modelMaker> factory = new Map();

  // This won't work!!!
  static operator [](type) => factory[type];
}

I know this does not work because I try it out only to get a nice little exception:

Internal error: 'package:hipster_mvc/hipster_factory.dart': error: line 11 pos 19: operator overloading functions cannot be static
  static operator [](type) => factory[type];

Bah!

All is not lost… as long as I am willing to resort to some compile time constant chicanery. And of course I am more than willing. I rename the Factory class as _Factory and declare a compile time constant of Factory which of type _Factory:

_Factory Factory = const _Factory();

class _Factory {
  static Map<Type, modelMaker> factory = new Map();

  const _Factory();
  operator [](type) => factory[type];
  operator []=(type, function) { factory[type] = function; }
}

Which does the trick, though it feels a little dirty. Dirty because I declare a compile-time constant whose sole purpose is to store changing key-value pairs. But it works because the instance variable factory fails on the _Factory instance which leaves Dart to fall back on static variable lookup.

And it is at this point that I realize that, yes, I am an idiot.

Because I have gone to all of this effort even though an empty HashMap is also a compile time constant. So the _Factory class and Factory compile time constant can be replaced with:

typedef HipsterModel modelMaker(Map attrs);
Map<Type, modelMaker> Factory = {};

Well, that was a long way to go to wind up with a simple HashMap lookup.

Ah well, maybe that const + static method trick will come in handy some day. Stranger things have happened.

But for today, I have my map-of-factories in better shape with an extremely small amount of code. Demonstrating publicly that I'm a dummy is a small price to pay for that!

Tomorrow: benchmarking.



Day #104

‹prev | My Chain | next›