Return to Answer

• 33.3k
• 2
• 46
• 90

• 33.3k
• 2
• 46
• 90

I can't find a semicolon anywhere in your code. Sure, it'll work without them, but it's still a bad habit. Sooner or later, you'll write some lines of code that do require semicolons to parse correctly, or you'll feed the script to a minifier that just removes whitespace, and all of sudden things are broken. Basically, while JS does do automatic semicolon insertion, relying on it is a little iffy in my opinion. The language has semicolons for a reason.

Otherwise, the code looks well-formatted. Some names could be more descriptive, though. For instance prop in your for...in loops might more descriptively be named stateName or simply state. "Prop" isn't terribly descriptive (later you have single-letter variables which are even more opaque). You've also got some variables you're not using at all like ind and arr in your forEach callbacks. If you're not using them, leave them out (and if you need them, ind should just be i or index as those are the most conventional names for an "index" variable.)

Functionality-wise, this comment worries me a bit:

// Initialize to an arbitrary state, client can transition to 
// whatever their desired initial state is

This basically means that users must make two calls to fully set up the state machine. If they don't, the initial state will be arbitrary. And if they do, but they've already registered transition callbacks, those callbacks will fire even though you're just trying to set up the state machine). So you may get some unexpected, spurious state-changes, simply as a result of setting up the state machine.

Of course, as long as you use an object to hold the states, there's no guaranteed ordering, so there's no "first" state. I'd simply recommend not using an object, for this very reason.

Instead, you could do something like:

function FSM(stateNames /*, ... */) {
 this.states = Array.prototype.slice.call(arguments);
 if(!this.states.length) {
 throw new Error("No states given");
 }
 this.state = this.states[0]; // default to the first named state
 // ...
}
// ...
var stateMachine = FSM("idle", "working", "completed");

But continuing with your current code, you've also got this line:

// Set the _fsmName of each state to its name in the states object
// This makes it much easier to check if we're in a particular state

However, you don't need the name to check. Your inState function could simply be written as

FSM.prototype.inState = function(name) {
 return this.state === this.states[name];
}

and there'd be no need to change the object you've been passed. The caller might hold their own reference to that object, and won't expect it to change.

Now, the following is a bunch of suggestions for different approaches; sometimes quite different. I'm not saying this is the "one true way" to build things, it's just how I'd probably do it:

For handling associated properties, I'd propose something different: Simply return an object, and let the caller sort it out, instead of having getters and setters.

stateMachine.properties() // => an object containing the current state's properties

For here, the caller can read/write whatever they want in that object, with no need for getters and setters. The properties function could accept an optional argument in case you want properties for something other than the current state.

For handling transition callbacks, I'd storing them like so:

this.callbacks = {
 stateName: {
 enter: [callback, callback, ...],
 exit: [callback, callback, ...]
 }
};

Which would make it easier to call them, when needed

FSM.prototype.transition = function (targetState) {
 if(this.states.indexOf(targetState) === -1) {
 throw new Error("State '" + targetState + "' does not exist");
 }
 this.callbacks[this.state].exit.forEach(function (callback) { callback(); });
 this.state = targetState;
 this.callbacks[targetState].enter.forEach(function (callback) { callback(); });
}

Speaking of, it might be nice to pass the current and upcoming state names to the callbacks.

Lastly, it could be a good idea to skip the prototype functions in favor of dynamically adding functions in the constructor, or returning an object with the functions you want. For prototype functions to work, things like this.state have to be accessible, which means they're also accessible to the outside world. So I could just call stateMachine.state = 'foobar' and sidestep the transition function (and its checks and callbacks) entirely.

Below is an alternative implementation (again, this is just how I might do it - it's not to say that this is the only way):

function FSM(stateNames) {
 // keep variables local
 var states = {},
 currentState;
 
 stateNames = Array.prototype.slice.call(arguments);
 // check arguments
 if(!stateNames.length) {
 throw new Error("No states given");
 }
 // set initial state
 currentState = stateNames[0];
 
 // create objects to hold state properties and callbacks
 stateNames.forEach(function (name) {
 states[name] = {
 properties: {},
 callbacks: {
 enter: [],
 exit: []
 }
 };
 });
 // return an object with the public API. This let's the functions
 // access the "private" variables above with having to expose them
 // to the outisde world, and let's us call either "new FSM(...)" or
 // just "FSM(...)" and get something useful
 return {
 // get the current state name
 state: function () {
 return currentState;
 },
 
 // return (a copy of) the list of state names
 states: function () {
 return stateNames.slice(0);
 },
 
 // transition to another state
 transition: function (targetState) {
 if(!states[targetState]) {
 throw new Error("State '" + targetState + "' does not exist");
 }
 
 states[currentState].callbacks.exit.forEach(function (callback) {
 callback(currentState, targetState);
 });
 
 currentState = targetState;
 
 states[currentState].callbacks.exit.forEach(function (callback) {
 callback(currentState, targetState);
 });
 },
 
 // get state properties (optional stateName argument)
 properties: function (stateName) {
 stateName = stateName || currentState;
 return states[stateName].properties;
 },
 // add a transition listener
 on: function (stateName, event, callback) {
 // this'll automatically throw an exception if the state or event doesn't exist
 states[stateName][event].push(callback);
 },
 // remove a transition listener
 off: function (stateName, event, callback) {
 var callbacks = states[stateName][event];
 
 for (var i = callbacks.length - 1 ; i >= 0 ; i++) {
 if(callbacks[i] === callback) {
 callbacks.splice(i, 1);
 }
 }
 }
 };
}