7
\$\begingroup\$

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();
}
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I leave your sidenote there, but I consider it noise in the question. We normally like to go to the point and leave as much unnessary information out of the question. Your side note could have been a comment on your question. \$\endgroup\$ – Marc-Andre Feb 11 '16 at 15:33
  • 3
    \$\begingroup\$ Understandable. There is redundancy between the sidenote and the tag "beginner". I'll take care of that. \$\endgroup\$ – Fritz Feb 11 '16 at 15:38
  • 3
    \$\begingroup\$ I forgot my good manners! Welcome to Code Review! I'll hope you'll have good reviews for your code! It looks like a good question! Nice edit too! \$\endgroup\$ – Marc-Andre Feb 11 '16 at 15:40
3
\$\begingroup\$

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.
\$\endgroup\$
2
\$\begingroup\$

@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. :-)

\$\endgroup\$
  • \$\begingroup\$ With regards to the loop, your version is functionally different in that fizzbuzz will not display properly. Using the classic values of 3 for fizz and 5 for buzz, the number 15 (and multiples of 15) will display as fizz and not fizzbuzz. The displayString process was how I tried to solve the issue of writing both fizz and buzz to the same line. I appreciate the feedback though! \$\endgroup\$ – Fritz Feb 11 '16 at 18:56
  • 1
    \$\begingroup\$ Ah, yes, thank you for pointing that out. I was negligent, and I shall amend the code as appropriate. \$\endgroup\$ – MY_G Feb 11 '16 at 19:00

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.