This is looking really good! I like the features you're building and the way you decompose the problem. =D You obviously have a good bit of practical programming experience already so here are a few more observations, mostly centered around idiomatic javascript. You can actually simplify syncProperty() further with a bit of idiomatic javascript: syncProperty = function(updatedToken, properties) {
var propList = _.intersection(
(properties && properties.split && properties.split("|")) || [],
state.TokenSync.syncList[updatedToken.get("represents")] || []
),
update = _.reduce(propList,function(m,p){
m[p]=updatedToken.get(p);
},{});
_.each(findObjs({ _subtype: "token", represents: updatedToken.get("represents")}),function(t){
t.set(update);
});
},
Broken down: (properties && properties.split && properties.split("|")) || [], This checks to see if properties is "truthy" and has a .split and calls split, or uses []. Building an update object with _.reduce() means you don't need to loop over the properties for each token, making the update an O(n) operation, rather than O(n 2 ). If the token doesn't represent a character, the proplist ends up being empty, which causes the update to be {} and the list of characters that match the id ends up as []. You could return before the _.each() if propList.length is 0 to avoid the findObjs(). add = function(charID,properties,silent) {
var propsRequested = properties.split("|");
var propsRejected = _.difference(propsRequested, syncable);
var propList = _.intersection(propsRequested, syncable);
var propsAlready = _.intersection(propList,(state.TokenSync.syncList[charID]||[]));
propList = _.difference(propList,(state.TokenSync.syncList[charID]||[]));
if (!silent) {
if (propsRejected.length) {
sendChat("TokenSync","Invalid sync propert"+(1===propsRejected.length ?'y':'ies')+": " +propsRejected.join());
}
if (propsAlready.length) {
sendChat("TokenSync","Already synchronizing: " + propsAlready.join());
}
if (propList.length) {
sendChat("TokenSync","Now synchronizing: " +propList.join());
}
}
state.TokenSync.syncList[charID] = _.union( (state.TokenSync.syncList[charID]||[]), propList);
_.each(propList,registerListener);
},
Sometimes it's hard to see the forest for the trees. =D The .join() method on Array defaults to joining the contents together with ", " as the separator, which visually simplifies this. Also using the ternary operator to pick the singular/plural ending means you can do the output in one line, though I hardly think anyone would fault you for just using the plural all the time. It's a bit more idiomatic in javascript just use the truthy/falsy nature of variables in logic tests, so !silent rather than silent !== true , and propList.length , rather than propList.length > 0 . I wonder if it might be simpler to simply register a single on('change:token',...), rather than keeping track of all the different propsListened. You'll already be attempting to sync every listened property on each token regardless of it that token is syncing that property so at worst, you attempt no more. However you change from doing O(n) calls to syncProperty() on each token change to only doing O(1) calls and you don't have to track what properties you've registered for. (and there isn't a way to unregister, to answer you comment question. =D). Speaking of propsListened, you don't actually need to put that in the state. Since you're setting it to [] in checkInstall(), its lifetime will always be the lifetime of the module closure so you could put it after lastUpdate. That will also remove the crash bug when it tries to set it before state.TokenSync has been created 2 lines lower. This isn't shorter (largely due to formatting), but you might like it as an alternative way to handle syncNewToken(): syncNewToken = function(newTok) {
var done = false,
charID = newTok.get('represents');
filterObjs(function(o){
if( !done &&
'graphic'===o.get('type') &&
'token'===o.get('subtype') &&
o.get('represents') === charID &&
o.id !== newTok.id )
{
syncProperty(o);
done = true;
}
return false;
});
},
filterObjs() is usually used to get a set of objects when you have more dynamic comparisons you need to perform. However, it's also a good way to perform an operation using the list of objects. It's slightly more efficient (according to Riley) than using findObjs() followed by filtering the list of returned objects. (It would be nice if we had a findObj() with a finish on first match...). Anyway, the first time this finds a token that represents the character but isn't the new token, it will sync and then just return false the rest of the time. Cheers! Disclaimer: I've not run any of the above code, just linted it.