10
\$\begingroup\$

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

    <html>
       <head>
             <title>Flatten your purse to least values</title>
             <style>
             #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;
             }
             </style>
          </head>
          <body>
          <h1>Flatten your purse to least values and best gold</h1>
          <p>
             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
          </p>
          <p>Select the DnD Edition</p>
          <form>
             <select id="edition" onchange="changeedition(this.value)">
                <option value=5>5th Edition</option>
                <option value=4>4th Edition</option>
             </select>
          </form>
          <table id="coins">
             <tr>
                <th>Type</th>
                <th>Value</th>
                <th>Weight</th>
                <th>x to CP</th>
                <th>in CP</th>
                <th>Weight in cp</th>
                <th>Optimum</th>
                <th>Weight in optimum</th>
                <th>Optimum in gp</th>
                <th>Weight in opto gp</th>
             </tr>
             <tr id="astral">
                <td>Astral Diamond:</td>
                <td><input id="ap" onchange="flatten(this)" type="text"></td>
                <td id="ap_weight"></td>
                <td id="ap_factor"></td>
                <td id="ap_in_cp"></td>
                <td id="ap_cp_weight"></td>
                <td id="ap_opto"></td>
                <td id="ap_opto_weight"></td>
                <td id="ap_opto_gp"></td>
                <td id="ap_opto_gp_weight"></td>
             </tr>
             <tr>
                <td>Platinum:</td>
                <td><input id="pp" onchange="flatten(this)" type="text"></td>
                <td id="pp_weight"></td>
                <td id="pp_factor"></td>
                <td id="pp_in_cp"></td>
                <td id="pp_cp_weight"></td>
                <td id="pp_opto"></td>
                <td id="pp_opto_weight"></td>
                <td id="pp_opto_gp"></td>
                <td id="pp_opto_gp_weight"></td>
             </tr>
             <tr>
                <td>Gold:</td>
                <td><input id="gp" onchange="flatten(this)" type="text"></td>
                <td id="gp_weight"></td>
                <td id="gp_factor"></td>
                <td id="gp_in_cp"></td>
                <td id="gp_cp_weight"></td>
                <td id="gp_opto"></td>
                <td id="gp_opto_weight"></td>
                <td id="gp_opto_gp"></td>
                <td id="gp_opto_gp_weight"></td>
             </tr>
             <tr id="electrum">
                <td>Electrum:</td>
                <td><input id="ep" onchange="flatten(this)" type="text"></td>
                <td id="ep_weight"></td>
                <td id="ep_factor"></td>
                <td id="ep_in_cp"></td>
                <td id="ep_cp_weight"></td>
                <td id="ep_opto"></td>
                <td id="ep_opto_weight"></td>
                <td id="ep_opto_gp"></td>
                <td id="ep_opto_gp_weight"></td>
             </tr>
             <tr>
                <td>Silver:</td>
                <td><input id="sp" onchange="flatten(this)" type="text"></td>
                <td id="sp_weight"></td>
                <td id="sp_factor"></td>
                <td id="sp_in_cp"></td>
                <td id="sp_cp_weight"></td>
                <td id="sp_opto"></td>
                <td id="sp_opto_weight"></td>
                <td id="sp_opto_gp"></td>
                <td id="sp_opto_gp_weight"></td>
             </tr>
             <tr>
                <td>Copper:</td>
                <td><input id="cp" onchange="flatten(this)" type="text"></td>
                <td id="cp_weight"></td>
                <td id="cp_factor"></td>
                <td id="cp_in_cp"></td>
                <td id="cp_cp_weight"></td>
                <td id="cp_opto"></td>
                <td id="cp_opto_weight"></td>
                <td id="cp_opto_gp"></td>
                <td id="cp_opto_gp_weight"></td>
             </tr>
             <tr>
                <td colspan=2><b>Total:</td>
                <td id="total_weight"></td>
                <td>&nbsp;</td>
                <td id="total_in_cp"></td>
                <td id="total_cp_weight"></td>
                <td id="total_opto"></td>
                <td id="total_opto_weight"></td>
                <td id="total_opto_gp"></td>
                <td id="total_opto_gp_weight"></td>
             </tr>
          </table>
          <p><b>This webserver carries Clacks overhead - <a href="http://www.gnuterrypratchett.com/" target=_blank>About Clacks Overhead</a></b></p>
    
          <script>
             var conversion=[];
             changeedition();
             var goldindex=1;
    
             function changeedition()
             {
                var i=0;
                switch(document.getElementById('edition').value)
                {
                   case "4":
                      document.getElementById('astral').style.display="table-row";
                      document.getElementById('electrum').style.display="none";
                      conversion[i++]={tag:'ap', factor:1000000,   weight:0.032};
                      conversion[i++]={tag:'pp', factor:1000,      weight:0.32};
                      conversion[i++]={tag:'gp', factor:100,       weight:0.32};
                      conversion[i++]={tag:'sp', factor:10,        weight:0.32};
                      conversion[i++]={tag:'cp', factor:1,         weight:0.32};
                      goldindex=2;
                      break;
                   case "5":
                   default:
                      document.getElementById('astral').style.display="none";
                      document.getElementById('electrum').style.display="table-row";
                      conversion[i++]={tag:'pp', factor:1000, weight:0.32};
                      conversion[i++]={tag:'gp', factor:100,  weight:0.32};
                      conversion[i++]={tag:'ep', factor:50,   weight:0.32};
                      conversion[i++]={tag:'sp', factor:10,   weight:0.32};
                      conversion[i++]={tag:'cp', factor:1,    weight:0.32};
                      goldindex=1;
                      break;
                }
                flatten(document.getElementById('pp'));
             }
    
             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.";
             }
    
             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);
             }
    
             function totalColumn(columnPrefix, table)
             {
                var total=0;
                for(var key in table)
                {
                   var varname=table[key].tag + columnPrefix;
                   total+=Number(document.getElementById(varname).innerHTML);
                }
                document.getElementById('total' + columnPrefix).innerHTML=total;
             }
    
             function flatten(obj)
             {
                var entry;
                for(var key in conversion)
                {
                   if(conversion[key].tag==obj.id)
                   {
                      entry=conversion[key]; 
                   }
                }
    
                document.getElementById(obj.id + '_in_cp').innerHTML = entry.factor * obj.value;
                document.getElementById(obj.id + '_factor').innerHTML = 'x ' + entry.factor;
                totalColumn('_in_cp', conversion);
                optomize(conversion, document.getElementById('total_in_cp').innerHTML, '_opto', 0);
                optomize(conversion, document.getElementById('total_in_cp').innerHTML, '_opto_gp', goldindex);
                var total_weight=0;
                var total_cp_weight=0;
                var total_opto_weight=0;
                var total_opto_gp_weight=0;
                for(var key in conversion)
                {
                   var tag=conversion[key].tag;
                   entry = conversion[key];
                   total_weight         += displayWeight(tag + '_weight', document.getElementById(tag).value, entry.weight);
                   total_cp_weight      += displayWeight(tag + '_cp_weight', entry.factor * document.getElementById(tag).value, entry.weight);
                   total_opto_weight    += displayWeight(tag + '_opto_weight', document.getElementById(tag + '_opto').innerHTML, conversion[key].weight);
                   total_opto_gp_weight += displayWeight(tag + '_opto_gp_weight', document.getElementById(tag + '_opto_gp').innerHTML, conversion[key].weight);
                }
                displayWeightOz('total_weight', total_weight);
                displayWeightOz('total_cp_weight', total_cp_weight);
                displayWeightOz('total_opto_weight', total_opto_weight);
                displayWeightOz('total_opto_gp_weight', total_opto_gp_weight);
             }
    
          </script>
    
       </body>
    </html>

\$\endgroup\$
7
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feed back - I've added an edit to original post. I'll try reworking it with your advice (and better spelling :-) - and repost a new version. And I'll add some comments so it is more readable. \$\endgroup\$ – Adrian May 4 '15 at 2:50

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.