-
-
Save Pharylon/4502183 to your computer and use it in GitHub Desktop.
using System; | |
using System.Collections.Generic; | |
using System.Linq; | |
using System.Text; | |
namespace ConsoleApplication1 | |
{ | |
class Program | |
{ | |
static void Main(string[] args) | |
{ | |
try | |
{ | |
for (int i = 0; i <= 100; i++) | |
{ | |
bool fizz = fizzCheck(i); | |
bool buzz = buzzCheck(i); | |
if (fizz == true && buzz == true) | |
Console.WriteLine("FizzBuzz"); | |
else if (fizz == true) | |
Console.WriteLine("Fizz"); | |
else if (buzz == true) | |
Console.WriteLine("Buzz"); | |
else | |
Console.WriteLine(i); | |
} | |
Console.ReadKey(); | |
} | |
catch (Exception ex) | |
{ | |
Console.WriteLine("Whoops!"); | |
Console.WriteLine(ex.Message); | |
} | |
} | |
private static bool fizzCheck(int i) | |
{ | |
bool fizz = false; | |
if (i % 3 == 0) | |
fizz = true; | |
return fizz; | |
} | |
private static bool buzzCheck(int i) | |
{ | |
{ | |
bool buzz = false; | |
if (i % 5 == 0) | |
buzz = true; | |
return buzz; | |
} | |
} | |
} | |
} |
(Headed over from /. since you asked if I'd hire you)
I actually give this exact task to interview candidates, so here are some things I'd pick up on if you submitted it to me.
Clearly, most of this is irrelevant if you're just writing FizzBuzz for fun, but if this is an interview test - and given that the task itself is trivial - the interviewer is looking for good style and practices.
- Change the namespace to something relevant, e.g.
namespace FizzBuzz
It's a trivial thing, but easy to get right - and that kind of attention to detail is a differentiator between you and the next candidate.
- Remove the unnecessary "using" boilerplate
you're not usingSystem.Collections.Generic
,System.Text
orSystem.Linq
- again, this is attention to detail; it shows me that you're thinking about what your doing, rather than just blindly accepting what the IDE wrote for you. - lost the
Console.ReadKey()
It doesn't add anything, leaves the user sitting there staring at a screen wondering what the computer's up to, and means I can't use this in a pipeline.
(If you really, really MUST have it, then at least print something to tell the user to press enter) - your fizzCheck (and buzzCheck) functions can be reduced to a single line:
private static bool fizzCheck( int i )
{
return ( i % 3 ) == 0;
}
Aside from the fact that GH ate my indentation sigh, this is far easier to read.
(but leave them a separate functions (that way, the name describes intent in the main loop)
- don't compare booleans with
true
- use them directly ( the== true
makes the code harder to read)
if( fizz && buzz )...
(don't compare them with false
either - use the !
operator)
- I'd give additional credit for factoring out the conversion, so you can write tests for it:
string convert( int i )
{
bool fizz = fizzCheck(i);
bool buzz = buzzCheck(i);
if( fizz && buzz )
{
return "FizzBuzz";
}
if( fizz )
{
return "Fizz";
}
if( buzz )
{
return "Buzz";
}
return i.ToString();
}
Then, using your test framework of choice, you can write assertions like:
Assert.Equal( "1", convert(1) );
Assert.Equal( "Fizz", convert(3) );
Assert.Equal( "Buzz", convert(5) );
etc.
(In fact, you should be writing unit tests BEFORE you cut the code....)
- Even MORE credit for factoring out the conversion mechanics into a separate class, that gets instantiated and called from Main (makes for easier testing and reuse).
I probably wouldn't hire you today - but take comfort in the knowledge that I've seen FAR worse solutions :)
Thanks for the feedback. I definitely take comfort in that. I'm only halfway through my book, "Visual C# Step by Step," leaning it as my first programming language. I wouldn't even think of actually trying to get hired to write code at this point, but hopefully that day will come at some point. :)
BTW, I didn't even realize I could return bools like you did in your example. Thanks!
dansouza - This is the spec: "Write a program that prints the numbers from 1 to 100. But for multiples of three print “Fizz” instead of the number and for the multiples of five print “Buzz”. For numbers which are multiples of both three and five print “FizzBuzz”."
Specifically you are supposed to print the words instead of the number. Your's would print the number on every line.