7
\$\begingroup\$

This is a attempt from this question, based on the popular 99 Bottles of Beer, by (lightly) using Java 8's stream-based processing.

Any room for improvement in terms of readability? I can only think of two concerns:

  1. It's not immediately obvious what the placeholders for each Line value are expecting.
  2. I need to skip() by HEADER_COUNT to trim away the... header.
public final class Bottles {

    enum Line {
        BOTTLES("%s bottle%s of beer"),
        ON_WALL_PREFIX("%s on the wall, %s."),
        ON_WALL_SUFFIX("%s, %s on the wall.");

        private final String pattern;

        private Line(String pattern) {
            this.pattern = pattern;
        }

        public String with(Object first, Object second) {
            return String.format(pattern, first, second);
        }
    }

    private static final int HEADER_COUNT = 2;
    private static final long ALCOLISM_LEVEL = 99;
    private static final String PASS_AROUND = "Take one down and pass it around";
    private static final String BUY_MORE = "Go to the store and buy some more";

    private Bottles() {
        // empty
    }

    public static void main(String[] args) {
        getLyrics(ALCOLISM_LEVEL).forEach(System.out::println);
    }

    public static List<String> getLyrics(long target) {
        return Stream.concat(LongStream.rangeClosed(0, target)
                        .map(i -> target - i)
                        .mapToObj(Bottles::toWord)
                        .flatMap(Bottles::getVerse)
                        .skip(HEADER_COUNT),
                    Stream.of(Line.ON_WALL_SUFFIX.with(BUY_MORE, toWord(target))))
                .collect(Collectors.toList());
    }

    private static String toWord(long i) {
        return Line.BOTTLES.with(i == 0 ? "no more" : i, i == 1 ? "" : "s");
    }

    private static Stream<String> getVerse(String value) {
        String last = Line.ON_WALL_PREFIX.with(value, value);
        return Stream.of(Line.ON_WALL_SUFFIX.with(PASS_AROUND, value), "",
                Character.toUpperCase(last.charAt(0)) + last.substring(1));
    }
}
\$\endgroup\$
2
  • 5
    \$\begingroup\$ long i? How many beers are you planning to have? \$\endgroup\$ Nov 25, 2015 at 17:53
  • 1
    \$\begingroup\$ @200_success up to 2^63 - 1. \$\endgroup\$
    – h.j.k.
    Nov 25, 2015 at 23:55

2 Answers 2

4
\$\begingroup\$

In a word, I would say that your program feels… discombobulated. I see recognizable bits and pieces of "99 Bottles of Beer", but it's not obvious how it all fits together.

Overall, I'm not sure that it is much of an improvement over the question that inspired this one. Naturally, I'm partial to my answer, which has the advantage of putting all of the conditionals right at the point of use by using the right tools for complex string substitutions. You can streamify the for-loop in my answer, if you like.

That said, I think there are ways to improve a stream-based implementation.

  • enum Line isn't doing much, and makes your code incoherent. "%s bottle%s of beer" is not really a line — it's just a phrase — and you're calling it a "line" just to reuse your wrapper for String.format(). The formatted BOTTLES, in turn, gets substituted into ON_WALL_PREFIX and ON_WALL_SUFFIX, so they are clearly not peers. Furthermore, you still have PASS_AROUND and BUY_MORE as constants defined elsewhere.
  • Defining string constants instead of just using string literals isn't necessarily better. It just makes your eyes jump around to figure out what's going on.
  • Your getVerse() is weird: it doesn't produce what I would think of as a verse, delimited by the paragraph breaks. Rather, it produces the end of one verse and the beginning of the next, unified by the fact that they have the same number of bottles.
  • In getLyrics(), using a stream to generate the sequence 99, 98, …, 1, 0, 99 is awkward. You have to start with an ascending count, reverse it, skip half of a weirdly delimited "verse", and concatenate a special case at the end that kind of looks like getVerse().
  • If you're using streams, why bother converting the lyrics to a List?
  • It's easier (though not necessarily more efficient) to lowercase an entire string than to uppercase just the first letter.
  • ALCOHOLISM_LEVEL was misspelled.
public final class Bottles {
    private static final long ALCOHOLISM_LEVEL = 99;

    private final long n, replenishment;

    private Bottles(long stock, long replenishment) {
        this.n = stock;
        this.replenishment = replenishment;
    }

    public String toString() {
        return String.format("%s bottle%s of beer",
                             n == 0 ? "No more" : n,
                             n == 1 ? "" : "s");
    }

    private String whatToDo() {
        return n == 0 ? "Go to the store and buy some more"
                      : "Take one down and pass it around";
    }

    private static Bottles next(Bottles b) {
        return new Bottles((b.n == 0) ? b.replenishment : b.n - 1,
                           b.replenishment);
    }

    private static String getVerse(Bottles b) {
        return String.format(
            "%1$s on the wall, %2$s.\n%3$s, %4$s on the wall.",
            b,
            b.toString().toLowerCase(),
            b.whatToDo(),
            next(b).toString().toLowerCase()
        );
    }

    public static Stream<String> getLyrics(long target) {
        return Stream.iterate(new Bottles(target, target), Bottles::next)
                     .limit(target + 1)
                     .map(Bottles::getVerse);
    }

    public static void main(String[] args) {
        System.out.println(getLyrics(ALCOHOLISM_LEVEL)
                           .collect(Collectors.joining("\n\n")));
    }

}
\$\endgroup\$
1
  • \$\begingroup\$ Cool, thanks for spotting the typo, and I do think using Bottles as a full-fledged class (instead of an utilities class, as I had) makes more sense too. :) \$\endgroup\$
    – h.j.k.
    Nov 25, 2015 at 23:58
1
\$\begingroup\$

For starters, I can make the toWord(long) method handle negative values properly as well (more on this in a bit):

// renamed as of() too
private static String of(long x) {
    return x < 1 ? Line.BOTTLES.with(x == 0 ? "no more" : "negative", "s")
            : Line.BOTTLES.with(Long.toString(x), x == 1 ? "" : "s");
}

Then, I need to validate the long argument to the getLyrics(long) method, and this is where the improvement above can be applied:

public static List<String> getLyrics(long target) {
    if (target < 0) {
        throw new IllegalArgumentException(of(target));
    }
    // ...
}

The final improvement is to standardize the handling of Title-casing the generated lyrics inside the enum Line:

enum Line {
    BOTTLES("%c%s bottle%s of beer"),
    ON_WALL_PREFIX("%C%s on the wall, %s."),
    ON_WALL_SUFFIX("%C%s, %s on the wall.");

    private final String pattern;

    private Line(String pattern) {
        this.pattern = pattern;
    }

    public String with(String first, String second) {
        return String.format(pattern, first.charAt(0), first.substring(1), second);
    }
}

The patterns now have an extra placeholder %c or %C (lower- and upper-case character, respectively), which the with(String, String) method handles internally. With this, the getVerse(String) method can be further simplified:

private static Stream<String> getVerse(String value) {
    return Stream.of(Line.ON_WALL_SUFFIX.with(PASS_AROUND, value), "",
                        Line.ON_WALL_PREFIX.with(value, value));
} 
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.