The code below is designed to deal with this situation:
There is a Battle environment with Characters; these Characters obviously have certain characteristics and abilities, and they need to be able to use Items from an inventory.
I would like as much feedback as possible on the various aspects of the code:
- The approach to the problem: did I make something that was simple, overly complex?
- The cleanliness/readability of the code: is the code readable and understandable? Am I following the best practices? (Spaces/Tabs/Operator alignment/Brackets…)
- Code syntax: am I
return
ing correctly? Am I missing some tests that I should do?
On the other hand, I have some very specific questions:
- Am I handling closures correctly? My approach has been to try to be as modular as possible, creating one
global object
to hold all my properties, and creating modules inside *iife*s. - I had encountered a problem especially when dealing with Constructors inside the closures. Obviously they are not accessible from the outside with the usual syntax, making it impossible for me to create new Objects in different modules. I solved this problem in the
Character
constructor by actually adding the function to a variable of theBattle
global object, as I would do for a method. That, however, doesn't feel right. - In the Items module the goal is to define the various available items in a dictionary-like
object
orarray
to later add to theBattle
global object. Does this approach make sense? Adding objects to thearray
the way I do, poses a little problem, though, as the variables lose their name. - That brings me to the way I solved this problem (which perhaps is unnecessary in the first place) in the Character module. Because I want the syntax of the
useItem(item)
method to acceptitem
as astring
of the name of the item, I go a bit around in loops to find out the corresponding object using the arrayfilter()
method. Is this the right way of finding what I am looking for? Would I just be better off by referring to the items as theirarray index
in the dictionary? - Redundancy: in the
useItem()
method, after checking if the character owns the particular item he's trying to use inside anif
statement whichreturns false
, I add anelse
. Given the fact that the function returns if the conditions are met, is it redundand to add anelse
? I don't know why, but it felt safer to add it. - As for the way that Characters are handled, would I be better off having the characters in a numbered
array
then having them being handled by their variable names? Especially considering that the Battle initialisation module is just an example and, ideally, will extract data from an XML or a JSON.
"Extra" questions:
- Documentation: I am always confused by best practices on how to document code, especially when it is Javascript (at least PHP as some more consolidated best practices).
- Dev tools: both Chrome and Firefox behave differently in the way they handle the debugging statements.
- For some reason, Chrome doesn't display an integer when it's 0, while Firefox does.
- On the other hand, Firefox doesn't correctly substitute the strings when encountering things such as
%2$s
and%ss
. Am I using those incorrectly? - In this article of the Chrome Dev Tools documentation, they show that, when using string substitution, the variables get colored in blue, which aids readability quite a lot. My dev tools are not working as such, showing it all in black: am I missing something or this has been changed since the documentation has been written? I know that I can style the whole message with
%c
, but I don't think that's what happens in that article section.
Global JS
/*
* Initialising the container global object
*/
var Battle = {
itemDefinitions: [],
characters: {}
};
Item module
(function(Battle){
/*
* Item constructor
*
* Defines a new Item object.
*
* @param string name The name of the item;
* @param string result The effect of the item;
* @param string augmentedResult The effect of the item if the user's
* support is 'experiment'
*/
function Item(name, result, augmentedResult){
this.name = name;
this.result = result;
this.augmentedResult = augmentedResult;
}
// Here come the items
var potion = new Item('Potion', '+50 hp', '+100 hp');
var hiPotion = new Item('Hi Potion', '+125 hp', '+150hp');
// Put all the created items in an array for easier iteration
var itemDefinitions = [potion, hiPotion];
// Populate the global object with item Definitions
itemDefinitions.forEach(function(entry, idx){
Battle.itemDefinitions.push(entry);
});
})(Battle);
Character module
(function(Battle){
/*
* Character constructor
*
* Defines a new Character object.
*
* @param string name The name of the character;
* @method mixed useItem Uses an item from the character's inventory;
*/
Battle.Character = function Character(name){
this.name = name;
};
/*
* Uses an item from the character's inventory.
*
* This method checks if the character has the item available in
* his inventory. If he does, it uses the item to the best of the
* character's ability.
*
* @param string item The name of the item to use;
* @return mixed Returns false if the item could not be used
* otherwise, returns the item's effect;
*/
Battle.Character.prototype.useItem = function(item) {
var itemType, ownedItem, result;
// Checks if the requested item has been defined in the
// items dictionary
itemType = Battle.itemDefinitions.filter(function(obj){
return obj.name === item;
});
itemType = itemType.length === 0 ? null : itemType[0];
if( itemType === null ) {
console.error('Invalid item type “%s”: item does not exist', item);
return false;
}
// Checks if the user has enough of those items in his
// inventory
ownedItem = this.inventory.filter(function(obj){
return obj.name === item;
});
ownedItem = ownedItem.length === 0 ? null : ownedItem[0];
if( ownedItem === null || ownedItem.quantity <= 0 ) {
console.error('Cannot complete action: not enough %ss', item);
return false;
} else {
// Defines the item's effect according to the user's
// actual ability
if ( this.support === 'experiment' )
result = itemType.augmentedResult;
else
result = itemType.result;
// Removes one of the said item from the user's inventory
// and logs the result.
ownedItem.quantity--;
console.log('%s used %s: %s; \n%d %2$s remaining.',
this.name,
item,
result,
ownedItem.quantity);
}
};
})(Battle);
Battle initialisation module
/*
* Battle initialisation
*
* This would ideally get data from an external source.
*/
(function(Battle){
var carl = new Battle.Character('Carl');
carl.inventory = [
{
name: 'Potion',
quantity: 2
},
{
name: 'Eyedrop',
quantity: 1
}];
carl.support = 'experiment';
var jerry = new Battle.Character('Jerry');
jerry.inventory = [
{
name: 'Hi Potion',
quantity: 1
}
];
Battle.characters.carl = carl;
Battle.characters.jerry = jerry;
})(Battle);
Battle execution
/*
* Battle execution
*
* Just a quick example of what happens when
* the defined functions are run.
*/
Battle.characters.carl.useItem('Potion');
Battle.characters.carl.useItem('Potion');
Battle.characters.carl.useItem('Potion');
Battle.characters.jerry.useItem('Hi Potion');
Battle
as more of a namespace, right now you have it as a catch-all. I don't have much time to go into more detail now, but I'll try to find some time in the near future. \$\endgroup\$