patternjavaMinor
99 times drunk on Java
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:
```
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
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
Linevalue are expecting.
- I need to
skip()byHEADER_COUNTto 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.
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 Lineisn'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 forString.format(). The formattedBOTTLES, in turn, gets substituted intoON_WALL_PREFIXandON_WALL_SUFFIX, so they are clearly not peers. Furthermore, you still havePASS_AROUNDandBUY_MOREas 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 likegetVerse().
- 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_LEVELwas 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.