5
\$\begingroup\$

I started to learn programming just a few days ago. And I've written code for a simple original game for practice, using some knowledge of JavaScript that I have gained so far.

Is the following code properly written? I'm especially concerned about the use of "Math.floor", "Math.random", "function", and "if / else".

Incidentally, "Metal King" is the name of a monster in the game, and "Angelo" is a character.

alert("Metal Kings (A, B, and C) have appeared!");



// Which Metal King (A, B, or C) will Angelo decide to attack? \
// 33% probability for each.

var MetalKingABC = 1 + Math.floor(Math.random() * 3);


if (MetalKingABC === 1) {
    MetalKingABC = "Metal King A";
}
else if (MetalKingABC === 2) {
    MetalKingABC = "Metal King B";
}
else {
    MetalKingABC = "Metal King C";
}



var WhichOneToAttack = function(MonsterName) {

    alert("Angelo is about to attack " + MonsterName + ".");


    // Metal King has a 50% chance of fleeing the battle.

    var FleeOrFight = 1 + Math.floor(Math.random() * 2);


    if (FleeOrFight === 1) {
        alert(MonsterName + " flees like lightning!");
        // The battle ends here before Angelo attacks.
    }

    else // Metal King stays, and Angelo attacks.
    {
        // 70% chance of MISS, 20% of ONE damage, 10% of Critical.

        var MissOneCritical = 1 + Math.floor(Math.random() * 10);

        if (MissOneCritical <= 7) {
            alert(MonsterName + " swiftly evades Angelo's attack.");
        }

        else if (MissOneCritical <= 9) {
            alert("Angelo inflicts 1 point of damage on " + MonsterName + ".");
        }

        else {
            // The damage dealt by a critical hit can range from 197 to 256 points.

            var CriticalRange = 197 + Math.floor(Math.random() * 60);

            alert("Angelo lands a critical hit on " + MonsterName + ", dealing massive \
            damage of " + CriticalRange + " points! Angelo defeats " + MonsterName + ".");

            // When defeated, Metal King drops a shield with an 80% probability.

            var ShieldDrop = 1 + Math.floor(Math.random() * 10);

            if (ShieldDrop <= 8) {
                alert(MonsterName + " drops a shield. Angelo obtains a Diamond Shield!");                
            }
            else {
            // No code is written in this else portion, since Metal King didn't drop \
            // any item.  
            }
        }
    }
};

// Finally, call the function.

WhichOneToAttack(MetalKingABC);
\$\endgroup\$
9
  • \$\begingroup\$ Are you concerned about how you're using if/else, or that you're using them? \$\endgroup\$
    – anon
    Commented Dec 9, 2015 at 2:17
  • \$\begingroup\$ @QPaysTaxes Actually, on both counts. I ended up with quite a lot of "if / else" statements, so I was wondering if they were used in an acceptable manner as well as if I could get around its repeated use in favour of some other code. \$\endgroup\$ Commented Dec 9, 2015 at 4:11
  • \$\begingroup\$ if/else statements are really common, and using lots is almost never a symptom of bad code. \$\endgroup\$
    – anon
    Commented Dec 9, 2015 at 4:12
  • 2
    \$\begingroup\$ I am confused. Is Metal King a king who is made of metal, or the king of all metals? or perhaps both? \$\endgroup\$ Commented Dec 9, 2015 at 7:04
  • 1
    \$\begingroup\$ @EricLippert I suppose the King of Metal is a far cousin of Michael Jackson :) \$\endgroup\$ Commented Dec 9, 2015 at 11:44

2 Answers 2

5
\$\begingroup\$

First, your game doesn't have a name! (Which is why I renamed the question like that...)

A few direct things to focus on:

if (MetalKingABC === 1) {
    MetalKingABC = "Metal King A";
}
else if (MetalKingABC === 2) {
    MetalKingABC = "Metal King B";
}
else {
    MetalKingABC = "Metal King C";
}

This is not how you do that.

Instead of using 1 + Math.floor get rid of the 1 as 0 is a valid array index, and is helpful in many situations.

Instead of using * 2, alongside an array, use * array.length.

Instead of using an if-else loop with duplicate logic (string assignment), use an array instead.

var metalKingTypes = ['A', 'B', 'C'];
var metalKing = metalKingTypes[Math.floor(Math.random() * metalKingTypes.length)];

Instead of using MissOneCritical: use a switch

After you decide who to attack, and then attack them, the game is over! Even if you miss or only do 1 damage!

Set a isActive variable to keep the game going.

A few indirect things:

  • WhichOneToAttack: could be better named: DecideOpponent
  • FleeOrFight: You naming structure should be camelCase where the first word is entirely lowercase.
  • You can use this to get a random true or false value: Math.floor(Math.random() * 2)

Your structure:

You have a bunch of loose functions that aren't linked to one another in any other way than the fact that they call each other.

Use a class structure, or a prototype chain, that way you can avoid passing parameters around:

Taking those changes into consideration would give you something like this:

    function Game(){
        alert("Metal Kings (A, B, and C) have appeared!");
            
        /* Which Metal King (A, B, or C) will Angelo decide to attack?
         * 33% probability for each.
         */
            
    
        var metalKingTypes = ['A', 'B', 'C'];
        this.monster = "Metal King " + metalKingTypes[Math.floor(Math.random() * metalKingTypes.length)];
        this.isActive = true;
        while (this.isActive) {
            this.DecideOpponent();
        }
    }
    Game.prototype.DecideOpponent = function(){
        alert("Angelo is about to attack " + this.monster + ".");
    
        // Metal King has a 50% chance of fleeing the battle.

        var fleeOrFight = Math.floor(Math.random() * 2);
    
        if (fleeOrFight) {
            alert(this.monster + " flees like lightning!");
            // The battle ends here before Angelo attacks.
            return false;
        }
        // 70% chance of MISS, 20% of ONE damage, 10% of Critical.
        var attackOptions = ['evade', 'normal', 'critical'];
    
        var attack = attackOptions[Math.floor(Math.random() * attackOptions.length)];
        switch (attack) {
            case 'evade':
                alert(this.monster + " swiftly evades Angelo's attack.");
                break;
            case 'normal':
                alert("Angelo inflicts 1 point of damage on " + this.monster + ".");
                break;
            case 'critical':
                var CriticalRange = 197 + Math.floor(Math.random() * 60);
    
                alert("Angelo lands a critical hit on " + this.monster + ", dealing massive " +
                      "damage of " + CriticalRange + " points! Angelo defeats " + this.monster + ".");
    
                // When defeated, Metal King drops a shield with an 80% probability.
    
                var shieldDrop = Math.floor(Math.random() * 2);
    
                if (shieldDrop) {
                    alert(this.monster + " drops a shield. Angelo obtains a Diamond Shield!");
                }
                this.isActive = false;
                break;
        }
    };
    
    // Finally, call the function.
    
    var g = new Game();


Additionally, instead of using alert consider building each line of the game to the console or to a HTML element.

Something like:

function buildToHTML(text){
    var field = document.getElementById('gameField');
    field.text += text;
}
\$\endgroup\$
5
\$\begingroup\$

Ooh, an alert-based game. The good old days. However, for the more tech-savvy, playing with the console will be a better experience. Very... command-liney.

Now the first problem in your code is that your state has no structure. You don't know what the current state of the game is (well, you have MetalKingABC). It would be nice if you had a structure that held the game. A simple object structure like this would be fine.

var game_state = {
  turn: 'player2', // add as many players
  pase: 'ACTION_PHASE', // INITIAL_PHASE, ACTION_PHASE, BATTLE_PHASE, END PHASE
  player1: {
    name: 'Angelo',
    damage: 25,
    health: 100,
    mana: 100,
    critRate: 0.10,
    evasion: 0.25,
    // and so on
  },
  player2: {
    name: 'MetalKingX',
    damage: 25,
    health: 100,
    mana: 100,
    critRate: 0.10,
    evasion: 0.25,
    // and so on
  }
};

Given the info above, I can easily set up rendering commands to show the name of the player, current stats and if any battle happens, I can simply look up the attributes and do calculations. If I wanted to know who's turn it is, I can look at turn and run the appropriate commands.

function render(gameState){
  // Based on data, write whatever you like to the page
}

With the above, the next issue is generating the player data. You can do that by creating a "default" object containing the base stats. You can then use Object.assign to copy over the data into another object for use in the game state. The same can be done with the player.

var metalKings = [{
  name: 'Metal King A',
  damage: 200,
 ...
},{
  name: 'Metal King B',
  damage: 200,
 ...
},{
  name: 'Metal King C',
  damage: 200,
 ...
}]

// Select a metal king randomly and copy over its base data.
Object.assign(gameState.player2, metalKings[Math.random() * 3 | 0]);

So now we have covered game state and character creation, let's move to your logic... which doesn't appear to have structure. So let's start by creating your logic.

Game state mutation is just a result of calculation. Given some input, it mutates the data. With that, we can extract operations into function that take in data, and based on that data, calculates the outcome. The key take away in this is that they should be "transparent". Given the same input, the operation should always give the same output, no matter when it's called.

function getRemainingHealth(p1Damage, p2Hp, p2Armor){
  return p2Hp - (p1Damage - p2Armor);
}

function evade(evasionRate){
  // 100% evasion is 1, no evasion is 0. Math.random() range is 0-1.
  return Math.random() <= evasionRate;
}

function crit(critRate){
  return Math.random() <= critRate;
}

function critDamage(unitBaseDamage, critDamage){
  return unitBaseDamage * critDamage;
}

Also notice that the game state only holds "base data" (hp, damage, rates etc.) Critical strike damage is an example of "derived data". You don't store the crit damage value, just it's constituents (base damage, crit rate, and crit percentage). This prevents your data from going out of sync.

Now the basics are covered, can then formulate your state machine

  1. INITIAL: render ui, prep data etc.
  2. render (initial ui on turn start)
  3. ACTION: select action
  4. render (reflect the action taken by player)
  5. BATTLE: compute stuff (damage, damage reductions, crits, remaining hps etc.)
  6. render (this is where the fireworks happen)
  7. END: sign off (post battle effects, buffs/debuffs, change turn etc.)
  8. render (post-battle effects, hand over messages, end of battle messages etc.)
  9. repeat

Each of these can be separate functions, which passes in the game state. Each phase function can then mutate the state.

function doStuff(phase, gameState){
  switch(phase):
    case: 'INITIAL'
      doInitialPhase(gameState);
      break;
    case: 'ACTION'
      doActionPhase(gameState);
      break;
    case: 'BATTLE'
      doBattlePhase(gameState);
      break;
    case: 'END'
      doEndPhase(gameState);
      break;
}

// Battle phase pseudo code
function doBattlePhase(state){
  console.log(state.turn + 'is attacking');
  if(!didEvade(p2)) state[p2].hp = calculateRemainingHp(p1Damage);
  ...and so on...
}

Now notice that I never made mention of an "enemy". That's right. That's because in a game, your enemy is also a player. Yes, and that's how AI games and multiplayer games work. Instead of an actual player dispatching commands, the game gets commands from an AI engine, or over the network (from some remote player). The game doesn't make a distinction of who's doing what. All the game will ever know is that some action does some changes to some data.

\$\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.