Parameterized FizzBuzz

Question

This is a FizzBuzz program, designed to take parameter inputs for start and ending numbers, as well as a "fizz" number and a "buzz" number. The program then outputs the numbers one per line. If the number is divisible by the "fizz" number (normally 3), "fizz" is printed instead, the same with the "buzz" number. If the number is divisible by both, "fizzbuzz" is the output.

I am primarily looking for advice with my choice of logic, syntax, and use of methods with regards to complexity and efficient code. However, all advice is appreciated.

using System;

class FizzBuzzProgram
{
static void Main()
{
    //initialize menu variables
    bool menuvalidinput = false;
    string menuchoice;

    /*get decision if user wants to fizzbuzz, looping
    until a valid input is provided*/
    do {
        Console.WriteLine("Would you like to fizzbuzz? [Y]/[N]");
        menuchoice = Console.ReadLine();
        menuchoice = menuchoice.ToLower();  //convert to lowercase
        if (menuchoice == "y" | menuchoice == "n"){
            menuvalidinput = true;          //accept input
        } else {
            Console.WriteLine("Sorry, input was not valid.");
        }
    } while (menuvalidinput == false);
    //end of input loop

    //interpret the result of either "y" or "n"
    if (menuchoice == "y"){
        FizzBuzzWithInput();
    } else {
        Console.WriteLine("That's ok, have a nice day!");
    }
}

static void FizzBuzzWithInput()
{
    //initialize fizzbuzz input (FBinput*) variables
    int FBinputS, FBinputE, FBinputF, FBinputB;
    bool FBvalidinput = false;

    /*take input for start and end values,
    if start is larger than end ask again*/
    do {
        Console.WriteLine("What number would you like to start from?");
        FBinputS = Convert.ToInt32(Console.ReadLine());
        Console.WriteLine("What number would you like to end at?");
        FBinputE = Convert.ToInt32(Console.ReadLine());

        if (FBinputS > FBinputE){   //check of start and end values
            Console.WriteLine("You chose a starting number larger than your ending number;");
            Console.WriteLine("please input new values.");
        } else {
            FBvalidinput = true;    //accept input
        }
    } while (FBvalidinput == false);
    //end validation loop

    //take fizz and buzz inputs, classically 3 and 5 respectively
    Console.WriteLine("What number would you like to fizz?");
    FBinputF = Convert.ToInt32(Console.ReadLine());
    Console.WriteLine("What number would you like to buzz?");
    FBinputB = Convert.ToInt32(Console.ReadLine());        

    //call the actual FizzBuzz method
    RunFizzBuzz(FBinputS, FBinputE, FBinputF, FBinputB);
}

static void RunFizzBuzz(int FBstart, int FBend, int FBfizz, int FBbuzz)
{
    //initialize the display string
    string c;

    Console.WriteLine("\nFizzbuzz time!\n");

    //fizzbuzz loop   
    for (int i = FBstart; i <= FBend; i++){
        //set c to blank string    
        c = "";

        if(i%FBfizz == 0){
            c = c + "fizz";
        }
        if(i%FBbuzz == 0){
            c = c + "buzz";
        }
        if(c.Length == 0){  /*"if the number is not fizz or buzz"
                            aka: if(i%FBfizz!=0 & i%FBbuzz!=0)*/
            c = Convert.ToString(i);
        }
        //print display string of number i
        Console.WriteLine(c);
    }
    //end fizzbuzz loop

    Console.WriteLine("\nFizzbuzz complete!");
    Console.ReadLine();
}
}

Answer 1

Some quick remarks

• variables should be named using camelCase casing
• avoid declaring multiple variables on the same line to improve readability.
• don't trust the users of your app! Never trust them! If you think, hey they can read and follow instructions you will get into great pain. Always check that inputs are valid. That beeing said you should use int.TryParse() instead of Convert.ToInt32()
• method arguments should be named using camelCase casing as well.
• let your variables have some room to breathe (see the % operator)
• don't use abbreviations for naming stuff. In a few months you won't know what they are about.
• comments should explain why something is done in the way it is. Let the code speak for itself what is done by using names as descriptive as possible.

Answer 2

@Heslacher has given some good suggestions, so I won't repeat what he wrote. Here are some additional comments:

Firstly, I would encapsulate everything in your wrote in Main into a private static method named ReadFromConsole(). Inside the Main method itself, there will be a single line of code:

FizzBuzzProgram.ReadFromConsole();

There is no need to expose the user of your program to unnecessary details regarding how you intend to parse his input -- he just wants to play FizzBuzz; he doesn't care about how you design or implement your program.

Now let's look at the method FizzBuzzWithInput(). It is already very evident from its name that the method runs Fizz Buzz using inputs provided by the user, so there is no need to include 'FB' in the variable names -- it is redundant, and does not convey additional information. So I would suggest that you instead rename

FBinputS, FBinputE, FBinputF, FBinputB;

to simply

start, end, fizz, buzz

Let's turn to the method RunFizzBuzz(). One of the first things I notice is that you have chosen the name c for your display string. But why c? What does it stand for? Would it not be better to pick a more informative name so that it is immediately clear what the string is supposed to be? Personally, I would just call it displayString, since it is supposed to be a display string. Make it a habit to choose clear, informative names -- it will be so much easier for yourself and others to read and maintain your code.

But, on second thoughts, is it really necessary to initialize a string here? After all, you are not trying to build a string to be returned from the method -- c is set to an empty string during each iteration of the for loop. So I would rewrite the method as follows:

for (int i = start; i <= end; i++)
{
    if (i % fizz == 0 && i % buzz == 0)
    {
        Console.WriteLine("FizzBuzz");
    }
    else if (i % fizz == 0)
    {
        Console.WriteLine("Fizz");
    }
    else if (i % buzz == 0)
    {
        Console.WriteLine("Buzz");
    }
    else
    {
        Console.WriteLine(i);
    }
}

Notice that you can pass a value of int type directly into Console.WriteLine(). :-)

I hope these comments are helpful. It is very nice to see such good effort from you; I am sure that you will keep getting better. :-)