46
\$\begingroup\$

As a variant on the fizzbuzz concept, and as an exercise for learning JavaScript, HTML5, and CSS (I know none of them very well).

The regular fizzbuzz is somewhat tired, but having a web-based, visualizable output seems to be useful, and being able to adjust the inputs at will, allows you to see the impact of how the modulo changes.

var fizzLoaded = false;
var fizzDiv, fizzFrom, fizzTo, fizzPlayers;

function fizzLoad() {
  if (fizzLoaded) {
    return;
  }
  fizzLoaded = true;
  var fizzForm = document.getElementById('fizzbuzz');
  fizzFrom = document.getElementById('rangeFrom');
  fizzTo = document.getElementById('rangeTo');
  fizzPlayers = [
    document.getElementById('frodo'),
    document.getElementById('sam'),
    document.getElementById('merry'),
    document.getElementById('pippin'),
    document.getElementById('bilbo')
  ];
  fizzDiv = document.getElementById('fizzOut');
}

function restrictRange() {
  var rFrom = parseInt(fizzFrom.value);
  var rTo = parseInt(fizzTo.value);
  fizzTo.min = rFrom;
  fizzFrom.max = rTo;
}

function validateValues() {
  var rFrom = parseInt(fizzFrom.value);
  var rTo = parseInt(fizzTo.value);
  if (rTo < rFrom) {
    alert("Illegal range from " + rFrom + " to " + rTo);
    return false;
  }
  for (var i = 0; i < fizzPlayers.length; i++) {
    var val = parseInt(fizzPlayers[i].value);
    if (val < 0 || val > 100) {
      alert("Illegal value " + val + " for player " + fizzPlayers[i].id);
      return false;
    }
  }
  return true;
}

function capitaliseFirstLetter(string) {
      return string.charAt(0).toUpperCase() + string.slice(1);
}

function fizzing() {
  fizzLoad();
  restrictRange();
  if (!validateValues()) {
    fizzDiv.innerHTML = "Illegal inputs";
    return;
  }

  var table = "";
  var rFrom = parseInt(fizzFrom.value);
  var rTo = parseInt(fizzTo.value);
  var active = [];
  var actfact = [];
  var actname = [];
  var player;

  for (var i = 0; i < fizzPlayers.length; i++) {
    player = fizzPlayers[i];
    var val = parseInt(player.value);
    if (val != 0) {
      active.push(player);
      actfact.push(parseInt(player.value));
      actname.push(capitaliseFirstLetter(player.id));
    }
  }

  table += "<table>\n";
  table += "  <tr><th>Value</th><th>Message</th></tr>\n";
  for (var i = rFrom; i <= rTo; i++) {

    var msg = "";
    for (var p = 0; p < active.length; p++) {
      if (i % actfact[p] == 0) {
        msg += actname[p];
      }
    }
    if (msg == "") {
      msg = "" + i;
    }
    table += "  <tr><td>" + i + "</td><td>" + msg + "</td></tr>\n";
  }
  table += "</table>\n";

  fizzDiv.innerHTML = table;
}
h1 {
    clear: left;
}

hr {
    clear: left;
}

label {
    display: inline-block;
    float: left;
    clear: left;
    width: 150px;
    text-align: left;
}
input {
  display: inline-block;
  float: right;
  text-align: right;
  padding-left:10px;
  width: 50px;
}
<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="utf-8">
    <title>FizzBuzz</title>
    <link rel="stylesheet" href="fizzbuzz.css">
    <script src="fizzbuzz.js" type="text/javascript"></script>
  </head>
  <body>
    <h1>Config</h1>
    <form id="fizzbuzz">
      <fieldset id="fizzControl" oninput="fizzing();">
        <label>Range From<input id="rangeFrom" type="number" min="1" max="100" value="1" required></label>
        <label>Range To<input id="rangeTo"   type="number" min="1" max="1024" value="100" required></label>
        <div id="players" >
          <label>Frodo<input id="frodo" type="number" min="0" max="100" value="3" required></label>
          <label>Sam<input id="sam" type="number" min="0" max="100" value="5" required></label>
          <label>Merry<input id="merry" type="number" min="0" max="100" value="0" required></label>
          <label>Pippin<input id="pippin" type="number" min="0" max="100" value="0" required></label>
          <label>Bilbo<input id="bilbo" type="number" min="0" max="100" value="0" required></label>
        </div>
      </fieldset>
    </form>
    <hr>
    <h1>Output</h1>
    <div id="fizzOut" >Change a value to get output (a snippet thing)
    <script>fizzing();</script>
  </body>
</html>

I have a number of concerns here that have frustrated me, and I am sure there are better ways to accomplish them...:

CSS:

I tried very hard to isolate the formatting/style from the HTML, but....

  1. The alignment of the input controls required left: clear to get each one on its line
  2. I had to specify the width of the labels
  3. similarly, h1 and hr both needed left:clear

HTML:

I worry that I had to add the script section to the end to 'initialize' the output. Note that this works well on my server, but, on stack snippets, you have to change an input value to get the output updated.

Are there other problems with the HTML, it seems simple enough....

JavaScript:

This is where I imagine there is much to say, I am sure there are bad practices. Things that frustrate me especially are:

  1. I was not able to get for (player in fizzPlayers) ... to work, the player would always be undefined. I was forced to do things like for (var i = 0; var ...) and then reference back in to the fizzPlayers array.
  2. I use globals, but limited them to just a few. I missed something there, I know, there must be a way to declare the document lookups just once, without resorting to globals.

In addition to those areas where I struggled, and where I am pretty sure there must be better ways, are there other items I got wrong?

Are there any I got right?

\$\endgroup\$
  • 7
    \$\begingroup\$ Interesting fact that never occurred to me before. Using the values 2,3,5,7,11 results in a kind of crude prime number sieve. \$\endgroup\$ – RubberDuck Dec 22 '14 at 19:56
29
\$\begingroup\$

Loading

This is unconventional:

var fizzLoaded;

function fizzLoad() {
  if (fizzLoaded) {
    return;
  }
  fizzLoaded = true;
  …
}

There isn't any harm in removing the "lock". However, a more elegant way would be to write

window.onload = function fizzLoad() {
    …
};

You can use the onload handler to populate the output immediately using the default settings.

active, actfact, actname

I don't recommend maintaining three parallel arrays like this:

var val = parseInt(player.value);
if (val != 0) {
  active.push(player);
  actfact.push(parseInt(player.value));
  actname.push(capitaliseFirstLetter(player.id));
}

It would be easier to understand with one array of objects:

var val = parseInt(player.value);
if (val != 0) {
  active.push({ name: capitaliseFirstLetter(player.id),
                factor: val });
}

Handling five players

Rather than fetching the five elements by ID:

  fizzPlayers = [
    document.getElementById('frodo'),
    document.getElementById('sam'),
    document.getElementById('merry'),
    document.getElementById('pippin'),
    document.getElementById('bilbo')
  ];

It would be better to treat them collectively, so that you don't have to hard-code the five IDs:

fizzPlayers = document.getElementById('players').getElementsByTagName('input');

Furthermore, I don't recommend munging the IDs to obtain the name. One problem is that a future maintainer grepping the code for names will be surprised to find that the names are lowercase in the code, but uppercase in the output. Another problem is that identifiers should be identifiers — they are subject to rules.

Event handling and input validation

I don't recommend using oninput, since it fires too easily. A user could be trying to fix an error, and be interrupted again by an alert in the process. That would be especially frustrating since alerts are modal dialogs. An onchange handler on each input element would be more appropriate.

Layout

Floats are a bit of a pain: once you have any float in the page, you have to ensure that everything surrounding it needs to avoid being reflowed inappropriately. Use them sparingly.

You certainly don't need to float the <label> elements.

label {
    display: inline-block;
    width: 150px;
    text-align: left;
}
input {
    display: inline-block;
    float: right;
    text-align: right;
    padding-left: 10px;
    width: 50px;
}
hr, input {
    clear: right;
}

Consider abandoning floats in favour of fixed-width labels, as I've done below. Use of a <table> would also be reasonable.

var fizzFrom, fizzTo, fizzPlayers, fizzDiv;

window.onload = function fizzLoad(event) {
  // Associate inputs with their labels (https://stackoverflow.com/a/285608)
  var labels = document.getElementsByTagName('label');
  for (var i = 0; i < labels.length; i++) {
    if (labels[i].htmlFor != '') {
      var elem = document.getElementById(labels[i].htmlFor);
      if (elem) elem.dataset.labelHTML = labels[i].innerHTML;
    }
  }
  
  fizzFrom = document.getElementById('rangeFrom');
  fizzTo   = document.getElementById('rangeTo');
  fizzFrom.onchange = fizzTo.onchange = fizzChanged;
  
  fizzPlayers = document.getElementById('players').getElementsByTagName('input');
  for (var e = 0; e < fizzPlayers.length; e++) {
    fizzPlayers[e].onchange = fizzChanged;
  }
  
  fizzDiv = document.getElementById('fizzOut');
  
  fizzChanged();
};

function validate() {
  // Omitted for brevity
  return true;
}

function fizzChanged(event) {
  if (!validate()) return;

  var table = "";
  var rFrom = parseInt(fizzFrom.value);
  var rTo = parseInt(fizzTo.value);
  var active = [];

  for (var i = 0; i < fizzPlayers.length; i++) {
    var player = fizzPlayers[i];
    var val = parseInt(player.value);
    if (val != 0) {
      active.push({ name: player.dataset.labelHTML, factor: val });
    }
  }

  table += "<table>\n";
  table += "  <tr><th>Value</th><th>Message</th></tr>\n";
  for (var i = rFrom; i <= rTo; i++) {

    var msg = "";
    for (var p = 0; p < active.length; p++) {
      if (i % active[p].factor == 0) {
        msg += active[p].name;
      }
    }
    if (msg == "") {
      msg = i;
    }
    table += "  <tr><td>" + i + "</td><td>" + msg + "</td></tr>\n";
  }
  table += "</table>\n";

  fizzDiv.innerHTML = table;
}
label {
    display: inline-block;
    width: 100px;
}
input {
    padding-left: 10px;
    width: 50px;
}
<fieldset>
  <div><label for="rangeFrom">Range From</label>
       <input id="rangeFrom" type="number" min="1" max="100" value="1" required>
  </div>
  <div><label for="rangeTo">Range To</label>
        <input id="rangeTo"  type="number" min="1" max="1024" value="100" required>
  </div>
  <div id="players">
    <div><label for="frodo">Frodo</label>
         <input id="frodo" type="number" min="0" max="100" value="3" required>
    </div>
    <div><label for="sam">Sam</label>
         <input id="sam" type="number" min="0" max="100" value="5" required>
    </div>
    <div><label for="merry">Merry</label>
         <input id="merry" type="number" min="0" max="100" value="0" required>
    </div>
    <div><label for="pippin">Pippin</label>
         <input id="pippin" type="number" min="0" max="100" value="0" required>
    </div>
    <div><label for="bilbo">Bilbo</label>
         <input id="bilbo" type="number" min="0" max="100" value="0" required>
    </div>
  </div>
</fieldset>
<hr>
<h1>Output</h1>
<div id="fizzOut"></div>

\$\endgroup\$
  • \$\begingroup\$ Isn't DOMContentLoaded the proper event? \$\endgroup\$ – CodesInChaos Jul 4 '15 at 18:54
  • \$\begingroup\$ @CodesInChaos Yes, that would be the "modern" way of doing it. \$\endgroup\$ – 200_success Jul 4 '15 at 18:55
16
\$\begingroup\$

Your CSS seems very good, although do you really need both hl and hr if they both do the same thing? Why don't you just get rid of one of them?

You have one error in your HTML - an unclosed div (http://validator.w3.org/check):

  <div id="fizzOut" >Change a value to get output (a snippet thing)
  <script>fizzing();</script>
</body>

That div should be closed:

<div id="fizzOut">Change a value to get output (a snippet thing)</div>
\$\endgroup\$
15
\$\begingroup\$

Names

I find that in your naming, while it appears consistent, the constant use of the fizz prefix is a bit odd, since when you run it you use other words than Fizz or Buzz.

var fizzDiv, fizzFrom, fizzTo, fizzPlayers;

Capitalize first letter

This seems like doubling the amount of work you are doing:

  fizzLoaded = true;
  var fizzForm = document.getElementById('fizzbuzz');
  fizzFrom = document.getElementById('rangeFrom');
  fizzTo = document.getElementById('rangeTo');
  fizzPlayers = [
    document.getElementById('frodo'),
    document.getElementById('sam'),
    document.getElementById('merry'),
    document.getElementById('pippin'),
    document.getElementById('bilbo')
  ];
  fizzDiv = document.getElementById('fizzOut');
}

And this:

function capitaliseFirstLetter(string) {
      return string.charAt(0).toUpperCase() + string.slice(1);
}

Perhaps declaring them with capitalization would be more ideal? I'm sure there is a reason you did this but it doesn't appear to be the most elegant to me.

One more thing

If you use larger number than 100 and you start seeing values with 3-4 matches, it would be a nice touch to have a space in between. Example:

330 FrodoSamPippin

Would read better:

330 Frodo Sam Pippin

\$\endgroup\$
  • 1
    \$\begingroup\$ By convention, attributes in HTML use camelCase while elements use PascalCase. \$\endgroup\$ – RubberDuck Dec 22 '14 at 20:02
13
\$\begingroup\$

The idiomatic modern JavaScript way to write a loop over elements of an array is forEach. The for...in idiom is designed for Object keys, not Array elements, and it won't work as you might expect. When using for...in with arrays, it's not the elements that get filled in the loop variable, but their indexes.

For example you can rewrite the loop in fizzing like this:

fizzPlayers.forEach(function(player) {
  var val = parseInt(player.value);
  if (val != 0) {
    active.push(player);
    actfact.push(parseInt(player.value));
    actname.push(capitaliseFirstLetter(player.id));
  }
});

Rewriting in validateValues is a bit more tricky, because you need to return from the loop. In that case it's easier to use for..in and iterate over the indexes instead of the elements:

  for (var i in fizzPlayers) {
    var player = fizzPlayers[i];
    var val = parseInt(player.value);
    if (val < 0 || val > 100) {
      alert("Illegal value " + val + " for player " + player.id);
      return false;
    }
  }

For more info, see the docs, and this related post on SO


A common approach to avoid polluting the global namespace is to put variables into a single global object, often named App, for example:

var App = window.App = {};

Then instead of referencing variables in the global namespace, you could reference them in App:

App.fizzLoaded = false;

So that replaces many globals with a single one, which is still not perfect but better.


The variable names are often not great:

  • fizzForm and fizzFrom only differ in a single letter and easy to mix up
  • Instead of rTo, rFrom, why not simply to and from?

{
var fizzLoaded = false;
var fizzDiv, fizzFrom, fizzTo, fizzPlayers;

function fizzLoad() {
  if (fizzLoaded) {
    return;
  }
  fizzLoaded = true;
  var fizzForm = document.getElementById('fizzbuzz');
  fizzFrom = document.getElementById('rangeFrom');
  fizzTo = document.getElementById('rangeTo');
  fizzPlayers = [
    document.getElementById('frodo'),
    document.getElementById('sam'),
    document.getElementById('merry'),
    document.getElementById('pippin'),
    document.getElementById('bilbo')
  ];
  fizzDiv = document.getElementById('fizzOut');
}

function restrictRange() {
  var rFrom = parseInt(fizzFrom.value);
  var rTo = parseInt(fizzTo.value);
  fizzTo.min = rFrom;
  fizzFrom.max = rTo;
}

function validateValues() {
  var rFrom = parseInt(fizzFrom.value);
  var rTo = parseInt(fizzTo.value);
  if (rTo < rFrom) {
    alert("Illegal range from " + rFrom + " to " + rTo);
    return false;
  }

  for (var i in fizzPlayers) {
    var player = fizzPlayers[i];
    var val = parseInt(player.value);
    if (val < 0 || val > 100) {
      alert("Illegal value " + val + " for player " + player.id);
      return false;
    }
  }

  return true;
}

function capitaliseFirstLetter(string) {
      return string.charAt(0).toUpperCase() + string.slice(1);
}

function fizzing() {
  fizzLoad();
  restrictRange();
  if (!validateValues()) {
    fizzDiv.innerHTML = "Illegal inputs";
    return;
  }

  var table = "";
  var rFrom = parseInt(fizzFrom.value);
  var rTo = parseInt(fizzTo.value);
  var active = [];
  var actfact = [];
  var actname = [];

  fizzPlayers.forEach(function(player) {
    var val = parseInt(player.value);
    if (val != 0) {
      active.push(player);
      actfact.push(parseInt(player.value));
      actname.push(capitaliseFirstLetter(player.id));
    }
  });

  table += "<table>\n";
  table += "  <tr><th>Value</th><th>Message</th></tr>\n";
  for (var i = rFrom; i <= rTo; i++) {

    var msg = "";
    for (var p = 0; p < active.length; p++) {
      if (i % actfact[p] == 0) {
        msg += actname[p];
      }
    }
    if (msg == "") {
      msg = "" + i;
    }
    table += "  <tr><td>" + i + "</td><td>" + msg + "</td></tr>\n";
  }
  table += "</table>\n";

  fizzDiv.innerHTML = table;
}
}
h1 {
    clear: left;
}

hr {
    clear: left;
}

label {
    display: inline-block;
    float: left;
    clear: left;
    width: 150px;
    text-align: left;
}
input {
  display: inline-block;
  float: right;
  text-align: right;
  padding-left:10px;
  width: 50px;
}
<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="utf-8">
    <title>FizzBuzz</title>
    <link rel="stylesheet" href="fizzbuzz.css">
    <script src="fizzbuzz.js" type="text/javascript"></script>
  </head>
  <body>
    <h1>Config</h1>
    <form id="fizzbuzz">
      <fieldset id="fizzControl" oninput="fizzing();">
        <label>Range From<input id="rangeFrom" type="number" min="1" max="100" value="1" required></label>
        <label>Range To<input id="rangeTo"   type="number" min="1" max="1024" value="100" required></label>
        <div id="players" >
          <label>Frodo<input id="frodo" type="number" min="0" max="100" value="3" required></label>
          <label>Sam<input id="sam" type="number" min="0" max="100" value="5" required></label>
          <label>Merry<input id="merry" type="number" min="0" max="100" value="0" required></label>
          <label>Pippin<input id="pippin" type="number" min="0" max="100" value="0" required></label>
          <label>Bilbo<input id="bilbo" type="number" min="0" max="100" value="0" required></label>
        </div>
      </fieldset>
    </form>
    <hr>
    <h1>Output</h1>
    <div id="fizzOut" >Change a value to get output (a snippet thing)</div>
    <script>fizzing();</script>
  </body>
</html>

\$\endgroup\$
12
\$\begingroup\$

According to JSLint, you should use fourspace indentation, not two-space indentation (http://jslint.com/). I don't know the official standard on this, though.

var fizzLoaded = false;
var fizzDiv, fizzFrom, fizzTo, fizzPlayers;

function fizzLoad() {
    if (fizzLoaded) {
        return;
    }
    fizzLoaded = true;
    var fizzForm = document.getElementById('fizzbuzz');
    fizzFrom = document.getElementById('rangeFrom');
    fizzTo = document.getElementById('rangeTo');
    fizzPlayers = [
        document.getElementById('frodo'),
        document.getElementById('sam'),
        document.getElementById('merry'),
        document.getElementById('pippin'),
        document.getElementById('bilbo')
    ];
    fizzDiv = document.getElementById('fizzOut');
}

function restrictRange() {
    var rFrom = parseInt(fizzFrom.value);
    var rTo = parseInt(fizzTo.value);
    fizzTo.min = rFrom;
    fizzFrom.max = rTo;
}

function validateValues() {
    var rFrom = parseInt(fizzFrom.value);
    var rTo = parseInt(fizzTo.value);
    if (rTo < rFrom) {
        alert("Illegal range from " + rFrom + " to " + rTo);
        return false;
    }
    for (var i = 0; i < fizzPlayers.length; i++) {
        var val = parseInt(fizzPlayers[i].value);
        if (val < 0 || val > 100) {
            alert("Illegal value " + val + " for player " + fizzPlayers[i].id);
            return false;
        }
    }
    return true;
}

function capitaliseFirstLetter(string) {
            return string.charAt(0).toUpperCase() + string.slice(1);
}

function fizzing() {
    fizzLoad();
    restrictRange();
    if (!validateValues()) {
        fizzDiv.innerHTML = "Illegal inputs";
        return;
    }

    var table = "";
    var rFrom = parseInt(fizzFrom.value);
    var rTo = parseInt(fizzTo.value);
    var active = [];
    var actfact = [];
    var actname = [];
    var player;

    for (var i = 0; i < fizzPlayers.length; i++) {
        player = fizzPlayers[i];
        var val = parseInt(player.value);
        if (val != 0) {
            active.push(player);
            actfact.push(parseInt(player.value));
            actname.push(capitaliseFirstLetter(player.id));
        }
    }

    table += "<table>\n";
    table += "    <tr><th>Value</th><th>Message</th></tr>\n";
    for (var i = rFrom; i <= rTo; i++) {

        var msg = "";
        for (var p = 0; p < active.length; p++) {
            if (i % actfact[p] == 0) {
                msg += actname[p];
            }
        }
        if (msg == "") {
            msg = "" + i;
        }
        table += "    <tr><td>" + i + "</td><td>" + msg + "</td></tr>\n";
    }
    table += "</table>\n";

    fizzDiv.innerHTML = table;
}
\$\endgroup\$
  • \$\begingroup\$ The Stack Snippet editor does two spaces. Perhaps you should file a bug report on Meta. \$\endgroup\$ – 200_success Dec 23 '14 at 6:43
9
\$\begingroup\$

Inline-block, float and clear?!

Most display properties are ignored when you float an element (inline-block, table-cell, etc.). Either float it or change it to inline-block, not both. However, what you're really looking for here is this:

label {
    display: table;
}

Now you can remove the float and clear properties.

Redundancy

Left is the default alignment, you don't need to specify it on your label (and I don't see anywhere where this is overriding a previous alignment declaration).

Width in px for text elements

Generally speaking, not safe to specify widths of text elements using px, since the user is the one who has the final say in the font size that is used. Ideally, you should be using ems. This goes for both your label and your input elements.

Fieldsets

You're using a fieldset, but never using legend? Seems like a strange choice. The fieldset element is meant to group similar controls together, not just contain everything. If it were me, there would have been 2 fieldsets:

<fieldset id="fizzControl">
    <legend>Iterations</legend>

    <label>Range From<input id="rangeFrom" type="number" min="1" max="100" value="1" required></label>
    <label>Range To<input id="rangeTo"   type="number" min="1" max="1024" value="100" required></label>
</fieldset>

<fieldset id="players" >
    <legend>Players</legend>

    <label>Frodo<input id="frodo" type="number" min="0" max="100" value="3" required></label>
    <label>Sam<input id="sam" type="number" min="0" max="100" value="5" required></label>
    <label>Merry<input id="merry" type="number" min="0" max="100" value="0" required></label>
    <label>Pippin<input id="pippin" type="number" min="0" max="100" value="0" required></label>
    <label>Bilbo<input id="bilbo" type="number" min="0" max="100" value="0" required></label>
</fieldset>
\$\endgroup\$
9
\$\begingroup\$
for (var i = 0; i < fizzPlayers.length; i++) {
    player = fizzPlayers[i];
    var val = parseInt(player.value);
    if (val != 0) {
        active.push(player);
        actfact.push(parseInt(player.value));
        actname.push(capitaliseFirstLetter(player.id));
    }
}

I would change this a little bit

First, I would use the val variable in your call to actfact.push() method, no point in calling parseInt twice

for (var i = 0; i < fizzPlayers.length; i++) {
  player = fizzPlayers[i];
  var val = parseInt(player.value);
  if (val != 0) {
    active.push(player);
    actfact.push(val);
    actname.push(capitaliseFirstLetter(player.id));
  }
}

But I would then suggest getting rid of that variable altogether like this:

for (var i = 0; i < fizzPlayers.length; i++) {
  player = fizzPlayers[i];
  if (player.value == 0) continue;
  active.push(player);
  actfact.push(parseInt(player.value));
  actname.push(capitaliseFirstLetter(player.id));
}

This continues to the next iteration only creating one variable, and moving in a linear direction.

\$\endgroup\$

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.