Repeating Undos in Command

My rather complicated command pattern has a bug. Perhaps my first instinct ought to be to simplify the implementation and maybe I will do that in the end. But first, I am going to see if I can fix the bug and, in doing so, see if this has something to say about my implementation of the pattern.

Last night's implementation on DartPad: https://dartpad.dartlang.org/efac76d03df17f6e3920.

The problem is in the undo operation. Complicating the undo is that some of the commands in my Dart implementation accept arguments:

  btnPlay.call(['It Had to Be You']);
  btnPlay.call(['Cheek to Cheek']);
  btnPlay.call(['At Last']);
  Button.undo();

I resolved a similar issue with these argument-commands the other night, but after some naming-inspired refactoring last night, the problem is back.

In the above, I am simulating a Mel Tormé aficionado playing songs on the velvet fog machine. The above script plays It Had to Be You, Cheek to Cheek, then At Last. While playing At Last, the Mel fan hits the undo button, which sends the song back to Cheek to Cheek. That still works:

$ ./bin/jukebox.dart
Play It Had to Be You
Play Cheek to Cheek
Play At Last
Undoing Instance of 'PlayCommand'
Play Cheek to Cheek

The problem occurs if I hit the undo button twice:

$ ./bin/jukebox.dart
Play It Had to Be You
Play Cheek to Cheek
Play At Last
Undoing Instance of 'PlayCommand'
Play Cheek to Cheek
Undoing Instance of 'PlayCommand'
Play Cheek to Cheek

Instead of playing At Last after the second undo, the velvet fog machine is still playing Cheek to Cheek.

My solution from the other night was to store the previous state information in the concrete command object—the PlayCommand in this case:

class PlayCommand implements Command {
  VelvetFogMachine machine;
  String _prevSong;

  PlayCommand(this.machine);

  void call([List args]) {
    _prevSong = machine.currentSong;
    machine.play(args.first);
  }

  void undo() {
    machine.play(_prevSong);
  }
}

Whenever the command is called, it first stores the current song being played, then tells the machine to play the requested selection. The whole point of the command pattern is to objectify the receiver (the velvet fog machine) and action (play), so this is all above board—I am not violating encapsulation.

This approach seemed to work the other night, but no longer. So what gives?

I initially thought this was caused by storing the history as a static property of the Button class:

class Button {
  static List _history = [];
  // ...
  void call([List args]) {
    command.call(args);
    _history.add(command);
  }
  // ...
}

That turns out not to be the cause. Instead, this is a pre-existing bug that I only noticed because of last night's reworking.

When the above code adds the command object to the history list, it is always adding the same instance when repeating commands. That is, multiple play button presses always send the same command to call():

  var btnPlay =
    new Button("Play", play);
  btnPlay.call(['It Had to Be You']);
  btnPlay.call(['Cheek to Cheek']);
  btnPlay.call(['At Last']);

To resolve this, I have to clone the command before adding it to the history list:

class Button {
  static List _history = [];
  // ...
  void call([List args]) {
    command.call(args);
    _history.add(command._clone());
  }
  // ...
}

That way, the previous song is remembered in the clone and not in the re-used instance.

Since there is no built-in clone() method in Dart, I have to make one of my own:

class PlayCommand implements Command {
  VelvetFogMachine machine;
  String _prevSong;
  PlayCommand(this.machine);
  // ...
  PlayCommand _clone() =>
    new PlayCommand(machine)
        .._prevSong = _prevSong;
}

With that, I am able to undo to my heart's content:

$ ./bin/jukebox.dart                                       
Play It Had to Be You
Play Cheek to Cheek
Play At Last
Undoing Instance of 'PlayCommand'
Play Cheek to Cheek
Undoing Instance of 'PlayCommand'
Play It Had to Be You

Although that does resolve my problem, I am not entirely satisfied. I have previously found that I need to clone receivers for similar reasons and now I have to clone commands. That is a lot of cloning to keep in order. I do not know if anything can be done to improve the situation. I think not, though I may take a look tomorrow. For now, I call this good enough.

Play with the code on DartPad: https://dartpad.dartlang.org/164484663ae8487e4aa3.


Day #32

‹prev | My Chain | next›