4
\$\begingroup\$

I decided to make a simple RPG game to practice JavaScript. I don't know a lot about JS best practices or things like that, so I rather have my code criticized now than build more code on top of bad habits. I'd be greatful for any advice.

document.addEventListener("DOMContentLoaded", function(){
  showStats();
});

let player = {
  stats : {
    "level" : 1,
    "experience" : 0,
    "maxHp" : 100,
    "currentHp" : 100,
    "maxEnergy" : 100,
    "currentEnergy" : 100,
    "maxMana" : 100,
    "currentMana" : 100,
    "gold" : 100,
    "diamonds" : 0,
  },
  skills : {
    fighting : {
      "lvl" : 1,
      "exp" : 0,
    },
    spellcasting : {
      "lvl" : 1,
      "exp" : 0,
    },
    archery : {
      "lvl" : 1,
      "exp" : 0,
    },
    fishing : {
      "lvl" : 1,
      "exp" : 0,
    },
    mining : {
      "lvl" : 1,
      "exp" : 0,
    },
    crafting : {
      "lvl" : 1,
      "exp" : 0,
    },
  }
}

const skillExp = {
  1 : 0,
  2 : 500,
  3 : 1100,
  4 : 2000,
  5 : 3500,
  6 : 5500,
  7 : 8500,
  8 : 13000,
  9 : 19000,
  10 : 27000,
  11 : 37000,
}

let minedGold = 0;
let info = "";

function main(){
  mine();
  showStats();
}

// REFILL PLAYERS ENERGY
function addEnergy(){
  player.stats.currentEnergy = 100;
  player.stats.gold -= 100;
  document.getElementById('info').innerHTML = "";
  showStats();
}

// MINE ACTION
function mine(){
  if(player.stats.currentEnergy >= 10){
    info = "";
    minedGold = Math.floor((Math.random()*100)+(player.skills.mining.lvl * 2));
    player.stats.gold += minedGold;
    player.stats.currentEnergy -= 10;
    calculateExp(player.skills.mining, minedGold);
    mineInfo();
  }
  else info = "Not enough energy";
  mineInfo();
}

// DISPLAY MINING COMUNICATES
function mineInfo(){
  document.getElementById('current-gold').innerHTML = minedGold;
  document.getElementById('info').innerHTML = info;
}

// ADD EXP AND CHECK FOR LVL UP
function calculateExp(skillName, expGained){
  skillName.exp += expGained;
  var a;
  for(a in skillExp){
   a = parseInt(a);
   if(skillExp[a] <= skillName.exp){
      skillName.lvl = a;
    }
  }
}

// DISPLAY UPDATED STATS
function showStats(){
  document.getElementById('level').innerHTML = player.stats.level;
  document.getElementById('totalExp').innerHTML = player.stats.experience;
  document.getElementById('currentHp').innerHTML = player.stats.currentHp;
  document.getElementById('maxHp').innerHTML = player.stats.maxHp;
  document.getElementById('currentEnergy').innerHTML = player.stats.currentEnergy;
  document.getElementById('maxEnergy').innerHTML = player.stats.maxEnergy;
  document.getElementById('gold').innerHTML = player.stats.gold;
  document.getElementById('diamonds').innerHTML = player.stats.diamonds;
  document.getElementById('fighting').innerHTML = player.skills.fighting.lvl;
  document.getElementById('spellcasting').innerHTML = player.skills.spellcasting.lvl;
  document.getElementById('archery').innerHTML = player.skills.archery.lvl;
  document.getElementById('fishing').innerHTML = player.skills.fishing.lvl;
  document.getElementById('mining').innerHTML = player.skills.mining.lvl;
  document.getElementById('crafting').innerHTML = player.skills.crafting.lvl;
}
body{
  background-color:#eee;
}

p span{
  color:green;
}

p:nth-child(1) span{
  color:blue;
}


main{
  display: flex;
  flex-flow: row wrap;
  justify-content: space-between;
}

#statistics{
  display: flex;
  flex-flow: row wrap;
}

#statistics div{
  margin: 0 10px;
  border: solid 1px black;
  padding: 20px;
}
<html>
<head>
  <meta charset="utf-8"/>
  <link rel="stylesheet" href="style.css" />
</head>
<body>
<main>
  <section>
    <p>Gold mined: <span id="current-gold">0</span></p>
    <button onclick="main()">Mine</button>
    <button onclick="addEnergy()">Get energy drink (100 gold)</button>
    <p id="info"></p>
  </section>
  <section id="statistics">
    <div>
      <h3>Statistics</h3>
      <p>Level: <span id="level"></span></p>
      <p>Total experience: <span id="totalExp"></span></p>
      <p>Health: <span id="currentHp"></span>/<span id="maxHp"></span></p>
      <p>Energy: <span id="currentEnergy"></span>/<span id="maxEnergy"></span></p>
      <p>Gold: <span id="gold"></span></p>
      <p>Diamonds: <span id="diamonds"></span></p>
    </div>
    <div>
      <h3>Skills</h3>
      <p>Fighting: <span id="fighting"></span></p>
      <p>Spellcasting: <span id="spellcasting"></span></p>
      <p>Archery: <span id="archery"></span></p>
      <p>Fishing: <span id="fishing"></span></p>
      <p>Mining: <span id="mining"></span></p>
      <p>Crafting: <span id="crafting"></span></p>
    </div>
  </section>
</main>

<script src="main.js"></script>
</body>
</html>

Improve this question
\$\endgroup\$
6
  • \$\begingroup\$ I noticed a bug: when you buy an energy drink, your amount of gold stays the same. \$\endgroup\$
    – Solomon Ucko
    Mar 16, 2018 at 12:07
  • \$\begingroup\$ Oh indeed, must have overlooked when changing code because it worked before, thanks for noticing! \$\endgroup\$
    – aMJay
    Mar 16, 2018 at 12:08
  • \$\begingroup\$ You're welcome. Changing code? \$\endgroup\$
    – Solomon Ucko
    Mar 16, 2018 at 12:12
  • \$\begingroup\$ yes I removed it for now since I'ts being displayed in stats window anyway \$\endgroup\$
    – aMJay
    Mar 16, 2018 at 12:14
  • \$\begingroup\$ Yep, it works now. \$\endgroup\$
    – Solomon Ucko
    Mar 16, 2018 at 12:19

1 Answer 1

Reset to default
5
\$\begingroup\$

You have a nice little project there.

As you will be expanding it for other skills, experience, fighting, etc. I thought I'd chip in with a few pointers.

I've left the HTML and CSS alone (they seem fine anyway) and concentrated on the javascript to streamline things as you develop this app.

All changes and advice are in comments in the code as I think it's easier to see where and why they are there.

Hope this helps.

document.addEventListener("DOMContentLoaded", function () {
    showStats();
});

let player = {
    stats: {
        "level": 1,
        "experience": 0,
        "maxHp": 100,
        "currentHp": 100,
        "maxEnergy": 100,
        "currentEnergy": 100,
        "maxMana": 100,
        "currentMana": 100,
        "gold": 100,
        "diamonds": 0,
    },
    skills: {
        fighting: {
            "lvl": 1,
            "exp": 0,
        },
        spellcasting: {
            "lvl": 1,
            "exp": 0,
        },
        archery: {
            "lvl": 1,
            "exp": 0,
        },
        fishing: {
            "lvl": 1,
            "exp": 0,
        },
        mining: {
            "lvl": 1,
            "exp": 0,
        },
        crafting: {
            "lvl": 1,
            "exp": 0,
        },
    }
}; // Missing semicolon

const skillExp = {
    1: 0,
    2: 500,
    3: 1100,
    4: 2000,
    5: 3500,
    6: 5500,
    7: 8500,
    8: 13000,
    9: 19000,
    10: 27000,
    11: 37000,
}; // Missing semicolon

// Because skillExp is an object, not an array, we have to get the keys (as an array), and get the last value.
// Saves performing this lookup (on a static set of values) every time it is needed.
// If you add more levels, this will always return the last one.
const maxSkillLevel = parseInt(Object.keys(skillExp)[Object.keys(skillExp).length - 1]);

// Use a shortcut to get to the common things you wish to access; reduces code and easier to read.
// Could reduce further to gold, currentEnergy, etc. if you think it applicable.
let stats = player.stats;
let skills = player.skills;

// Get the DOM Elements into variables once; not every time you want to access them.
let elemLevel = document.getElementById('level');
let elemTotalExp = document.getElementById('totalExp');
let elemCurrentHp = document.getElementById('currentHp');
let elemMaxHp = document.getElementById('maxHp');
let elemCurrentEnergy = document.getElementById('currentEnergy');
let elemMaxEnergy = document.getElementById('maxEnergy');
let elemGold = document.getElementById('gold');
let elemDiamonds = document.getElementById('diamonds');
let elemFighting = document.getElementById('fighting');
let elemSpellcasting = document.getElementById('spellcasting');
let elemArchery = document.getElementById('archery');
let elemFishing = document.getElementById('fishing');
let elemMining = document.getElementById('mining');
let elemCrafting = document.getElementById('crafting');
let elemCurrentGold = document.getElementById('current-gold');
let elemInfo = document.getElementById('info');

// Remove main function - not required. Call mine function directly.

// REFILL PLAYERS ENERGY
function addEnergy() {
    stats.currentEnergy = 100;
    stats.gold -= 100;
    elemInfo.textContent = "";
    showStats();
}

// MINE ACTION
function mine() {
    // Removed minedGold and info variables from global scope, and now using them directly where needed.
    // Pass the minedGold and message values on to any function that requires them..
    var minedGold = 0;
    var message = "";
    if (stats.currentEnergy >= 10) {
        minedGold = Math.floor((Math.random() * 100) + (skills.mining.lvl * 2));
        stats.gold += minedGold;
        stats.currentEnergy -= 10;
        calculateExp(skills.mining, minedGold);
        showStats(); // Update stats when finished mining. No need to call this if mining didn't occur.
    }
    else {
        message = "Not enough energy";
    }
    // Removed call to mineInfo() from first if block;
    // Previously, (without curly braces around second if block), 
    // the second mineInfo() would be called even if it was called in the first block.
    // Only the first line of code after an if/else etc. will be called when matched, 
    // everything after will run as normal code - because not encased in a block.
    mineInfo(message, minedGold);
}

// DISPLAY MINING COMUNICATES
function mineInfo(message, minedGold) {
    // Pass in any message to display, rather than relying on a global variable (that some other action may change).
    // Same for minedGold.
    // This way, this function doesn't have to rely on variables outside of its scope, 
    // that could change or move in the future - Separation Of Concerns.
    elemCurrentGold.textContent = minedGold;
    elemInfo.textContent = message;
}

// ADD EXP AND CHECK FOR LVL UP
function calculateExp(skillName, expGained) {
    skillName.exp += expGained;
    // Update total experience too.
    stats.experience += expGained;
    // Remove loop. No need to iterate over every item in skillExp comparing values.

    // If player has more experience than the next level requires, level up.
    // A while loop allows for the case where the player's experience jumps more than one level from an experience gain.
    // And an additional check that the player's level is not at max.
    while (skillName.lvl < maxSkillLevel && skillName.exp > skillExp[skillName.lvl + 1]) {
        skillName.lvl++;
    }
}

// DISPLAY UPDATED STATS
function showStats() {
    // Use textContent rather than innerHTML.
    // If you need to place style tags etc. in your text (e.g <b>Hello world</b>), then innerHTML is the way to go.
    // But you should try to style the elements themselves rather than the text within.
    elemLevel.textContent = stats.level;
    elemTotalExp.textContent = stats.experience;
    elemCurrentHp.textContent = stats.currentHp;
    elemMaxHp.textContent = stats.maxHp;
    elemCurrentEnergy.textContent = stats.currentEnergy;
    elemMaxEnergy.textContent = stats.maxEnergy;
    elemGold.textContent = stats.gold;
    elemDiamonds.textContent = stats.diamonds;
    elemFighting.textContent = skills.fighting.lvl;
    elemSpellcasting.textContent = skills.spellcasting.lvl;
    elemArchery.textContent = skills.archery.lvl;
    elemFishing.textContent = skills.fishing.lvl;
    elemMining.textContent = skills.mining.lvl;
    elemCrafting.textContent = skills.crafting.lvl;
}
Improve this answer
\$\endgroup\$

Your Answer

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

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