0

I made a simple D&D dice roll application for practice and noticed there is a lot of repetitive code. I can't find a good way to refactor it myself though. Any tips are appreciated. Fair warning, I'm a noob so I already know it's probably got more problems than I'm aware of. Feel free to really let me have it.

https://jsfiddle.net/sakonexus/nd82qjec/

function calcRoll(e) {
  let die = e.target.previousElementSibling.name;
  let rolls = e.target.previousElementSibling.value;
  let topRow = {};
  let bottomRow = {};
  let randomRoll = 0;
  
  if (rolls == '') {
    rolls = 0;
  }
  
  if (die.includes("d4")) {
    topRow = document.getElementById("d4-roll-results").insertRow(0);
    bottomRow = document.getElementById("d4-roll-results").insertRow(1);
    topRow.insertCell(0).innerHTML = "Roll Count";
    bottomRow.insertCell(0).innerHTML = "Roll Result";
    
    for(i = 0; i < rolls; i++) {
      topRow.insertCell(i + 1).innerHTML = i + 1;
    }
    
    for(j = 0; j < rolls; j++) {
        randomRoll = Math.floor(Math.random() * 4) + 1;
        bottomRow.insertCell(j + 1).innerHTML = randomRoll;
    }
  } else if (die.includes("d6")) {
    topRow = document.getElementById("d6-roll-results").insertRow(0);
    bottomRow = document.getElementById("d6-roll-results").insertRow(1);
    topRow.insertCell(0).innerHTML = "Roll Count";
    bottomRow.insertCell(0).innerHTML = "Roll Result";
    
    for(i = 0; i < rolls; i++) {
      topRow.insertCell(i + 1).innerHTML = i + 1;
    }
    
    for(j = 0; j < rolls; j++) {
        randomRoll = Math.floor(Math.random() * 6) + 1;
        bottomRow.insertCell(j + 1).innerHTML = randomRoll;
    }
  } else if (die.includes("d8")) {
    topRow = document.getElementById("d8-roll-results").insertRow(0);
    bottomRow = document.getElementById("d8-roll-results").insertRow(1);
    topRow.insertCell(0).innerHTML = "Roll Count";
    bottomRow.insertCell(0).innerHTML = "Roll Result";
    
    for(i = 0; i < rolls; i++) {
      topRow.insertCell(i + 1).innerHTML = i + 1;
    }
    
    for(j = 0; j < rolls; j++) {
        randomRoll = Math.floor(Math.random() * 8) + 1;
        bottomRow.insertCell(j + 1).innerHTML = randomRoll;
    }
  } else if (die.includes("d10")) {
    topRow = document.getElementById("d10-roll-results").insertRow(0);
    bottomRow = document.getElementById("d10-roll-results").insertRow(1);
    topRow.insertCell(0).innerHTML = "Roll Count";
    bottomRow.insertCell(0).innerHTML = "Roll Result";
    
    for(i = 0; i < rolls; i++) {
      topRow.insertCell(i + 1).innerHTML = i + 1;
    }
    
    for(j = 0; j < rolls; j++) {
        randomRoll = Math.floor(Math.random() * 10);
        bottomRow.insertCell(j + 1).innerHTML = randomRoll;
    }
  } else if (die.includes("d12")) {
    topRow = document.getElementById("d12-roll-results").insertRow(0);
    bottomRow = document.getElementById("d12-roll-results").insertRow(1);
    topRow.insertCell(0).innerHTML = "Roll Count";
    bottomRow.insertCell(0).innerHTML = "Roll Result";
    
    for(i = 0; i < rolls; i++) {
      topRow.insertCell(i + 1).innerHTML = i + 1;
    }
    
    for(j = 0; j < rolls; j++) {
        randomRoll = Math.floor(Math.random() * 12) + 1;
        bottomRow.insertCell(j + 1).innerHTML = randomRoll;
    }
  } else if (die.includes("d20")) {
    topRow = document.getElementById("d20-roll-results").insertRow(0);
    bottomRow = document.getElementById("d20-roll-results").insertRow(1);
    topRow.insertCell(0).innerHTML = "Roll Count";
    bottomRow.insertCell(0).innerHTML = "Roll Result";
    
    for(i = 0; i < rolls; i++) {
      topRow.insertCell(i + 1).innerHTML = i + 1;
    }
    
    for(j = 0; j < rolls; j++) {
        randomRoll = Math.floor(Math.random() * 20) + 1;
        bottomRow.insertCell(j + 1).innerHTML = randomRoll;
    }
  }
  
  
}
Sako
  • 95
  • 1
  • 10

1 Answers1

0

To start with, you should avoid inline handlers, they have way too many problems to be worth using. Attach listeners properly using JavaScript with addEventListener instead.

On the click of a button, navigate to its parent form or div to identify the type of dice. Then, from the parent div, navigate to the table child of the div and populate it:

document.querySelector('#dice').addEventListener('click', (e) => {
  if (!e.target.matches('button')) {
    return;
  }
  e.preventDefault();
  const { target } = e;
  const div = target.closest('div');
  const dieType = div.dataset.dieType;
  const numDice = target.previousElementSibling.value;
  const table = div.querySelector('table');
  table.innerHTML += `
    <tr>
      <td>Roll Count</td>
      ${Array.from({ length: numDice }, (_, i) => `<td>${i + 1}</td>`).join('')}
    </tr>
    <tr>
      <td>Roll Result</td>
      ${Array.from({ length: numDice }, () => `<td>${Math.floor(Math.random() * dieType) + 1}</td>`).join('')}
    </tr>
  `;
});
#dice table {
  border-collapse: collapse;
}
#dice td {
  border: 1px solid black;
  padding: 4px;
}

#dice div {
  padding: 10px 0 0;
}
<div>
  <h1>Select the number of rolls for each dice</h1>
  <div id="dice">
    <div data-die-type="4">
      <form>
        <label><span>D4&#160;&#160;</span><input type="number" min="0" max="10"><button>Confirm</button></label>
      </form>
      <div><table></table></div>
    </div>
    <hr />
    <div data-die-type="6">
      <form>
        <label><span>D6&#160;&#160;</span><input type="number" min="0" max="10"><button>Confirm</button></label>
      </form>
      <div><table></table></div>
    </div>
    <hr />
    <div data-die-type="8">
      <form>
        <label><span>D8&#160;&#160;</span><input type="number" min="0" max="10"><button>Confirm</button></label>
      </form>
      <div><table></table></div>
    </div>
    <hr />
    <div data-die-type="10">
      <form>
        <label><span>D10</span><input type="number" min="0" max="10"><button>Confirm</button></label>
      </form>
      <div><table></table></div>
    </div>
    <hr />
    <div data-die-type="12">
      <form>
        <label><span>D12</span><input type="number" min="0" max="10"><button>Confirm</button></label>
      </form>
      <div><table></table></div>
    </div>
    <hr />
    <div data-die-type="20">
      <form>
        <label><span>D20</span><input type="number" min="0" max="10"><button>Confirm</button></label>
      </form>
      <div><table></table></div>
    </div>
  </div>
</div>

See how your HTML can be made dramatically more DRY as well - there's no need for all those different IDs and attributes.

CertainPerformance
  • 356,069
  • 52
  • 309
  • 320
  • Thanks for this! I will need to study it and try to apply this type of approach for future projects. – Sako Aug 28 '20 at 18:22