Dungeons and Dragons money optimizer

I wrote this little page to calculate the least weight and/or optimum gold amounts for DnD for 4th and 5th editions.

I think I've managed to refactor it to its best, but I have little experience with JavaScript so I would like a critical review of what I have done so far.

Public version to try



`

Flatten your purse to least values

#coins {
font-family: "Trebuchet MS", Arial, Helvetica, sans-serif;
border-collapse: collapse;
}

#coins td, #coins th {
font-size: 1em;
border: 1px solid #98bf21;
padding: 3px 7px 2px 7px;
}

#coins th {
font-size: 1.1em;
text-align: left;
padding-top: 5px;
padding-bottom: 4px;
background-color: #A7C942;
color: #ffffff;
}

#coins tr.alt td {
color: #000000;
background-color: #EAF2D3;
}



Flatten your purse to least values and best gold


Just enter the values in your purse and it (should) calculate the least amount of coins needed. To make sure it calc's properly click outside the table. Enter you coins into the value column


Select the DnD Edition


5th Edition
4th Edition




Type
Value
Weight
x to CP
in CP
Weight in cp
Optimum
Weight in optimum
Optimum in gp
Weight in opto gp


Astral Diamond:

I don't play D&D, so I have little-to-no idea what's going on here, but code is code, so I'll give it a shot.

I notice this question has been slow to receive a review, and I think that's partly because the structure of the things makes it hard to follow. You have to constantly refer back and forth between the HTML table's structure, the conversion "table", opaque arguments, and raw values.

To be blunt, I don't know for sure what this is supposed to do. Again, I don't play D&D, but I imagine the idea is to take whatever money you have, and redistribute it into different types of currency of the same total value but lower weight. Like taking 4 quarters, and changing it to a $1 bill? Maybe?

Point is, it's not super clear. For instance, the "main" function seems to be this:

function optomize(table, value, fieldend, start)
{
  var i=start;
  do
  {
    document.getElementById(table[i].tag + fieldend).innerHTML=Math.floor(value / table[i].factor);
    value-=Math.floor(value / table[i].factor) * table[i].factor;
    ++i;
    if(i==table.length)
    {
      i=0;
    }
  } while(i!=start);
}

To read this, you need to know a naming scheme for HTML elements and have an idea of the page's layout, and have a pretty good notion of what's being passed to the function - and why. Plus, that's a funky looping loop you got there, but why?

Before I go further, though, I'll just point two low-level things:

• It's optimize with an "i"

• Most JS style guides will tell you to use brace-on-same-line style

• Some horizontal whitespace wouldn't hurt

What I'd like to see is a cleaner separation between the UI and the business logic. That'll keep the business logic clean and easier to understand.

For instance, I'd like to be able to see how all the data manipulation happens without having it being tangled up in the markup and DOM queries. And vice-versa.

As an example, you have these two functions:

function displayWeight(fieldId, coins, weight)
{
  var totalOzWeight=coins * weight;
  displayWeightOz(fieldId, totalOzWeight);
  return totalOzWeight;
}

function displayWeightOz(fieldId, weight)
{
  var lbWeight=Math.floor(weight / 16);
  var ozWeight=(weight - (lbWeight * 16)).toFixed(2);
  document.getElementById(fieldId).innerHTML=lbWeight + " lb. " + ozWeight + " oz.";
}

which mix formatting and updating the DOM. It requires you to pass an element ID (itself an indirection rather than an actual element), to a function that doesn't use it but just passes it on. And why does displayWeight return something? Its stated purpose is to "display weight", yet it returns weight formatted as whole pounds while displaying pounds and ounces on the page. It's very confusing.

I'd prefer a separate formatWeight function that just takes a number and returns a string. For instance:

function formatWeight(weight) {
  var pounds = weight / 16 | 0, // bitwise Math.floor equivalent
      ounces = weight % 16;

  return pounds + "lb. " + ounces.toFixed(2) + "oz."
}

That's it. What happens to the string after that (logging, displaying, alerting...) is a wholly separate concern.

Incidentally, the modulo operator is also useful for the coin distribution. As a simplified example:

var coinValues = [1000, 100, 50, 10, 1], // denominations ordered high to low
    total = 1231, // value to be distributed
    count;

coinValues.forEach(function (value) {
  count = total / value | 0;
  total %= value;
  console.log(count + "x " + value);
});

which'll print:

1x 1000
2x 100
0x 50
3x 10
1x 1

While I'm at it, another minor thing I noticed was the way you set up the conversion table by writing/overwriting array indices depending on whether the user chooses 4e or 5e. A simpler way would be to simply redefine the array:

conversion = [
  {tag:'ap', factor:1000000,   weight:0.032},
  {tag:'pp', factor:1000,      weight:0.32},
  // ...
];

No need to manually increment an index.

As it is, though, I frankly don't have the inclination to really dive into things, because it'd take me a long time to go through all of it.

StackExchange Code Review Q#88676, answer score: 7