5
\$\begingroup\$

What this method should do is return a String that displays how many albums and songs there are. This method is a private method of my Artist class which has a method called albumCount() and songCount(). The whole class probably needs an improvement but I will try that later. First I'd like this single method to be improved if possible.

public String toAlbumSongCount() {
    StringBuilder builder = new StringBuilder();

    final int albumCount = albumCount();
    final int songCount = songCount();

    builder.append( albumCount );
    //Append albums instead of album if albumCount is greater than 1
    builder.append( (albumCount > 1) ? " albums" : " album" );
    builder.append( "," ); //Move this to the line above?
    builder.append( songCount );
    //Append songs instead of song if songCount is greater than 1
    builder.append( (songCount > 1 ) ? " songs" : " song" );

    return builder.toString();
}

I used the StringBuilder class because I heard a dozen of times that it will improve performance when concatenating Strings. I wasn't sure about those two lines :

builder.append( (albumCount > 1) ? " albums" : " album" );
builder.append( (songCount > 1 ) ? " songs" : " song" );

Are they good enough with the comments above them?

I appreciate any help even if it means completely rewriting this method.

\$\endgroup\$
5
\$\begingroup\$

I don't think these are any major improvements, but there are many small things that I would change.

  • albumCount and songCount is both the name of a variable, and of a method
  • Pluralization can be extracted to a separate method
  • Remove unnecessary comments and spaces

So here's what I would do:

public String toAlbumSongCount() {
    StringBuilder builder = new StringBuilder();
    final int albumCount = getAlbumCount();
    final int songCount = getSongCount();

    builder.append(albumCount);
    builder.append(pluralize(albumCount, " album", " albums"));
    builder.append(", ");
    builder.append(songCount);
    builder.append(pluralize(songCount, " song", " songs"));

    return builder.toString();
}

private static final String pluralize(int count, String singular, String plural) {
    return count == 1 ? singular : plural;
}

A tiny extra thing that is often forgotten is to pass an argument to the StringBuilder, by default it is initialized with a capacity of 16 characters. If we look at a little example, 99 albums, 999 songs we see that that's 20 characters. So the internal array in AbstractStringBuilder will need to be resized once. We could create the StringBuilder with a capacity of 24 to reduce the need for the resize.

StringBuilder builder = new StringBuilder(24);
\$\endgroup\$
  • 2
    \$\begingroup\$ I like the way that pluralize can handle other types of plurals, like "mouse" and "mice". It will also be faster than the concatenation. \$\endgroup\$ – rolfl Jun 15 '15 at 13:42
  • \$\begingroup\$ Your plurals are hardcoded which hinders localization. Of course, many times you don't need that. But if you do, there are languages for it is not enough to use a single plural. For example, in Czech, the plural form for 2 is different than the one for 5. See en.wikipedia.org/wiki/Czech_declension#Plural_forms \$\endgroup\$ – user140547 Jun 15 '15 at 14:07
  • 1
    \$\begingroup\$ @user140547 This was not a question about localization. I never considered localization here. The issue that you raise can be easily fixed though, by for example using different pluralization methods for albums and songs, and read the appropriate strings from a localization file. Perhaps this might be something that you want to implement yourself? Could be an interesting follow-up to this question. \$\endgroup\$ – Simon Forsberg Jun 15 '15 at 14:42
  • \$\begingroup\$ Many times you don't need localization, for sure, but it can also be kept in mind that sometimes you need it. Anyway, Java has already a solution for that kind of problems, as I mentioned in my answer. \$\endgroup\$ – user140547 Jun 15 '15 at 14:53
5
\$\begingroup\$

Text manipulation like that is always a pain. I like to extract functions for things like this, though.

private static final String simplePlural (int count, String noun) {
    if (count == 1) {
        return "1 " + noun;
    }
    return count + " " + noun + "s";
}

With that function, your code becomes:

public String toAlbumSongCount() {
    return simplePlural(albumCount(), "album") 
       + ", " + simplePlural(songCount(), "song");
}

I know there's no StringBuilder in there, but the performance benefit for something that's seldom used, will be negligible, and this is more readable.

Note that the code above adds a space after the comma, which your code is missing.

\$\endgroup\$
  • 2
    \$\begingroup\$ and now localize it ;) Your code does not account for a different plural form than s \$\endgroup\$ – Vogel612 Jun 15 '15 at 13:44
  • \$\begingroup\$ There is a StringBuilder there - it's implicit. If you look at the byte code generated, you'll see the creation and destruction of StringBuilders. I would also point out that this code appears to create three StringBuilders (once for each simplePlural invocation, one for the overall), and two additional Strings (which are then left to collection). Consider the object creation / collection when running simplePlural hundreds or thousands of times. \$\endgroup\$ – user22048 Jun 15 '15 at 16:51
5
\$\begingroup\$

If you want to keep the StringBuilder, you use its fluent interface like this to remove some clutter.

    builder.append("first")
       .append("second")
       .toString();

BTW, there is also some support in Java for pluralization and so on, but I have to admit I have never used it because in most cases it is really simpler to use a simple if. But if you want the perfect, maybe a bit overengineered solution for internationalization, you can have a look at ChoiceFormat

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

I would use Java's built-in message formatting method, MessageFormat. It has a means to select different text choices based on a number:

public String toAlbumSongCount() {      
    return MessageFormat.format(
        "{0,number} {0,choice,1#album|1<albums},{1,number} {1,choice,1#song|1<songs}",
        albumCount(), // parameter 0
        songCount()); // parameter 1
}
\$\endgroup\$

protected by rolfl Jun 15 '15 at 13:43

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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