3
\$\begingroup\$

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 returning correctly? Am I missing some tests that I should do?

On the other hand, I have some very specific questions:

  1. 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.
  2. 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 the Battle global object, as I would do for a method. That, however, doesn't feel right.
  3. In the Items module the goal is to define the various available items in a dictionary-like object or array to later add to the Battle global object. Does this approach make sense? Adding objects to the array the way I do, poses a little problem, though, as the variables lose their name.
  4. 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 accept item as a string of the name of the item, I go a bit around in loops to find out the corresponding object using the array filter() 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 their array index in the dictionary?
  5. Redundancy: in the useItem() method, after checking if the character owns the particular item he's trying to use inside an if statement which returns false, I add an else. Given the fact that the function returns if the conditions are met, is it redundand to add an else? I don't know why, but it felt safer to add it.
  6. 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.
    1. For some reason, Chrome doesn't display an integer when it's 0, while Firefox does.
    2. 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?
    3. 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.

Working code

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');
\$\endgroup\$
1
  • 2
    \$\begingroup\$ I would structure 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\$
    – Shmiddty
    Commented Jul 18, 2013 at 23:07

1 Answer 1

2
\$\begingroup\$

As Shmiddty said, you want your Battle object to be more of a namespace providing access to the modules:

var Battle = function () {}

Battle.Character = (function (/* dependencies */) {

  // constructor
  var Character = function (name) {
     this.name = name;
  };

  // instance method
  Character.prototype.useItem = function () {
     [...]
  };

  // private method
  var soSomething = function () {
     [...]
  };

  return Character;

}(/* dependencies */));
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.