14
\$\begingroup\$

I decided that since I was learning AngularJS and Bootstrap, I'd be remiss not to post a FizzBuzz, but FizzBuzz bores me, so I've extended it.

My version of FizzBuzz is configurable, and allows you to create an infinite number of different "Fizz", "Buzz" etc elements with whatever names you like.

function element(name, frequency) {
  this.name = name;
  this.frequency = frequency;
};

function fizzBuzz(index, value) {
  this.index = index;
  this.value = value || index.toString();
};

var app = angular.module("fizzBuzzApp", []);

app.controller("fizzBuzzController", function($scope) {
  $scope.model = {
    elements: [
      new element("Fizz", 3),
      new element("Buzz", 5),
      new element("Lizard", 7),
      new element("Spock", 11),
    ],
    fizzBuzz: []
  };
  $scope.controller = {
    elementCount: 4,
    fizzBuzzCount: 100,
    generateElements: function() {
      $scope.model.elements = [];
      $scope.model.fizzBuzz = [];
      for (var i = 0; i < $scope.controller.elementCount; i++) {
        $scope.model.elements.push(new element("Element", 0));
      }
    },
    generateFizzBuzz: function() {
      $scope.model.fizzBuzz = [];
      $scope.model.elements.sort(function(a, b) {
        return a.frequency - b.frequency
      });

      for (var i = 1; i <= $scope.controller.fizzBuzzCount; i++) {
        var result = "";
        var anyElementMatches = false;

        $scope.model.elements.forEach(function(element) {
          if (i % element.frequency == 0) {
            result += element.name;
            anyElementMatches = true;
          }
        });

        if (!anyElementMatches) {
          result = i.toString();
        }

        $scope.model.fizzBuzz.push(new fizzBuzz(i, result));
      }
    }
  };
  $scope.controller.generateFizzBuzz();
  $scope.$apply();
});
<head>
  <script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
  <script src="https://ajax.googleapis.com/ajax/libs/angularjs/1.2.23/angular.min.js"></script>
  <script src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.4/js/bootstrap.min.js"></script>
  <link href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.4/css/bootstrap.min.css" rel="stylesheet" />
</head>

<body ng-app="fizzBuzzApp" ng-controller="fizzBuzzController">
  <div class="container-fluid">
    <div class="jumbotron">
      <div class="row">
        <h2 class="col-sm-8">FizzBuzzer<br /><small>A configurable FizzBuzz</small></h2>
      </div>
    </div>
    <hr />
    <div class="panel-group" id="accordion" role="tablist" aria-multiselectable="true">
      <div class="panel panel-default">
        <div class="panel-heading" role="tab" id="configHeading">
          <h3 class="panel-title">
                        <a data-toggle="collapse" data-parent="#accordion" href="#collapseConfig" aria-expanded="true" aria-controls="collapseConfig">
                            <span class="glyphicon glyphicon-cog"></span> Configuration
                        </a>
                    </h3>
        </div>
        <div id="collapseConfig" class="panel-collapse collapse in" role="tabpanel" aria-labelledby="configHeading">
          <div class="panel-body">
            <form role="form" class="form-horizontal">
              <div class="form-group">
                <label for="inputElementCount" class="control-label col-sm-4">Elements to use</label>
                <div class="col-sm-6">
                  <div class="input-group">
                    <input type="number" class="form-control" id="inputElementCount" placeholder="How many items elements should there be?" ng-model="controller.elementCount" />
                    <span class="input-group-btn">
                                            <button class="btn btn-primary" type="button" ng-click="controller.generateElements()">
                                                <span class="glyphicon glyphicon-refresh"></span> Generate
                    </button>
                    </span>
                  </div>
                </div>
              </div>
              <hr />
              <div ng-repeat="element in model.elements">
                <div class="form-group">
                  <label for="inputElementName" class="control-label col-sm-4">Element Name</label>
                  <div class="col-sm-6">
                    <input type="text" class="form-control" id="inputElementName" placeholder="Enter name for Fizz" ng-model="element.name" />
                  </div>
                </div>
                <div class="form-group">
                  <label for="inputElementFrequency" class="control-label col-sm-4">{{element.name}} Frequency</label>
                  <div class="col-sm-6">
                    <div class="input-group">
                      <span class="input-group-addon">Every</span>
                      <input type="number" class="form-control" id="inputElementFrequency" placeholder="Enter how often {{element.name}} appears." ng-model="element.frequency" />
                      <span class="input-group-addon">numbers</span>
                    </div>
                  </div>
                </div>
                <hr />
              </div>
            </form>
          </div>
        </div>
      </div>
      <div class="panel panel-default">
        <div class="panel-heading" role="tab" id="fizzBuzzHeading">
          <h3 class="panel-title">
                        <a class="collapsed" data-toggle="collapse" data-parent="#accordion" href="#collapseFizzBuzz" aria-expanded="false" aria-controls="collapseFizzBuzz">
                            <span class="glyphicon glyphicon-play-circle"></span> FizzBuzz
                        </a>
                    </h3>
        </div>
        <div id="collapseFizzBuzz" class="panel-collapse collapse" role="tabpanel" aria-labelledby="fizzBuzzHeading">
          <div class="panel-body">
            <form role="form" class="form-horizontal">
              <div class="form-group">
                <label class="col-sm-4 control-label">Generate</label>
                <div class="col-sm-6">
                  <div class="input-group">
                    <input type="number" class="form-control" id="inputFizzBuzzCount" placeholder="Enter how many numbers to generate." ng-model="controller.fizzBuzzCount" />
                    <span class="input-group-addon">numbers</span>
                  </div>
                </div>
              </div>
              <div class="form-group">
                <button class="btn btn-primary col-sm-6 col-sm-offset-4" type="button" ng-click="controller.generateFizzBuzz()"><span class="glyphicon glyphicon-play-circle"></span> Fizz Buzz</button>
              </div>
            </form>
            <hr />
            <div class="col-sm-offset-4 col-sm-6" ng-repeat="fizzBuzz in model.fizzBuzz">
              <h4>{{fizzBuzz.index}} <small>{{fizzBuzz.value}}</small></h4>
              <hr />
            </div>
          </div>
        </div>
      </div>
    </div>
  </div>
</body>

I'm looking for a general review here, particularly focusing on best practices, since I'm fairly new to HTML and JS and completely new to AngularJS and Bootstrap.

\$\endgroup\$
6
+50
\$\begingroup\$

Very nice. I can't really see any major issues here, just some nits to pick. Note, though, that my knowledge of Angular is near-nil, so perhaps there's something there that someone else will be better equipped to comment on.

Markup

The markup is all-Bootstrap, so it's pretty by-the-book already.

There's a lot of it though, making it a hard to skim. To get a good look I copied it to my editor, and basically add more whitespace to separate things. In general, you seem to skimp on whitespace (both in markup and JavaScript) a little too much for my tastes.

You can also leave some things out, and combine others; some classes work fine on non-div elements.

Still, it'd probably help your readers (and you), to add comments by major close tags. This isn't something I usually do myself, but in this case the structure is so dense (that's Bootstrap for ya) that it makes sense.

For instance, the first panel take up around 50 lines. Doing something like

<div id="configPanel" class="panel panel-default">
  ...
</div> <!-- configPanel -->

would clarify things.

Also, I noticed you've got an id attribute on an element that gets repeated by Angular (inputElementFrequency). So, in turn, you end up with duplicate IDs. IDs are meant to be unique across the page, but it's up to you to maintain that uniqueness; it's not enforced by browsers. However, the expectation/assumption of uniqueness is very real, and duplicates can cause all manner of confusion.

Lastly, you probably shouldn't print the output as a bunch of headings. That doesn't really make sense, semantically speaking; they're not headings. A list, however, would be a good fit. Better yet, an ol list will automatically be numbered from 1 and up, so you only need to print the fizz/buzz text.

JavaScript

The element and fizzBuzz constructors should, by convention, be named with PascalCase. So it should be FizzBuzz and Element - except that Element is a native constructor. And redefining a native constructor will only lead to trouble.

You can sort of get around that by putting your constructors inside the function used to define the model and controller, like so:

app.controller("fizzBuzzController", function ($scope) {
  function Element(name, frequency) { ... }
});

Since any function creates a local scope for your variables and functions, you can have your own Element. It'll still shadow the global, built-in one, but it won't walk all over it.

But I'd simply pick another name, like Word or something.

Then again for something this simple - basically a container to two variables - you could just get away with an object literal:

{ name: "Fizz", frequency: "Buzz" }

In practice the result is the same as your current new element("Fizz", 3): You get an object with a name and a frequency property. And you can do the same for fizzBuzz of course.

Next, this is a bit iffy:

elementCount: 4

You should rather do $scope.model.elements.length, and avoid the hard-coded 4.

Then there's the main event, generateFizzBuzz. I'd consider working on a copy of $scope.model.elements; when you sort it directly, you also end up sorting the UI used to create the elements. There's no real need for that, and maybe a user would be confused by the input having been swapped around. So you could instead do:

var elements = $scope.model.elements.slice(0).sort(...);

slice(0) is a common JS idiom - basically a poor man's array copy/dup.

As for the main loop, I'd do this instead:

for (var i = 1; i <= $scope.controller.fizzBuzzCount; i++) {
  var matches = $scope.model.elements.map(function(element) {
    if(i % element.frequency === 0) {
      return element.name;
    }
  });

  $scope.model.fizzBuzz.push({ value: i, result: matches.join("") || i });
}

The mapping will return either a name or undefined. And undefined values become empty strings when joining. So if there are no matches, you'll end up with an array full of nuthin' that'll join to an empty string, and it'll fall back to i instead.

By the way, you don't need to muck around with toString; a number will end up as a string anyway. Besides, the way your fizzBuzz constructor worked, you didn't need the anyMatches check; your constructor handled that for you.

Finally, while I don't know Angular, I have a feeling that fizzBuzzCount should be a property on the model, not the controller.

Overall

This isn't about the code itself, really, but I'd probably prefer a solution where I can simply click to add or remove an "element", instead of having to regenerate a fixed number of inputs. When the list is regenerated, it also removes whatever's already been typed, so it's a hassle having to re-input everything.

You could handle the list expanding/contracting without regenerating all the elements, but.. eh'. Simply adding and removing input directly is more direct.

I've tried some stuff below. I've skipped the accordion panels, just to keep it simple, and the thing will stretch to 100% wide (didn't bother with columns).

var app = angular.module("fizzBuzzApp", []);

app.controller("fizzBuzzController", function($scope) {
  $scope.model = {
    words: [
      { word: "Fizz", frequency: 3 },
      { word: "Buzz", frequency: 5 }
    ],
    count: 100,
    result: []
  };
  
  $scope.controller = {
    addWord: function () {
      $scope.model.words.push({ word: "Word", frequency: 1 });
    },
    
    removeWord: function (index) {
      $scope.model.words.splice(index, 1);
    },
    
    generateList: function() {
      var matches, words = $scope.model.words.slice(0).sort(function(a, b) {
        return a.frequency - b.frequency
      });

      $scope.model.result = [];
      
      for(var i = 1; i <= $scope.model.count; i++) {
        matches = words.map(function (item) {
          if(i % item.frequency === 0) {
            return item.word;
          }
        });
        
        $scope.model.result.push(matches.join("") || i);
      }
    }
  };
  
  $scope.controller.generateList();
  $scope.$apply();
});
<head>
  <script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
  <script src="https://ajax.googleapis.com/ajax/libs/angularjs/1.2.23/angular.min.js"></script>
  <link href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.4/css/bootstrap.min.css" rel="stylesheet" />
</head>

<body ng-app="fizzBuzzApp" ng-controller="fizzBuzzController">
  <div class="container-fluid">
    
    <div class="panel panel-default">
      <div class="panel-body">
        <p class="input-group" ng-repeat="item in model.words">
          <span class="input-group-addon">Print</span>
          <input type="text" class="form-control" placeholder="A word to print" ng-model="item.word">
          <span class="input-group-addon">every</span>
          <input type="number" class="form-control" placeholder="How often {{item.word}} appears" ng-model="item.frequency">
          <span class="input-group-addon">numbers</span>
          <span class="input-group-btn">
            <button type="button" class="btn btn-danger" ng-click="controller.removeWord($index)">
              <span class="glyphicon glyphicon-remove"></span> Remove
            </button>
          </span>
        </p>
        
        <button type="button" class="btn btn-primary" ng-click="controller.addWord()">
          <span class="glyphicon glyphicon-plus"></span> Add
        </button>
      </div> <!-- panel-body -->
    </div> <!-- panel -->
  
    <div class="panel panel-default">
      <div class="panel-body">
        <div class="input-group">
          <span class="input-group-addon">Print</span>
          <input type="number" min="1" class="form-control" placeholder="Number of iterations" ng-model="model.count">
          <span class="input-group-addon">numbers</span>
          <span class="input-group-btn">
            <button type="button" class="btn btn-primary" ng-click="controller.generateList()">
              <span class="glyphicon glyphicon-refresh"></span> Generate
            </button>
          </span>
        </div>
      </div>
      
      <ol class="list-group">
        <li class="list-group-item" ng-repeat="text in model.result track by $index">{{text}}</li>
      </ol>
    </div> <!-- panel -->
    
  </div> <!-- container -->
</body>

\$\endgroup\$
  • \$\begingroup\$ Thank you for the thorough review! When you say I don't use whitespace much, could you post an example of what you would consider a preferred use of whitespace? \$\endgroup\$ – Nick Udell Jun 4 '15 at 8:27
  • 1
    \$\begingroup\$ @NickUdell I was thinking mostly of vertical whitespace, aka blank lines. Your HTML didn't have a single blank like in the body, which made it that bit harder to see where a "chunk" like a panel began and ended. Your JS had some inside function, but not between them. But of course there's no hard and fast rule for when to add some whitespace - some people use more, some use less. Using none is a little on a low side though :) The code in my answer has some blank lines here and there, so that's the example I'd give. \$\endgroup\$ – Flambino Jun 4 '15 at 9:31
  • 1
    \$\begingroup\$ @NickUdell No prob, glad it helped :) \$\endgroup\$ – Flambino Jun 4 '15 at 10:56
3
\$\begingroup\$

Your HTML has a few issues. First, you should have a DOCTYPE statement, the latest of which is:

<!DOCTYPE html>

Second, you should have a title element in your head. This sets the text specified on the tab title:

<title>Page Title Here</title>

The other issues appear to be issues with how you connect your HTML to the JS:

Attribute ng-app not allowed on element body at this point.

I have minimal experience doing this, so I don't have a fix, but you can test everything for yourself at the W3C HTML validator.

\$\endgroup\$
  • \$\begingroup\$ I wasn't aware I could include DOCTYPE in the snippets, so I kept it out. Thanks for pointing out the HTML validator and the page title! \$\endgroup\$ – Nick Udell Jun 4 '15 at 8:12
  • 2
    \$\begingroup\$ @NickUdell The markup will not pass validation as long as you use ng-* attributes, since they're not part of any HTML spec (it's something Angular made up). But! There is an alternative, namely using data-ng-* instead. Arbitrary data-* attributes are part of the HTML5 spec, so they should validate just fine if you're declaring the HTML5 doctype (the one in the above answer) \$\endgroup\$ – Flambino Jun 4 '15 at 15:44

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.