4
\$\begingroup\$

I discovered the FizzBuzz test while looking up something else on stackoverflow, so I decided to try it and see what I would come up with. I'm kind of new to jquery and JavaScript, so I'm looking for a critique on my code. What can I do better? Changes? Simplifications?

My priorities on this were -

  1. Maintainability & readability
  2. A page that was responsive to browser resizing & different screen sizes
  3. Presenting it in a nice format without overdoing it. I came up with a simple, 4 column static version in about 45 minutes, but then refactored and styled it quite a bit.

Two additional notes:

  1. I used the defer attribute in my script tag and put it in the head.

    <script src="fizzbuzz.js" charset="utf-8" defer="defer"></script>
    

    Is that acceptable? Is there a better way?

  2. Setting intervals/timeouts still confounds me. How would you create a small interval between each number line printing, such that they print out sequentially and in an animated manner. I think you would need setInterval() or setTimeout(), but I'm not really sure how to use it without breaking everything.

Here's a copy of the code, edited per st88's suggestion. The old calculateColumns() function is commented out, and has been replaced by two functions (i.e. createColumns and calculateColumns) that deal with the conditional differently and remove a repeating code error.

var fizzContainer = $("div.number-container");

var divisor1 = 3,
  divisor2 = 5,
  maxFizz = 100,
  word1 = "Fizz",
  word2 = "Buzz",
  windowWidth = window.innerWidth,
  minNumOfColumns = 1,
  maxNumOfColumns = 12,
  widthPerColumn = 150,
  columns,
  linesPerColumn,
  columnSizingParameters = {};

calculateHeading(windowWidth);
calculateColumns();
createColumns();

$(window).resize(function() {
  windowWidth = window.innerWidth;
  calculateHeading(windowWidth);

  $(".number-container").replaceWith("<div class='number-container'></div>");
  createColumns();
});

function calculateHeading(windowSize) {
  if (windowSize <= 500) {
    $("h1").attr("class", "transition xSmallHeading");
  } else if (windowSize <= 700) {
    $("h1").attr("class", "transition smallHeading");
  } else if (windowSize <= 1000) {
    $("h1").attr("class", "transition mediumHeading");
  } else if (windowSize >= 1000) {
    $("h1").attr("class", "transition largeHeading");
  }
}


// function calculateColumns() {
//    for (var i = 1; i <= maxNumOfColumns; i++) {
//       if ((windowWidth >= columnSizingParameters[i]) && (windowWidth <= columnSizingParameters[i + 1])) {
//            columns = i;
//            break;
//       }
//    }
//    linesPerColumn = Math.ceil(maxFizz / columns);
//    createNumbers();
// }

function calculateColumns() {
  var columnIt = minNumOfColumns - 1;
  for (columnIt; columnIt <= maxNumOfColumns; columnIt++) {
    columnSizingParameters[columnIt] = columnIt * widthPerColumn;
  }
}

function createColumns() {
  for (var i = 1; i <= maxNumOfColumns; ++i) {
    if (columnSizingParameters[i] > windowWidth) {
      columns = i - 1;
      break;
    }
  }
  linesPerColumn = Math.ceil(maxFizz / columns);
  createNumbers();
}

function createNumbers() {
  var i = 1;

  for (var c = 1; c <= columns; c++) {
    var col = $('<div></div').appendTo(".number-container");
    col.addClass('column');

    for (var a = 1; a <= linesPerColumn && i <= maxFizz; a++, i++) {
      if ((i % divisor1 === 0) && (i % divisor2 === 0)) {
        createContentLine(word1 + word2, "double-match", col, i, true);
        continue;
      } else if (i % divisor1 === 0) {
        createContentLine(word1, "match-1", col, i, true);
        continue;
      } else if (i % divisor2 === 0) {
        createContentLine(word2, "match-2", col, i, true);
        continue;
      } else {
        createContentLine(i, "num", col, i, false);
        continue;
      }
    }
  }
}

function createContentLine(toPrint, cssClass, col, i, appendNumToWord) {
  var contentLine = $("<p>" + toPrint + "</p>").appendTo(col);
  contentLine.addClass(cssClass);

  if (appendNumToWord) {
    var appendedNumber = $("<span></span").appendTo(contentLine);
    appendedNumber.text(" • " + i).addClass("num");
  }
}
* {
  padding: 0px;
  margin: 0px;
  box-sizing: border-box;
}
.container {
  width: auto;
  height: 100%;
  display: block;
}
h1 {
  display: block;
  background-color: black;
  color: white;
  width: 100%;
  font-family: 'Berkshire Swash', cursive;
}
.transition {
  transition: 500ms;
}
/* Screen width Less than 500px */

.xSmallHeading {
  font-size: 25px;
  line-height: 25px;
  padding: 15px;
}
/* Less than 700px */

.smallHeading {
  font-size: 32px;
  line-height: 32px;
  padding: 20px;
}
/* Less than 1000px */

.mediumHeading {
  font-size: 40px;
  line-height: 40px;
  padding: 30px;
  margin-bottom: 10px;
}
/* Greater than 1000px */

.largeHeading {
  font-size: 50px;
  line-height: 50px;
  padding: 40px;
  margin-bottom: 20px;
}
.number-container {
  display: flex;
  margin-top: 20px;
  margin-bottom: 20px;
  text-align: center;
  line-height: 30px;
  width: 100%;
  height: calc(100% - 150px);
}
.column {
  flex-grow: 1;
  display: inline-block;
  border-right: 1px solid lightblue;
}
.column:last-of-type {
  border-right: none;
}
.num {
  color: #585858;
  font-weight: bold;
  font-style: normal;
  font-family: sans-serif;
  font-size: 12px;
}
.double-match {
  font-weight: 600;
  font-size: 20px;
  color: #3f89d6;
  font-family: 'Crimson Text', serif;
}
.match-1,
.match-2 {
  font-family: 'Josefin Slab', serif;
  font-weight: 700;
  font-size: 18px;
}
.match-1 {
  color: #7ac962;
}
.match-2 {
  color: #ba62d4
}
<html>

<head>
  <meta charset="utf-8">
  <title>FizzBuzz in Javascript</title>

  <link href='https://fonts.googleapis.com/css?family=Berkshire+Swash|Crimson+Text:600|Josefin+Slab:700' rel='stylesheet' type='text/css'>

  <link rel="stylesheet" href="style.css" media="screen" charset="utf-8">

  <script src="https://code.jquery.com/jquery-2.2.3.min.js" integrity="sha256-a23g1Nt4dtEYOj7bR+vTu7+T8VP13humZFBJNIYoEJo=" crossorigin="anonymous"></script>

  <script src="fizzbuzz.js" charset="utf-8" defer="defer"></script>
</head>

<body>
  <div class="container">

    <h1>The fizzbuzz test...in Javascript!</h1>

    <div class="number-container">

    </div>
  </div>
</body>

</html>

I also put it on CodePen, for those so inclined.

\$\endgroup\$
  • \$\begingroup\$ Wouldn't it be enough to check ... " if (columnSizingParameters[i] > windowWidth) { columns = --i; } " ? On the first column which is bigger then allowed you stop and take the one before. Because that one was still alright. \$\endgroup\$ – michael.zech May 2 '16 at 6:47
  • \$\begingroup\$ Yup your right. I changed it to that, but with a break at the end so I don't always end up with 12 columns. I also separated the calculateColumns() function in to two, calculateColumns(), which defines the columnSizingParameters[], and createColumns(), which does the creation. Looking at it again made me realize that there was no need to redefine columnSizingParameters every time the page is resized. \$\endgroup\$ – Alessia May 3 '16 at 20:14
1
\$\begingroup\$

I know that this was posted more than 2.5 years ago and you likely have learned a fair amount about JavaScript and/or jQuery in that time. Still this question hasn't had any answers (at the time of writing) so I would like to take out this zombie.

Your Questions

I used the defer attribute in my script tag and put it in the head. Is that acceptable? Is there a better way?

I believe that is acceptable, however there are other ways to have the code executed after certain events have occurred. For example, one could use event.addEventListener() to trigger a function call after events occur, like DOMContentLoaded.

As walter memtioned in this SO answer "-The Yahoo! Exceptional Performance team recommend placing scripts at the bottom of your page because of the way browsers download components._" so one can consider putting the <script> tag in the <body> tag instead of the <head>.

Setting intervals/timeouts still confounds me. How would you create a small interval between each number line printing, such that they print out sequentially and in an animated manner.

Yes, using setTimeout() would be a good way to achieve such a feature. a function that calls that function, passing a function to execute after a set amount of time could call the outer function until all elements have been printed, or similarly with setInterval() and clearInterval().

Review feedback

Overall I like the layout of the view. The code is organized well for the most part. There are a few recommendations I have, listed below.


The collection of <div> elements with class name number-container is stored in fizzContainer at the start of the script:

var fizzContainer = $("div.number-container");

This is great, because whenever that collection is needed later, it can be utilize by that variable name. Sadly, I don't see that name utilized anywhere after it is assigned, thus the assignment seems superfluous... You could utilize that to replace the line below, in the resize callback:

$(".number-container").replaceWith("<div class='number-container'></div>");

With a call to .html():

fizzContainer.html('');

Assigning DOM collections to variables in the beginning could minimize DOM lookups for other collections, e.g. $('h1'), $(".number-container").


In that resize function I see the following:

$(window).resize(function(){
  windowWidth = window.innerWidth;
  calculateHeading(windowWidth);

It appears that calculateHeading() accepts the window width as a parameter, whereas all other functions just utilize the global variables. Why not use one tactic or the other?

I also noticed that in the codepen code three spaces are used for indentation in that resize callback function, while the code posted above uses two spaces.


If performance was an issue, I would recommend adding the columns after the contents have been added, and potentially adding the columns after all have been generated, so as to minimize reflows.

\$\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.