3
\$\begingroup\$

I was trying to come up with the most dynamic fizzbuzz answer I could.. and well, this is what I came up with:

public static void main(String[] args) {

    ultraFizzBuzz((Integer arg) -> {
        String retVal = "";
        if(arg % 3 == 0) { retVal += "fizz"; }
        if(arg % 5 == 0) { retVal += "buzz"; }
        return retVal;
    });
}

public static void ultraFizzBuzz(modConditions<Integer> tester) {

    for(int i = 1; i < 100; i++) {
        String result = tester.test(i);
        if(result != "")
            System.out.println(tester.test(i));
        else
            System.out.println(i);
    }
}

In here the modConditions is an interface with a method (test) that returns type String.

What I was basically trying to do is create a super dynamic function that lets you add as many fizzes and buzzes and pops as you wanted.. but I think in the process accidentally made it so you end up writing 90% of the code in a lambda statement everytime you want to call it.

Am I overcomplicating things? Would this be considered bad programming practice?

\$\endgroup\$
  • \$\begingroup\$ What is modConditions? Also, in traditional FizzBuzz, the program should echo every number that is not a multiple of 3 or 5 — does your code do that? (Putting this question on hold until these issues are resolved.) \$\endgroup\$ – 200_success Mar 9 '18 at 0:43
  • \$\begingroup\$ It wasn't as much of an issue as it was me not knowing the rules to fizzbuzz, but alrighty, I believe it's fixed. modConditions is just an interface with one abstract method that returns a string and takes an argument of type T. \$\endgroup\$ – NaN Mar 9 '18 at 0:53
  • 2
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Simon Forsberg Mar 9 '18 at 22:15
6
\$\begingroup\$

The most important problem I have with your code is, that it totally fails to achieve the goal you set yourself. This is not dynamic at all. Basically, you take the "normal" method body and put it into a lambda, but this lambda now is a complex function, which contains all of the business logic.

Dynamically, you'd need a collection (probably list for being ordered) of lambdas, which each check a single input value and return an appropriate string representation (i.e. "fizz" or "buzz" or "whatever" or the empty string) and a method to apply this complete list on a single input, accumulate the results and perform the final check whether to output that string or the integer itself.

Thus, start out with something like:

Function<Integer, String> fizz = i -> i % 3 == 0 ? "fizz" : "";
Function<Integer, String> buzz = i -> i % 5 == 0 ? "buzz" : "";
Function<Integer, String> barz = i -> i % 7 == 0 ? "barz" : "";
List<Function<Integer, String>> fizzBuzzList = Arrays.asList(fizz, buzz, barz);

And try to come up with code that operates on such a list. Note: Function is the standard function type with an integer as an input and a string as the output. No need to define your own interface.

If you have achieved this, you can easily add another function to the list to check another value, and then you have achived something that might be considered dynamic.

\$\endgroup\$
  • \$\begingroup\$ @NaN Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Simon Forsberg Mar 9 '18 at 22:15
  • \$\begingroup\$ @SimonForsberg so what do I do, post another question on the site to see if the changes I incorporated could still be improved upon? Seems a bit wasteful.. I'll find a different website to post it on. thanks \$\endgroup\$ – NaN Mar 9 '18 at 22:25
5
\$\begingroup\$

This is suboptimal, and kind of buggy:

String result = tester.test(i);
if(result != "")
    System.out.println(tester.test(i));
else
    System.out.println(i);

First of all, you are calling test.test(i) again, unnecessarily, instead of using result.

Second, result != "" is not a good way to test for string equality. You're relying on the fact that the "" in ultraFizzBuzz() and the "" in the lambda happen to be the same exact object, due to the constant pool. If the lambda had been written with String retVal = new String(new char[] {}); instead, then the comparison would break. You should test for either "".equals(result) or result.isEmpty().

You should never omit the "optional" braces, as you will be contributing to a future coding accident.

Here's one better way to write the loop body:

String result = tester.test(i);
System.out.println("".equals(result) ? String.valueOf(i) : result);

I'd expect a method that is named test() to return a boolean. It's surprising that it returns a string.

I think that making the modConditions interface generic is overkill. If you think you might need to support long or BigInteger (really?!), then just write it using long or BigInteger. And if it's not any kind of integer at all, then at that point it wouldn't really be FizzBuzz anyway. YAGNI.

Since you are already using a lambda, I would recommend writing this code using Java 8 streams. It would look more elegant, and you would avoid boxing the int into Integer.

\$\endgroup\$
-1
\$\begingroup\$

I would probably create the following methods:

public static String singleUltraFizzBuzz(SortedMap<Integer, String> fizzBuzzes, int i) {
    StringBuilder result = new StringBuilder();
    bool isEmpty = true;

    for(Map.Entry<String, Object> entry : fizzBuzzes.entrySet()) {
        if(i % entry.getKey() == 0) {
            result.append(entry.getValue());
            isEmpty = false;
        }
    }

    return isEmpty ? i.toString() : result.toString();
}

public static String ultraFizzBuzz(SortedMap<Integer, String> fizzBuzzes, int min, int max) {
    String out = "";

    for(int i = min; i < max; i++) {
        out = out + singleUltraFizzBuzz(fizzBuzzes, i) + "\n";
    }
}

which would be called as follows:

SortedMap<Integer, String> fizzBuzzes = new TreeMap<>();
fizzBuzzes.put(3, "fizz");
fizzBuzzes.put(5, "buzz");
System.out.write(ultraFizzBuzz(fizzBuzzes, 1, 100));
\$\endgroup\$
  • \$\begingroup\$ Why the downvote? \$\endgroup\$ – Solomon Ucko Mar 14 '18 at 13:33

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.