4
\$\begingroup\$

I'm currently working on a JRPG style turn-based game and I read an article about that an action queue is necessary for these types of games, so I wrote one.

It basically has 4 methods:

  1. .add (add an object to the queue)
  2. .remove (remove an object from the queue, might be useful if some entity dies mid fight and shouldn't action if it's still in the queue)
  3. .removeFirst (remove the first object from the queue after it invoked its action function e.g attack, defend, spell etc.)
  4. .reset (Resets the whole queue to an empty array [], could be useful if the player kills the target and the battle ends, then the queue should also be emptied)

The queue looks rather complex in code and the arrays I'm putting these in feel a bit awkward to access. Generally the code feels poorly written and inefficient, which is why I'm asking for a code review.

function keys(object) {
  return Object.keys(object);
}

let queue = {
  attackQueue: [],

  add: function(object) {
    let propName = keys(object), array = [], outerArr = [];

    for (let i = 0; i < propName.length; i++) {
      if (propName[i] === "name") {
        outerArr.push(object[propName[i]]);
      } else if (propName[i] === "command" || propName[i] === "hitChance") {
        array.push([propName[i], object[propName[i]]]);
      }
    }
    outerArr.push(array);
    queue.attackQueue.push(outerArr);
  },

  remove: function(object) {

    for (let i = 0; i < queue.attackQueue.length; i++) {
      if (object.name === queue.attackQueue[i][0]) {
        queue.attackQueue.splice([i], 1);
      }
    }
  },

  sort: function() {
    queue.attackQueue.sort(function(a, b) {
      if (a[1][0][1] < b[1][0][1]) {
        return 1;
      }
    })
  },

  removeFirst: function() {
    queue.attackQueue.shift();
  },

  reset: function() {
    queue.attackQueue = [];
  }
}

function executeRound() {
  let length = queue.attackQueue.length, arr = queue.attackQueue;

  while (queue.attackQueue.length !== 0) {
    window[arr[0][1][1][1].toLowerCase()]();  // I used toLowerCase just in case the commands would have been wrote in upperCase
    console.log("removed entity " + arr[0][0] + " !");
    queue.removeFirst();
  }
}

function attack() {
  console.log("attack");
}

function defend() {
  console.log("defend");
}

// example
let player1 = {
  name: "player1",
  hitChance: 9000,
  command: "attack"
}

// add player1 to the queue
queue.add(player1);

// execute function "executeRound"
executeRound();

console.log(queue.attackQueue) // --> "removed entitiy player1 !"
\$\endgroup\$
  • \$\begingroup\$ Which version of JavaScript are you looking for, ES6? I have to say, those array indexers are no joy to read, also, you seem to be missing a context when calling things :) \$\endgroup\$ – Icepickle Aug 23 '17 at 13:48
  • \$\begingroup\$ @Icepickle I usually take whatever fits best, I dont mind if it's ES6 or 5. I just used Object.keys because that's the only way I know to do such operation for these cases. Yes it's horrible to read, that's why I want some feedback on how to make this look better, maybe with objects rather than arrays. What you mean with missing a context ? \$\endgroup\$ – codeForBreakFast Aug 23 '17 at 17:43
3
\$\begingroup\$

Separation of concerns

A queue is a well-known data structure for the general purpose of processing things following first-in-first-out logic. Such FIFO logic has a few well-defined essential operations, such as enqueue, dequeue, isEmpty.

The posted implementation does much more than expected from a general purpose queue. The add function in particular maps the input objects to different kind of objects before adding them.

It would be better to separate the queue data structure from the rest of the logic specific to the game.

OOP

The queue is a custom user object, designed by you. It would be good to go further and design a custom object to model the items you add to the action queue. What you currently have, a plain array with another array inside is not very easy to understand or work with.

Alternative implementation

A queue backed by an array can be implemented like this:

var queue = (() => {
  var arr = [];

  return {
    isEmpty: () => arr.length == 0,
    add: item => arr.push(item),
    remove: () => arr.shift(),
    removeAll: filter => arr = arr.filter(x => !filter(x))
  };
})();

Notable points:

  • It implements only essential features: the common features of a queue, and the removeAll method required by your application, but concerning only the internals of the queue, no other objects
  • The queue doesn't need to know anything about the objects you put in it
  • The queue's storage, arr, is inaccessible from the outside

You also had a sort function in your queue, but you didn't explain why. This is not a common functionality of queues, and it goes directly against the FIFO behavior, so I don't think it belongs here. It's also possible that you need not a queue, but something else.

You could put your player plain objects in this queue, without converting them to the nested array structure. If the player plain object in the example is not useful to you, you could define your own data structure, just like you did for your queue implementation. You can convert into that structure before putting in the queue.

\$\endgroup\$
  • \$\begingroup\$ I dont quite understand. The array looks bad and it's terrible to work with it, I get that. But I can't really find a way to loop through a queue object which is filled with other objects. The problem is that a for .. in loop is not reliable because it jumps sometimes instead of always going through the line. That's why I choosed using arrays instead of plain objects. Could you post a simple example ? Maybe I get a better grasp for the OOP model. \$\endgroup\$ – codeForBreakFast Aug 23 '17 at 17:48
  • \$\begingroup\$ @codeForBreakFast I add an "alternative implementation" section, I hope it helps \$\endgroup\$ – janos Aug 23 '17 at 18:47
2
\$\begingroup\$

The queue looks rather complex in code

It's because your game logic is mixed with your queueing logic. The queue should just be that, a queue. It takes in objects on one end, and spits them out the other. It shouldn't care what the items are all about. Because of this, your queue gets demoted to a simple array.

If you really want a custom queue API, the answer provided by janos addresses that.

if (a[1][0][1] < b[1][0][1])

Consider using objects. Using arrays may be more compact especially with storage, but accessing stuff with indexes is horrible for maintenance.

for (let i = 0; i < propName.length; i++) {
  if (propName[i] === "name") {
    outerArr.push(object[propName[i]]);
  } else if (propName[i] === "command" || propName[i] === "hitChance") {
    array.push([propName[i], object[propName[i]]]);
  }
}

One other thing I notice is the command property and friends. Doing it this way pushes much of the implementation to the library instead of the consumer. This will then lead to "code mapping", which is an unnecessary bulk in the implementation. Instead, consider making the queue a queue of functions. Let the consumer define what those functions do and still have the queue not care about it.

Here's a simple example:

// game-lib.js

// Nothing fancy here, just a custom front for an array.
function createQueue() {
  const queue = []
  return {
    add: item => queue.unshift(item),
    get: () => queue.pop(),
    // Fancy trick to clear an array - set its length to zero.
    clr: () => queue.length = 0
  }
}

// your-game.js
const gameState = {
  currentPlayer: 'player1',
  players: {
    player1: {
      hp: 10000
    },
    player2: {
      hp: 10000
    },
  }
}

// Define what's possible in game. Since this code is on the consumer
// side, you have full visibility of game state and variables.
// The following functions create a function that executes with the
// given details. Yes, it's building "dynamic" functions.

const attack = (targetPlayer, damage) => () => {
  console.log(`Attacking ${targetPlayer} for ${damage}`)
  gameState.players[targetPlayer].hp -= damage
}

const switchPlayer = targetPlayer => () => {
  console.log(`${targetPlayer}'s turn`)
  gameState.currentPlayer = targetPlayer
}

// To run the game, you simply create actions
const actionQueue = createQueue()

// Queue the actions
actionQueue.add(switchPlayer('player1'))
actionQueue.add(attack('player2', 100))
actionQueue.add(switchPlayer('player2'))
actionQueue.add(attack('player1', 100))
actionQueue.add(switchPlayer('player1'))

// Execute actions. Again, a queue is a queue. It shouldn't know
// that it contains actions. The extra () is because we're executing
// the function we got from queue.get()
actionQueue.get()()
actionQueue.get()()
actionQueue.get()()
actionQueue.get()()
actionQueue.get()()

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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