No worries. =D It's fun to help. I've got a few more thoughts for you: You can apply the same logic you did the the step*Disposition*() functions with the diplomacyDCDispostion() function. Right now it's doing 2 things: Get a disposition from a character return a DC based on that disposition. I meant to point out above the Single Responsibility Principle from Robert C. Martin 's SOLID Principles of Object Oriented Design . The basic ideas is that any object should have just one reason to change. It's described colloquially as Curly's rule from City Slickers: "One thing!" Anyway, when I look at code, I try to find places where there are duplication or places where something feels like it's not in the right place. In the case of those functions and this function, the fact that they are looking up a character attribute is a red flag, because they don't have anything to do with characters, just dispositions. I can imagine situations where you might want to list the DC for a particular disposition, but you don't have a character, or even a token. Should you still be able to find the DC of a disposition in that case? I'd say yes, so that lookup doesn't belong in there. var diplomacyDCDisposition = function (start) {
var dispositions = {'Hostile':25, 'Unfriendly':20, 'Indifferent':15, 'Friendly':10, 'Helpful':0};
return ( _.has(dispositions, start) ? dispositions[start] : 15 );
};
It also makes testing easier. The other function depended on token Ids and lookup functions, this one I can test in the javascript console of Chrome without creating a bunch of mock functions. This implementation uses an object to lookup the DC, and defaults to 15 if it doesn't know about the disposition passed in. I'm starting to see a whole little dispositions subsystem within your script. You can almost imagine a sub-module that just handles translating dispositions around. Shifting them by steps, providing their DCs, etc. Exercise for the User. =D One thing I did in the extraction of functions above is wrap the whole thing in an immediately executing anonymous function. This is a technique that lets you encapsulate code within a scope that is separated from the scope around it. It is the first half of creating a closure (some might argue it is a closure, but there is intent to be considered). In particular, what it does above is create a smaller scope where your functions are defined, such that only the functions in that scope (in that closure) can access them. It also prevents them from being overwritten by other scripts and keeps them from overwriting other scripts. (function(){
/* your code */
}()); It is common practice to wrap an immediately executing anonymous function in parentheses so that other programmers will recognize it for what it is. Speaking of duplication, looking at your getCharacterAttribute()/setCharacterAttribute() functions, they each do the following: var token = findObjs({type: 'graphic', _id: tokenID })[0];
var character = findObjs({type: 'character', _id: token.get('represents')})[0]; These are also the first 2 out of 3 lines of checkTokenRepresents(). You could replace them in both functions with: var character = checkTokenRepresents(tokenID); Additionally, you're calling getCharacterAttribute() a minimum of 8 times, which means you're looking up the same information with findObjs() over and over. findObjs() is a fairly expensive operation, as such things go, as it has to look through every defined object, O(n) runtime. That means as games get more complicated, it will get slower and slower. I've seen some scripts that used it and ran so slowly that they had to break up the execution of the script to run over the course of 20 or so timeouts. Yikes! Luckily, it's pretty easy to avert this problem by introducing some caching. Reducing the duplication to just calling checkTokenRepresents() means you only have to add the caching logic in one place. =D var tokenCharacterCache = {};
var checkTokenRepresents = function (tokenID) {
if(_.has(tokenCharacterCache,tokenID)){
return tokenCharacterCache[tokenID].character;
}
var token = findObjs({type: 'graphic', _id: tokenID })[0];
var character = findObjs({type: 'character', _id: token.get('represents')})[0];
tokenCharacterCache[tokenID]={
token: token,
character: character
};
return character
}
You might also consider renaming it to getCharacterIfExists() or similar, to reflect what its roll has become. Second installment finished! =D I'm happy to see you embracing scripting and scripting improvements. It's very hard as a programmer to be "ego-less" about your code, and many programmers struggle with it (I do!!!). You seem to be off to a great start!
