HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

99 times drunk on Java

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
javatimesdrunk

Problem

This is a rags-to-riches 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:

  • It's not immediately obvious what the placeholders for each Line value are expecting.



  • 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 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 getVerse(String value) {
String last = Line.ON_WALL_PREFIX.with(value, value);
return Stream.of(Line.ON_WALL_SUFFIX.with(PASS_ARO

Solution

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 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")));
    }

}

Code Snippets

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")));
    }

}

Context

StackExchange Code Review Q#111583, answer score: 4

Revisions (0)

No revisions yet.