11
\$\begingroup\$

I'm new to coding and have been directed here from Stack Overflow. As I'm new to coding, I'm not entirely sure if my program works completely, but I think it does. I'm looking for suggestions on how to improve my code, particularly the format, as well as the code itself please.

Rather than output FizzBuzz for the first 100 numbers, i.e., 1 - 100, I wanted to write a program that could output FizzBuzz for any positive whole number range.

const fizzBuzz = (num) => {
  let fizzBuzzNumbers = [];

  const populateArray = num => {
    for (let i = 1; i < num + 1; i++) {
      fizzBuzzNumbers.push(i);
    }
  };

  populateArray(num)

  for (let i = 0; i < fizzBuzzNumbers.length; i++) {
    if (fizzBuzzNumbers[i] % 3 === 0 && fizzBuzzNumbers[i] % 5 === 0) {
      console.log('FizzBuzz');
    } else if (fizzBuzzNumbers[i] % 3 === 0 && fizzBuzzNumbers[i] % 5 !== 0) {
      console.log('Fizz');
    } else if (fizzBuzzNumbers[i] % 3 !== 0 && fizzBuzzNumbers[i] % 5 === 0) {
      console.log('Buzz');
    } else {
      console.log(fizzBuzzNumbers[i]);
    }
  }
}; 

Any feedback or suggestions will be greatly appreciated.

\$\endgroup\$
4
  • 6
    \$\begingroup\$ You can start with i = 1 and then drop the array completely. \$\endgroup\$ – konijn Feb 28 at 15:22
  • \$\begingroup\$ @konijn thank you very much for the suggestion. As I'm very new to coding, I'm unsure of how to implement the change. Is it possible to show me where in the code I can start with i = 1 and drop the array please? \$\endgroup\$ – Marc Sharp Feb 28 at 16:57
  • 16
    \$\begingroup\$ I'm not entirely sure if my program works completely, but I think it does. How do you come to be unsure? Have you run the program? Did it work? Have you run it for some other ranges? Have you considered what ranges might cause weird behaviours? Welcome to Software development. More than half of the work is testing and debugging the code you wrote :D \$\endgroup\$ – Brondahl Mar 1 at 0:24
  • \$\begingroup\$ @Brondahl - I think it's just a confidence thing because I've only been learning for a few months. I've tested programs I've written as part of a supported learning pathway before, but this was the first time I have tried to write a program from scratch off of a brief that was just 'Write a program that...' As for debugging, that's something I'm yet to become familiar with, so your comment has got me investigating that now. Really appreciate your comment. Thanks. \$\endgroup\$ – Marc Sharp Mar 3 at 18:14
15
\$\begingroup\$

A short review;

  • You don't need the array, you could just use i
  • Part of the challenge is to realize that you only have to check for 3 and 5
  • i is a good short variable, I would go for num -> n or even count
  • I only use fat arrow syntax => for inline functions
  • Function names ideally follow <verb>Thing syntax so fizzBuzz -> logFizzBuzz

This is my counter proposal;

function logFizzBuzz(count){

  for (let i = 1; i <= count; i++) {
    let out = '';
    if (i % 3 === 0){
      out += 'Fizz';
    }
    if (i % 5 === 0){
      out += 'Buzz';
    }
    console.log(out || i);
  }
}; 


logFizzBuzz(45);

\$\endgroup\$
3
  • 3
    \$\begingroup\$ Fantastic! Your solution is so much more elegant and has given me real motivation to revise my own. I knew mine looked clunky, but being so early in my learning, I couldn't work out how to refine it. Thank you for taking the time to review. It's greatly appreciated @konijn. \$\endgroup\$ – Marc Sharp Feb 28 at 17:10
  • \$\begingroup\$ While the array isn't necessary for the usual sequential FizzBuzz, it could be useful in a more general version. E.g. you could populate the array with random numbers. \$\endgroup\$ – Barmar Mar 1 at 16:00
  • 3
    \$\begingroup\$ One of the tricks of FizzBuzz is that there are lots of ways to do the "otherwise, print the number", and none of them are obviously better than the others. There is no reason a new programmer should be expected to write console.log(out || i); instead of something like console.log(out == "" ? i : out); or even if(out=="") console.log(i); else console.log(out); \$\endgroup\$ – user253751 Mar 1 at 17:00
10
\$\begingroup\$

There are a couple of things that are worrisome in this code. The one that caught me first was the shadowing of the num variable... You have 2 different variables called num.

In the populateArray function you should have a different name for it, not num.

This got me thinking, and the name num is a poor name choice. It should be count or similar, because it's not an actual number you are computing FizzBuzz on, but rather the count of values....

In addition, your loop for populating the array is

for (let i = 1; i < num + 1; i++) {
  fizzBuzzNumbers.push(i);
}

Loops typically are 0-based in computing, and when you need a 1-based loop like you do now, instead of having let i = 1; i < num + 1; i++ it is more common to have let i = 1; i <= num; i++ - it is probably inconsequential on performance, but having the additional + 1 in each check of the loop is messy. Note that <= x is the same as < x + 1

Finally, your if-else-if-else-if ... chain has too many comparisons. Your first comparison compares for both %5 and %3 so for your subsequent checks you don't need to check for the negative match on things.

You have:

if (fizzBuzzNumbers[i] % 3 === 0 && fizzBuzzNumbers[i] % 5 === 0) {
  console.log('FizzBuzz');
} else if (fizzBuzzNumbers[i] % 3 === 0 && fizzBuzzNumbers[i] % 5 !== 0) {
  console.log('Fizz');
} else if (fizzBuzzNumbers[i] % 3 !== 0 && fizzBuzzNumbers[i] % 5 === 0) {
  console.log('Buzz');
} else {
  console.log(fizzBuzzNumbers[i]);
}

But that could be just:

if (fizzBuzzNumbers[i] % 3 === 0 && fizzBuzzNumbers[i] % 5 === 0) {
  console.log('FizzBuzz');
} else if (fizzBuzzNumbers[i] % 3 === 0) {
  console.log('Fizz');
} else if (fizzBuzzNumbers[i] % 5 === 0) {
  console.log('Buzz');
} else {
  console.log(fizzBuzzNumbers[i]);
}

Technically, your code would read simpler if it was also in a forEach loop, instead of a for(...) loop. Consider the outer loop:

fizzBuzzNumbers.forEach((value) => {
  if (value % 3 === 0 && value % 5 === 0) {
    console.log('FizzBuzz');
  } else if (value % 3 === 0) {
    console.log('Fizz');
  } else if (value % 5 === 0) {
    console.log('Buzz');
  } else {
    console.log(value);
  }
});
\$\endgroup\$
3
  • 1
    \$\begingroup\$ This is excellent feedback @rolfl. Thank you very much! It'll take me a while to understand and digest it all completely, as I'm still a struggling fledgling, but I can see how clunky my code was. I really appreciate you taking the time to review. Thanks again. \$\endgroup\$ – Marc Sharp Feb 28 at 17:08
  • \$\begingroup\$ Checking for multiples of both 3 and 5 can be simplified to multiplies of 15. \$\endgroup\$ – Barmar Mar 1 at 15:57
  • \$\begingroup\$ @Barmar- my answer is not intended to give the best fizzbuzz solution, rather it's to provide some progressive improvements for the OP. I chose a few improvements to point out that will make the OP a better programmer in general, not specifically to solve fizzbuzz ;-) Your point about the %15 is valid though for a highly-performant answer, and if the OP was more advanced in his learning, I would have maybe considered it \$\endgroup\$ – rolfl Mar 1 at 16:24
2
\$\begingroup\$

Aside from fizzBuzzNumbers[i] always being identical to i (as others pointed out), I think this is a fantastic first attempt. I.e. if num was 3, fizzBuzzNumbers would be [1,2,3] and i would go from 1-to-2-to 3 without even needing to reference the array at all beyond length, which is just your input num. Your array could actually be ['big', 'bad', 'wolf'] and the output would be the same for an input of 3.

One resource that I think is worth a watch after trying this problem is Tom Scott's FizzBuzz: One Simple Interview Question on YouTube. He breaks down some common ways that people approach the problem, as well as why some approaches are better than others.

\$\endgroup\$
1
  • \$\begingroup\$ thank you for your comment! It had me grinning because that video was where I got the idea from. Tom says 'pause the video and go and attempt to write a program' and I did. It was the first time I sat down and planned out how to write a program from scratch without assistance. I'm incredibly thankful for your encouragement, especially in this beginning phase of my learning. Thank you. \$\endgroup\$ – Marc Sharp Mar 3 at 20:02

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.