Caffeine-Driven FizzBuzz

Question

I have been given following problem to solve:

• If the integer is divisible by 3, return the string "Java".
• If the integer is divisible by 3 and divisible by 4, return the string "Coffee"
• If the integer is one of the above and is even, add "Script" to the end of the string. Otherwise, return the string "mocha_missing!"

Sample Input and Output

• caffeineBuzz(1) => "mocha_missing"
• caffeineBuzz(3) => "Java"
• caffeineBuzz(6) => "JavaScript"
• caffeineBuzz(12) => "CoffeeScript"

---

//Class 

    public class CoffieScriptGenerator {

        public String caffeineBuzzz(Integer number) {

            if(isDivisbleByThreeAndFour(number))
            {
                if(isEven(number))
                return "CoffeeScript";
                else
                return "Coffee";
            }
            if(isDivisbleByThree(number))
            {
                if(isEven(number))
                return "JavaScript";
                else
                return "Java";
            }
            return "mocha_missing!";


        }

        private boolean isEven(Integer number) {
            return number%2==0; 
        }

        private boolean isDivisbleByThreeAndFour(Integer number) {

            return isDivisbleByThree(number) && number%4==0;
        }

        private boolean isDivisbleByThree(Integer number) {
            return number%3==0;
        }

    }

Please provide suggestions to refactor it and improve it and how i can improve such nesting if else with help of patterns like strategy.

Answer 1

I would change this up a bit

    public String caffeineBuzzz(Integer number) {
        if(isDivisbleByThreeAndFour(number))
        {
            if(isEven(number))
            return "CoffeeScript";
            else
            return "Coffee";
        }
        if(isDivisbleByThree(number))
        {
            if(isEven(number))
            return "JavaScript";
            else
            return "Java";
        }
        return "mocha_missing!";
    }

First thing is that you are not indenting the if statement blocks that are nested, that smells. the next thing is that you are repeating the call to the isEven function, I would pull this out of the code and perform it after checking the divisibility of the number.

I think that you should have an if/else if/else statement and return at the end of the code. like this

String outputString = "";

if (isDivisbleByThreeAndFour(number)) {
    outputString = "Coffee";
} else if (isDivisbleByThree(number)) {
    outputString = "Java";
} else {
    return "mocha_missing";
}

if (isEven(number)) {
    outputString = outputString.concat("Script");
}

return outputString;

I also created an output variable so that I could concatenate instead of hard code extra strings, if we added something to your "fizzbuzz" like for a 5 or a 6 it could get messy if you have to keep writing the code like you did, especially if there are going to be different combinations of numbers and phrases.

---

another thing that doesn't shorten the length of the written code is that if a number is divisible by 4 it is divisible by 2 and is therefore even so just return there as well like this

if (isDivisbleByThreeAndFour(number)) {
    return = "CoffeeScript";
} else if (isDivisbleByThree(number)) {
    outputString = "Java";
} else {
    return "mocha_missing";
}

You won't ever have "Coffee" without "Script"

String outputString = "";

if (isDivisbleByThreeAndFour(number)) {
    return = "CoffeeScript";
} else if (isDivisbleByThree(number)) {
    outputString = "Java";
} else {
    return "mocha_missing";
}

if (isEven(number)) {
    outputString = outputString.concat("Script");
}

return outputString;

if we do what @Mat'sMug says about the divisibility we can probably clean this up a bit more

String outputString = "";

if (number % 3 == 0) {
    if (number % 4 == 0) {
        outputString = "CoffeeScript";
    } else if (number % 2 == 0) {
        outputString = "JavaScript";
    } else {
        outputString = "Java";
    }
} else {
    outputString = "mocha_missing";
}

return outputString;

This does each check at the most once.

We should return early and often...

if (number % 3 == 0) {
    if (number % 4 == 0) {
        return "CoffeeScript";
    } else if (number % 2 == 0) {
        return "JavaScript";
    } else {
        return "Java";
    }
} else {
    return "mocha_missing";
}

some would say that you should always have a default outside of the if statements, and in this case I would probably remove the outside else statement and return "mocha_missing" if that is reached.

so here is probably what I would end with....

if (number % 3 == 0) {
    if (number % 4 == 0) {
        return "CoffeeScript";
    } else if (number % 2 == 0) {
        return "JavaScript";
    } else {
        return "Java";
    }
} 
return "mocha_missing";

Answer 2

Your private one-liner functions do very little to help with anything, and they're not really DRY. Might as well do this:

private boolean isDivisibleBy(Integer number, Integer divideBy) {
    return number % divideBy == 0;
}

As you can see this does nothing much more than wrap the modulo operator with a function call... which is questionable. Notice:

• "Divisible" - not sure why your functions lost an i in the process.
• Spacing. There's no need to cram operators and operands together into as little horizontal spaces as possible - let code breathe. Especially if the reason for having these functions in the first place, is to improve readability!

---

Your code doesn't comply with the specs: it's not adding "Script" to the end of the string when the number is even. In fact, your logic is redundant:

if(isDivisbleByThreeAndFour(number)) {
    ...
}

if(isDivisbleByThree(number)) {
    ...
}

Notice:

• Both blocks check if number can be divided by 3.
• Consistent brace position. If you're going to put scope-opening braces at the end of the line, keep 'em that way - don't switch to C#-style next-line bracing in the middle of your code!

Your bracing style not only inconsistent, the braces themselves are inconsistently present!

    if(isEven(number))
    return "CoffeeScript";
    else
    return "Coffee";

Don't do that. Use braces, and indentation. For your own health.

if(isEven(number)) {
    return "CoffeeScript";
} else {
    return "Coffee";
}

Note, when you have if (x) { return foo; } else { return bar; }, it can be rewritten with a ternary operator, like so:

return x ? foo : bar;

But you don't want to do that here.

You don't want that, because you'll need nested ternaries to remove the redundancies, and that would look terrible.

What would the correct logic be then?

Just follow the specs:

• Check to see if it divides by 3 - if it does:
  • If it also divides by 4, we got "Coffee". Else, we got "Java".
  • If it also divides by 2, append "Script" to whatever we got.
• Otherwise, return "mocha_missing!"

The nesting here should give you a clue: you only need to check once if the number can divide by 3.

String value;
if (number % 3 == 0) {
    if (number % 4 == 0) {
        value = "Coffee";
    } else {
        value = "Java";
    }
    if (number % 2 == 0) {
        value += "Script";
    }
} else {
    value = "mocha_missing!";
}

return value;

Strategy Pattern seems very much overkill here. If this is for an interview, I'd say don't overthink it - they're just trying to see if you can read specs.

Answer 3

I would start by asking for a better specification. The way it's worded now:

• If the integer is divisible by 3, return the string "Java".
• If the integer is divisible by 3 and divisible by 4, return the string "Coffee"
• If the integer is one of the above and is even, add "Script" to the end of the string. Otherwise, return the string "mocha_missing!"

...it's not clear exactly when the final clause about returning "mocha_missing!" should be invoked. As it's shown, it should really apply to 3 (for one example) so caffeineBuzz(3) => "Java" is really wrong--this should yield mocha_missing!.

If we modify it so the final clause is its own bullet point:

• If the integer is divisible by 3, return the string "Java".
• If the integer is divisible by 3 and divisible by 4, return the string "Coffee"
• If the integer is one of the above and is even, add "Script" to the end of the string.
• Otherwise, return the string "mocha_missing!"

...then the spec probably matches the examples.

Assuming that's the real intent, we can start by observing that any number that's divisible by 4 is also divisible by 2, so the "Coffee" leg of the testing, there's no real point in testing for divisibility by 2 as well.

Since the result is determined solely by the remainder after dividing by 3 and/or 4, we can determine the result based on the remainder after division by 12:

public String caffeineBuzzz(Integer number) {
    return results[number % 12];
}

...where the results array looks something like this:

String results[] = {
    "CoffeeScript",
    "mocha_missing!",
    "mocha_missing!",
    "Java",
    "mocha_missing!",
    "mocha_missing!",
    "JavaScript",
    "mocha_missing!",
    "mocha_missing!",
    "Java",
    "mocha_missing!",
    "mocha_missing!"
};

This could also be done with something like a map that only contained entries for the defined values, and then returning mocha_missing! for any value not present in the map. If we had to deal with a larger number of values, that'd become worthwhile, but with only 12 values, we're using so little to start with that there's almost no chance to gain anything meaningful from that.

Answer 4

• If the integer is divisible by 3, return the string "Java".
• If the integer is divisible by 3 and divisible by 4, return the string "Coffee"
• If the integer is one of the above and is even, add "Script" to the end of the string.
• Otherwise, return the string "mocha_missing!"

Now depending on whether your priority is performance or simplicity/maintainability would determine which of two different paths I would take the code as the requirements can be reduced to either:

• If the integer is divisible by 12, return "CoffeeScript"
• If the integer is divisible by 6, return "JavaScript"
• If the integer is divisible by 3, return "Java"
• Otherwise, return "mocha_missing!"

or:

• If the integer is divisible by 3 and:
  • is divisible by 4, return "CoffeeScript"
  • is divisible by 2, return "JavaScript"
  • is not divisible by 2, return "Java"
• Otherwise, return "mocha_missing!"

Simplicity

private static final String COFFEE  = "CoffeeScript";
private static final String JS      = "JavaScript";
private static final String JAVA    = "Java";
private static final String MISSING = "mocha_missing!";

public static boolean isDivisibleBy(
        final int value,
        final int divisor
){
    return value % divisor == 0;
}

public static String toCaffeineBuzz(
        final int value
){
    if ( isDivisibleBy( value, 12 ) )
        return COFFEE;

    if ( isDivisibleBy( value, 6 ) )
        return JS;

    if ( isDivisibleBy( value, 3 ) )
        return JAVA;

    return MISSING;
}

Performance

You can test for divisibility by 4, even and odd values by looking at the 2 least-significant bits (i.e. value & 3).

• If it is divisible by 4 then these bits will be 0
• If it is even but not divisible by 4 then the bits will be 2
• If it is odd then the bits will be 1 or 3.

You then only need to check if the answer is divisible by 3 and which of those options it is.

public static String toCaffeineBuzz2(
        final int value
){
    if ( isDivisibleBy( value, 3 ) )
    {
        switch( value & 3 )
        {
        case 0:  return COFFEE;
        case 2:  return JS;
        default: return JAVA;
        }
    }
    return MISSING;
}

Comparison

public static void main( final String[] args ) throws Exception{
    final int NUM_TESTS = 10000;
    final int TEST_SIZE = 1000000;

    String[] valuesSimple = new String[TEST_SIZE];
    String[] valuesPerf   = new String[TEST_SIZE];
    long    simpleTime = 0,
            perfTime = 0, 
            startTime, 
            endTime;

    for ( int i = 0; i < NUM_TESTS; i++ )
    {
        startTime = System.nanoTime();
        for ( int j = 0; j < TEST_SIZE; j++ )
            valuesSimple[j] = toCaffeineBuzz(j);
        endTime   = System.nanoTime();
        simpleTime += endTime - startTime;

        startTime = System.nanoTime();
        for ( int j = 0; j < TEST_SIZE; j++ )
            valuesPerf[j] = toCaffeineBuzz2(j);
        endTime   = System.nanoTime();
        perfTime += endTime - startTime;
    }

    System.out.println( "Simple Test (ms): " + ( simpleTime / NUM_TESTS / 1e6 ));
    System.out.println( "Performance Test (ms): " + ( perfTime / NUM_TESTS / 1e6 ));

    for ( int j = 0; j < TEST_SIZE; j++ )
        if ( valuesSimple[j] != valuesPerf[j] )
            throw new Exception( "Tests do not match" );
}

Outputs:

Simple Test (ms): 13.779076
Performance Test (ms): 7.38712

Checking the 2 least-significant bits is done in just over half the time compared to checking multiple divisors.

Answer 5

I prefer avoiding at all cost nested if when possible. Our brain does not work by branching into complex logic. It is easier to evaluate guards one by one. Guards are defined like this by Wikipedia:

https://en.wikipedia.org/wiki/Guard_(computer_science)

In computer programming, a guard is a boolean expression that must evaluate to true if the program execution is to continue in the branch in question.

I would refactor this code to be similar to the english version of your requirement. You did not use branching in your requirement so why implement functional requirement with branching?

/*
 - If the integer is divisible by 3, return the string "Java".
 - If the integer is divisible by 3 and divisible by 4, return the string "Coffee"
 - If the integer is one of the above and is even, add "Script" to the end of the string. 
 - Otherwise, return the string "mocha_missing!"
 */
public String caffeineBuzzzRefactored(Integer number) {
    String result = null;
    if (isDivisibleBy(3, number))
        result = "Java";
    if (isDivisibleBy(3, number) && isDivisibleBy(4, number))
        result = "Coffee";

    // if is one of the above we continue. otherwise terminate with disappointing mocha_missing.
    if (result == null)
        return "mocha_missing!";

    // for even numbers divisible by 3 or 4 we add Script
    if (isEven(number))
        return result.concat("Script");

    // otherwise return previous result with no addition
    return result;
}

private boolean isDivisibleBy(Integer divisor, Integer number) {
    return number % divisor == 0;
}

I feel like this could be even better with one function resolving the first part of the word and then adding "Script" if needed.

String result = resolve(number); // return "Java", "Coffee" or null
if (result == null)
    return "mocha_missing!"

if (isEven(number))
    return result.concat("Script");

return result;