Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FizzBuzz-Redux__W5-A4 Hollis (final) #3

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hollislau
Copy link

Remove all DOM related content from the code and refactor into a CommonJS module pattern to publish the FizzBuzz app as an npm package.

@hollislau hollislau changed the title FizzBuzz-Redux__W5-A4 Hollis FizzBuzz-Redux__W5-A4 Hollis (final) Oct 2, 2015
var _outputArray;

var FizzBuzz = function(string1, string2) {
_stringsArray.push(string1, string2);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than simply pushing your values in to your _stringsArray, when your constructor is run you might want to also reset your _stringsArray; the current implementation actually keeps it from allowing different instances to have different output. Currently, this is what's happening:

var FizzBuzz = require("sea-d44-fizz-buzz-hl");
var buzz1 = new FizzBuzz();
buzz1.input(1,10);
console.log(buzz1.output()); // >> [1,2,"fizz",4,"buzz",...]

var buzz2 = new FizzBuzz("tick", "tock");
buzz2.input(1,100);
console.log(buzz2.output()); // >> [1,2,"fizz",4,"buzz"...]

Basically, with these private variables, you're sharing your data between the instances (which can be useful in some cases). It might make more sense to store the _stringsArray as a property of the instance: this.stringsArray instead:

var FizzBuzz = function(string1, string2) {
  this.stringsArray = [string1, string2];
};

@miketierney
Copy link

Looks pretty good; the only real issue is with the instance "pollution" for the string values. 14/15.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants