3
\$\begingroup\$

I wrote a FizzBuzz program to get comfortable with C++. I haven't learned any languages before this, but I'm doing this on my own. I learned if, for, std::cout, and some operators. I wrote several questions below, but if that isn't allowed I can revise.

Program uses nested for loops to evaluate whether a quotient is a equivalent to an integer.

Questions

  1. Any problems with my variable names? Commenting? White space usage?

  2. Instead of evaluating comparisons every time the main counter increments, would it be faster to store all possible increments in a "list" of some kind and then do my comparisons with elements in that list? Collating multiples of multiples within a list would require searching for that element within a list, which could be costly.

  3. Would a straight forward counting approach or a list approach be faster for n counter values greater than a few digits? i.e. finding all multiples up to 100,000 or 1,000,000.

  4. I realize after writing this that I could have used modulus %. However, modulus would only account for integer multiples, correct? (i.e. I would not be able to use "3.5" as one of my multiples.)

  5. To add to the above, I want to run this algorithm using floats instead of integers (e.g. multiples of fractions, or PI to several digits out), and I want to count out to some arbitrary limit using appropriate intervals (e.g. +0.5, +0.001). Regardless of my approach, are these types of evaluations inextricably linked to issues with float precision (imprecision)?

  6. This question is off-topic (sorry), but how much of a problem in coding is the issue of organization in code? I ask because I didn't need to use the variables int passFizz and int passBuzz because I could have used int checkFizzBuzz in their places instead. However, if I were to add dozens of multiples to check for, I'd think that using a single global variable would be organizationally poor.

#include <iostream>

// Prints numbers "1" through "100" as integers.
// Every multiple of "3" is printed as "Fizz".
// Every multiple of "5" is printed as "Buzz".
// Every common multiple of "3" and "5" is printed as "FizzBuzz".
// for loops evaluate whether a quotient is an integer.
int main()
{
    float counterMain = 1;
    // factorMain will be set to the multiples for Fizz or "Buzz.
    float factorMain = 0;
    // factorTest "tests" factorMain to see if it is an integer factor of counterMain.
    float factorTest = 1;
    // factorLimit sets the divisor (factor) upper limit (only one other factor in this case).
    // factorLimit is a quotient of factorMain and factorTest.
    float factorLimit = 0;
    int passFizz = 0;
    int passBuzz = 0;

    // counterMain gets printed at the end if nothing is passed to checkFizzBuzz.
    for(counterMain = 1; counterMain <= 100; ++counterMain)
    {
        // Resets pass check.
        passFizz = 0;
        passBuzz = 0;
        // "Fizz" value set here.
        factorMain = 3;
        // Sets the divisor upper limit as a quotient of counterMain.
        factorLimit = counterMain / factorMain;

        //factorTest counts to the divisor upper limit.
        for(factorTest = 1; factorTest <= factorLimit; ++factorTest)
        {
            // Tests if factorTest is an integer quotient of counterMain divided by factorMain.
            if(factorTest == factorLimit)
            {
                // Passes 1 to passFizz if factorMain is an integer.
                passFizz = 1;
                // Sets factorTest to for loop exit condition.
                factorTest = factorLimit;
            }
        }

        // "Buzz" value is set here.
        factorMain = 5;
        factorLimit = counterMain / factorMain;
        // Copy of the above for loop but passes 2 to passBuzz instead.
        for(factorTest = 1; factorTest <= factorLimit; ++factorTest)
        {
            if(factorTest == factorLimit)
            {
                passBuzz = 2;
                factorTest = factorLimit;
            }
        }

        // Fizz-Buzz pass check, then prints out.
        int checkFizzBuzz = (passFizz + passBuzz);
        // Value of 0 means no factors were found, prints counterMain value instead.
        if(checkFizzBuzz == 0)
        {
            std::cout << counterMain;
        }
        if(checkFizzBuzz == 1)
        {
            std::cout << "Fizz";
        }
        if(checkFizzBuzz == 2)
        {
            std::cout << "Buzz";
        }
        if(checkFizzBuzz == 3)
        {
            std::cout << "FizzBuzz";
        }
        std::cout << "\n";
    // Return to counterMain for loop.
    }

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Program is far more complicated than it needs to be. Did you check its output? Does it run correctly? \$\endgroup\$ – markspace Sep 28 at 6:41
  • \$\begingroup\$ I realize after writing this that I could have used modulus %. However, modulus would only account for integer multiples, correct? (i.e. I would not be able to use "3.5" as one of my multiples.) You realize that point of FizzBuzz is to give a simple test to screen out programmers who can't code at all? Your added complication seems to miss that point entirely. blog.codinghorror.com/why-cant-programmers-program \$\endgroup\$ – markspace Sep 28 at 6:43
  • 1
    \$\begingroup\$ @markspace Some people like to make it a bit more interesting, nothing wrong with that. Wrote an overly complicated version myself once. \$\endgroup\$ – Mast Sep 28 at 7:35
  • \$\begingroup\$ @markspace All questions to Code Review must contain working code. There are a number of online compilers to test with. See stackoverflow.com/questions/567682/…. \$\endgroup\$ – lapostal Sep 28 at 20:25
1
\$\begingroup\$

The first thing I'd suggest is to learn to put algorithms in separate functions. This allows your maintenance to be much simpler. If a change is needed, as long as the parameters and the return type don't change, you don't have to worry about the rest of the program.

If you want to use different types with your algorithm, one way to go is to use generics. The main caveat to this is that checking that the type used is appropriate(i.e. using strings would throw an error), becomes very complicated.

One of the most common mistakes people who are learning this algorithm make, is checking 3 times, once for each factor then both factors combined. if you check for both factors back to back this will automatically give you the combined factor.

You are using int to indicate a valid factor. It would make more sense to use a bool.

Putting it all together, it can be simplified very greatly. Something like this would work:

template<typename T>
void FizzBuzz(T start, T end, T factor1, T factor2, T step)
{
    for (T i = start; i < end; i += step)
    {
        bool fizz = (i % factor1) == 0;
        bool buzz = (i % factor2) == 0;
        if (!fizz && !buzz)
        {
            cout << i << '\n';
        }
        else
        {
            if (fizz)
            {
                cout << "Fizz";
            }
            if (buzz)
            {
                cout << "Buzz";
            }
            cout << '\n';
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I agree that finding a way to use my algorithm as a template would be organizationally simpler. I would like to add that the cost-benefit curve for checking two factors at once falls below the curve for checking factors independently, the greater the counting number becomes (e.g. counting to 1,000,000). This algorithm uses a per-factor divisor upper limit to reduce the number needed to count to. However, I agree that adding more to-be-checked factors would require a case-by-case analysis. \$\endgroup\$ – lapostal Sep 28 at 22:09
  • \$\begingroup\$ While bool seems preferred to my summed int approach, wouldn't the return "all are zero" (!fizz && !buzz) be more efficient at run time if it were coded as discrete evaluations? Isn't my initial approach of listing named variables considered essentially the same as discretely listing elements in a list, outside of list form? Does the else have value here? The following ifs will run anyway since boolean comparisons can only evaluate to either true or false, therefore the following statements will run as if the previous if had passed 0 anyway. \$\endgroup\$ – lapostal Sep 28 at 22:14

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.