fizzbu| – Fizzbuzz with dynamic height

By Posted on

Problem

Here is a variation on a theme: fizzbuzz. After providing this answer recently, I decided to practice some VueJS skills with outputting the values and conditionally applying styles based on the value. The specific styles are as follows:

  • red border for Fizz
  • blue border for Buzz
  • purple border for FizzBuzz

And then I decided to allow the user to change the height of the container, in case he/she wanted a smaller viewport.

What, if anything, would you change?

const getIndex = (number, index) => index + 1;
const app = new Vue({
  el: "#app",
  data: {
    numbers: new Array(100).fill(1).map(getIndex),
    height: 3000
  },
  filters: {
    getOutput: function(number) {
      if (number % 3 === 0 && number % 5 === 0) return 'FizzBuzz';
      if (number % 3 === 0) return 'Fizz';
      if (number % 5 === 0) return 'Buzz';
      return number;
    }
  },
  methods: {
    getClass: function(number) {
      const output = this.$options.filters.getOutput(number);
      if (isNaN(parseInt(output, 10))) {
      	return output;
      }
      return '';
    }
  }
});
body {
  padding: 4px;
  font-family: serif;
}
h1 {
   font: 400 20px cursive;
}
input[type="range"] {
  width: 100%;
}
#listContainer {
  overflow-y: hidden;
}

li {
  background: #fff;
  color: #000000;
  border-radius: 4px;
  border: 2px solid #6a737c;
  padding: 3px;
}

li.FizzBuzz {
  border-color: #f800ff; 
}

li.Fizz {
  border-color: #f80000;
}

li.Buzz {
  border-color: #0000ff;
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/vue/2.5.17/vue.js"></script>
<div id="app">
  <h1>
    FizzBuzz with dynamic height container
  </h1>
  <div>
   Height: {{ height }}
  </div>
  <input type="range" min="200" max="3000" v-model="height" />
  <div id="listContainer" v-bind:style="{ height: height + 'px'}">
    <ul>
      <li v-for="number in numbers" :class="getClass(number)">
        {{ number | getOutput }}
      </li>
    </ul>
  </div>  
</div>

Expand snippet

Solution

Found a little inconsistency in your Vue template

Using v-bind: syntax here,

<div id="listContainer" v-bind:style="{ height: height + 'px'}">

… but using : shorthand here:

<li v-for="number in numbers" :class="getClass(number)">

Useless variable app

const app = new Vue({ /* ... */ });

Do I need to say more?

EDIT: YES.
From ESLint: “Variables that are declared and not used anywhere in the code are most likely an error due to incomplete refactoring. Such variables take up space in the code and can lead to confusion by readers.”

Object method notation shorthand

Instead of using:

getOutput: function(number) { /* ... */ }
// ...
getClass: function(number) { /* ... */ }

You could use:

getOutput(number) { /* ... */ }
// ...
getClass(number) { /* ... */ }

Too specific CSS selector

You’re specifying these selectors,

li.FizzBuzz { /* ... */ }
li.Fizz { /* ... */ }
li.Buzz { /* ... */ }

but there aren’t any elements that would have these classes (FizzBuzz, Fizz and Buzz) other than li‘s. This means you could simplify it to:

.FizzBuzz { /* ... */ }
.Fizz { /* ... */ }
.Buzz { /* ... */ }

As the fizz has been done to death I will review this in terms of a page that displays a list of items, could be anything, dates, measurements, or whatnot.

Problems and bugs

There is a sizing problem.

Not all the values indicated by the height slider can be scrolled to. This is because you set the container size incorrectly <div id="listContainer" v-bind:style="{ height: height + 'px'}"> it should be height: height * listItemHeight + 'px' with listItemHeight matching the height of a list item.

Better yet don’t set the height let the layout engine do that. You use colon property :class="getClass(number)" You can add another class named Empty and return that if the function is called with a number greater than height.

The containing div will size itself to fit the content.

You only display 100 items

Changing the height slider (min value is 200) I imagine changes the number of items in the list. However only 100 items are displayed no matter what the height value is.

The initial setting is incorrect

When the page loads you set the slider height to 3000 but the array you set to 100. Maybe a constant in the JS to set up the height, and numbers array would help. (See first example)

Use a label

Use a label to associate the height slider with the height value display rather than an unassociated div. You can just nest the input within the label to make the association implicit.

JavaScript style

  • Delimit single line statement blocks with curlies if (foo) {...}
  • isNaN does not require a number, the explicit convertion is supluflorouse. isNaN(parseInt(output, 10)) is the same a isNaN(output) Also use Number to convert base 10 numbers if you know you are not rounding Number(output) is better than parseInt(output, 10)
  • Be consistency in style. In the function getOutput you use inline undelimited single
    line statements if (number % 5) return 'Buzz'; yet in getClass you use 3 line delimited single line statements if (isNaN(output)) {n return output;n } with n for new lines.
  • Less is best. Use the short form of code where you can. Eg last 4 lines of getClass can be on ternary. (see example)

Use dynamic views for large data sets.

I think that the approach is a little over the top. Having very long pages has a cost in resources. Considering that you can never see more than a screen full at a time it would be more effective to generate the list as a view in place as the user scrolls. That way you only need to create as many elements as can be seen at once. You could use the height slider to replace the scroll and us it to reference the top view item.

With a little more effort such a list should be zoomable as well, only ever displaying one screenfull. (Really scroll bars are old, ugly, and awkward. Good design means weighted intuitive gesture control, even with a mouse)

Example 1

Addressing some of the problems and bugs. Uses a complete list (set to 1000 for practicality)

Show code snippet

Example 2

This does not create the long list, rather it uses a view controlled by the height slider replacing the scroll bar. This lets you display a much larger range of values without having to tax the device with a huge lists of elements.

I have removed the title (not needed we know what the page does) and height label as that value is now the first fizzBuzz item. This gives needed realestate back to the app.

Increased the view range to 10000. Also using HEX CSS alpha format colors thus will look a little ugly for some older browsers.

Show code snippet

Have not tried Vue.js.

number within getIndex is not used, the first parameter can be replaced with an underscore 

const getIndex = (_, index) => index + 1;


Array.from({length: 100}, getIndex)

can be substituted for 

new Array(100).fill(1).map(getIndex)

to reduce Array method calls.

getOutput() function body can be reduced to two lines with one return statement by using destructuring assignment, AND && operator for variables Fizz and Buzz, which FizzBuzz is derived from, and OR || operator

function getOutput(number) {
  const [f, b, fb = f && b && 'FizzBuzz'] = [number % 3 && 'Fizz', number % 5 && 'Buzz'];
  return fb || f || b || number;
}

FizzBuzz, Fizz and Buzz variable names can be substituted for single character variable names fb, f and b variable names within getOutput function body if necessary.

A single return statement can be substituted for two return statements within getClass function by using conditional operator condition ? expression0 : expression1.

function getClass(number) {
  const output = this.$options.filters.getOutput(number);
  return isNaN(parseInt(output, 10)) ? output : '';
}

Leave a Reply

Your email address will not be published. Required fields are marked *