9
\$\begingroup\$

Fairly new to learning interview questions and I saw this one and gave it a shot. I would very much appreciate some constructive criticism of how I went about it and what you would tell me if you were the interviewer and watched me implement it this way.

public class FizzBuzz {
  public static void main(String[] args) {
    // Go from 1 to 100
    for (int i = 1; i <= 100; i++) {
      String result = "";
      if (i % 3 == 0) { // If divisible by 3
        result += "Fizz";
      }
      if (i % 5 == 0) { // If divisible by 5
        result += "Buzz";
      }
      // If it was divisible by either 3 or 5 (or both), print Fizz, Buzz, or FizzBuzz accordingly.
      if (i % 3 == 0 || i % 5 == 0) {
        System.out.println(result);
      } else { // Or else just print the integer
        System.out.println(i);
      }
    }
  }
\$\endgroup\$
18
\$\begingroup\$

The point of FizzBuzz is to weed out obviously unqualified candidates, not to over-optimize the solution or to show off. Your code is a fine interview answer.

Make sure that your braces all match up. It would be more conventional to use 4 or 8 spaces per level of indentation.

My main suggestion — and this is an opinionated nitpick — is to tone down the comments, because comments that restate obvious code give the impression that you might be unfamiliar with the language. Experienced programmers never write // Go from 1 to 100 to explain for (int i = 1; i <= 100; i++). If you feel compelled to demonstrate your documentation skills, write some JavaDoc instead.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the reply 200_success, I really appreciate it! So, for the braces, I was using the GoogleStyle formatter I read about. I changed it to indentation size to 4, for spaces. That's what you mean correct? Yeah, that's understandable. I just did it because I wanted to walk myself through it but I usually don't comment this much. \$\endgroup\$ – ProjectDefy Nov 20 '16 at 9:29
4
\$\begingroup\$

Quite perfect so far.

public class FizzBuzz {
  public static void main(String[] args) {
    // Go from 1 to 100
    for (int i = 1; i <= 100; i++) {
 1)   String result = "";
 2)   if (i % 3 == 0) { // If divisible by 3
        result += "Fizz";
      }
 2)   if (i % 5 == 0) { // If divisible by 5
        result += "Buzz";
      }
      // If it was divisible by either 3 or 5 (or both), print Fizz, Buzz, or FizzBuzz accordingly.
  2)  if (i % 3 == 0 || i % 5 == 0) {
        System.out.println(result);
      } else { // Or else just print the integer
        System.out.println(i);
      }
    }
  }

The only things I see:

In 1) the interviewer could ask if the code could be made faster. In this case, while += is perfectly fine, you could use concat or StringBuilder(). But for FizzBuzz I see it as unnecessary optimization.

In 2) you are testing the conditions twice. This is not so good because if the condition changes, you will likely forget to update the code on both sides.

Better simply test the String.

if (!result.isEmpty())

Cleaner way is to avoid negation and exchange the branches

if (result.isEmpty()) {  // Print integer
  ...
}
else {                   //Print Fizz Buzz
  ... 
}

or use booleans

String result = "";  
boolean isFizz = i % 3 == 0;  
boolean isBuzz = i % 5 == 0;

if (isFizz || isBuzz) {
...
}

The next step which is overkill for interview question (so your solution is preferable because you less likely to commit errors, but it may be asked if the interviewer wants to know if you can clean up the code) is e.g. replacing i with something more descriptive ("number" in this case, "index" if you use it for array access and "counter" if it is only used to count sth down) and replace 3 and 5 as magic numbers by descriptive constants.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

The thing about FizzBuzz is that you can write simple and elegant code to solve the problem in almost any program language without going over the top.

This code below is simple and shows you understand hierarchy of if/elseif/else:

public class FizzBuzz {
  public static void main(String[] args) {
    for (int i = 1; i <= 100; i++) {
      if (i%3 == 0 && i%5 == 0)
        System.out.println("FizzBuzz"); 
      else if (i%5 == 0) 
        System.out.println("Buzz"); 
      else if (i%3 == 0) 
        System.out.println("Fizz"); 
      else 
        System.out.println(i);
    }
  }
}
| improve this answer | |
\$\endgroup\$
  • 8
    \$\begingroup\$ FizzBuzz questions tend to attract a lot of "you could write it like this" answers. Since this is Code Review, could you be specific about what your recommendations are, and why? \$\endgroup\$ – 200_success Nov 20 '16 at 10:56
0
\$\begingroup\$

Here it is my c++ implementation. It uses a recursive algorithm. Basically the last iteration create the vector then the result array is populated on every recursion with a push_back. It uses the "module 15", "module 5", module 3" in a ternary operator series.

vector<string> FizzBuzz::execute(int n) {

    if(n == 0)
        return vector<string>();

    auto push = n % 15 == 0 ? "fizzbuzz" :
                n % 3 == 0 ? "fizz" :
                n % 5 == 0 ? "buzz" :
                to_string(n);

    auto execution = execute(n-1);
    execution.push_back(push);
    return execution;
}
| improve this answer | |
\$\endgroup\$
-2
\$\begingroup\$

Since you want to improve, compare these:

var x = default

if a
    x += i
if b 
    x += j

if x != default
    func(x)

.

var x = default

if (a & b)
    x = k
else if a
    x = i
else if b 
    x = j

if x != default
    func(x)

.

if (a & b)
   func(k)
else if a
   func(i)
else if b 
   func(j)

First, the most important thing you'll notice is that there isn't much of a difference. Second, you'll notice that the one where the reader spends the least amount of time to understand the intent of the code is the last one.

I used pseudocode because this isn't specific to java.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ this is nonsense, they do different things for starters. You seem to be implying a function call is easier to understand than the one line of code it replaces - very debatable. We use a limit of ~30 lines for requiring a new function/method \$\endgroup\$ – NimChimpsky Nov 20 '16 at 23:20
  • \$\begingroup\$ @nimchimpsky no, to all of the assertions you made. This is not nonsense, they do the same thing, and I'm not implying what you say. \$\endgroup\$ – Peter Nov 20 '16 at 23:34
-4
\$\begingroup\$

The "FizzBuzz" Programm is a coding kata.

Coding Katas are very simple problems which are easy to solve. And their main aim is not to solve the problem (since it is quite easy) but to train problem solving approaches.

One of this problem solving approaches (and the origin of Coding Katas) is Test Driven Developement (TDD).

So When I was the interviewer and you would not start the Coding by writing a Test you were not my first choice... ;o)

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ DV for requiring tests in a test obviously made for verifying you are familiar with a certain language, not for testing methodologies. \$\endgroup\$ – Pål GD Nov 20 '16 at 12:54
  • \$\begingroup\$ @PålGD: You (and I) cannot tell what an interviewer may have in mind. And the as far as I understood the TO he is preparing for interviews... \$\endgroup\$ – Timothy Truckle Nov 20 '16 at 13:00
  • \$\begingroup\$ Didin't downvote, but if I was the interviewer, I'd prefer the candidate who correctly solves the problem in less time than the TDD candidate needs to write the test suite. Going TDD on a trivial problem during an interview is gambling. \$\endgroup\$ – Peter Nov 20 '16 at 22:29
  • \$\begingroup\$ @Peter: If you prefer working with "duck tape programmers", your choice, I don't. Also: this particular Kata tells nothing about the Java skills of the candidate. The solution could be copy/pasted to almost any other programming language and would run with very little modifications. \$\endgroup\$ – Timothy Truckle Nov 20 '16 at 22:38
  • 2
    \$\begingroup\$ @TimothyTruckle I prefer working with programmers who have a large toolset, know when to use which tool, and also have duct tape in their toolbox. \$\endgroup\$ – Peter Nov 20 '16 at 22:41

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.