Creating and listing a song collection

Question

Songs Class:

public class Songs {
    //-----------------------------------------------------------------
    //  Creates a SongCollection object and adds some songs to it. Prints
    //  reports on the status of the collection.
    //-----------------------------------------------------------------
    public static void main (String[] args)
    {
        SongCollection music = new SongCollection ();

        music.addSong ("Storm Front", "Billy Joel", 14.95, 10);
        music.addSong ("Come On Over", "Shania Twain", 14.95, 16);
        music.addSong ("Soundtrack", "Les Miserables", 17.95, 33);
        music.addSong ("Graceland", "Paul Simon", 13.90, 11);

        System.out.println (music);

        music.addSong ("Double Live", "Garth Brooks", 19.99, 26);
        music.addSong ("Greatest Hits", "Jimmy Buffet", 15.95, 13);

        System.out.println (music);
    }
}

---

Song Collection Class:

import java.text.NumberFormat;

public class SongCollection {
    private final int NUM_SONGS = 100;
    private Song[] collection;
    private int count;
    private double totalCost;

    //-----------------------------------------------------------------
    //  Constructor: Creates an initially empty collection.
    //-----------------------------------------------------------------
    public SongCollection ()
    {
        collection = new Song[NUM_SONGS];
        count = 0;
        totalCost = 0.0;
    }

    //-----------------------------------------------------------------
    //  Adds a song to the collection, increasing the size of the
    //  collection if necessary.
    //-----------------------------------------------------------------
    public void addSong (String title, String artist, double cost, int tracks)
    {
        if (count == collection.length)
            increaseSize();

        collection[count] = new Song (title, artist, cost, tracks);
        totalCost += cost;
        count++;
    }

    //-----------------------------------------------------------------
    //  Returns a report describing the CD collection.
    //-----------------------------------------------------------------
    public String toString()
    {
        NumberFormat fmt = NumberFormat.getCurrencyInstance();

        String report = "~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n";
        report += "My Song Collection\n\n";

        report += "Number of songs: " + count + "\n";
        report += "Total cost: " + fmt.format(totalCost) + "\n";
        report += "Average cost: " + fmt.format(totalCost/count);

        report += "\n\nSong List:\n\n";

        for (int i = 0; i < count; i++) {
            report += collection[i].toString() + "\n";
        }

        return report;
    }

    //-----------------------------------------------------------------
    //  Increases the capacity of the collection by creating a
    //  larger array and copying the existing collection into it.
    //-----------------------------------------------------------------
    private void increaseSize ()
    {
        Song[] temp = new Song[collection.length * 2];

        for (int i = 0; i < collection.length; i++)
            temp[i] = collection[i];

        collection = temp;
    }
}

---

Song Class:

import java.text.NumberFormat;

public class Song {
    private String title, artist;
    private double cost;
    private int tracks;

    //-----------------------------------------------------------------
    //  Creates a new Song with the specified information.
    //-----------------------------------------------------------------
    public Song (String name, String singer, double price, int numTracks)
    {
        title = name;
        artist = singer;
        cost = price;
        tracks = numTracks;
    }

    //-----------------------------------------------------------------
    //  Returns a string description of this song.
    //-----------------------------------------------------------------
    public String toString()
    {
        NumberFormat fmt = NumberFormat.getCurrencyInstance();

        String description;

        description = fmt.format(cost) + "\t" + tracks + "\t";
        description += title + "\t" + artist;

        return description;
    }
}

Output:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
My Song Collection

Number of songs: 6
Total cost: $97.69
Average cost: $16.28

Song List:

$14.95  10  Storm Front Billy Joel
$14.95  16  Come On Over    Shania Twain
$17.95  33  Soundtrack  Les Miserables
$13.90  11  Graceland   Paul Simon
$19.99  26  Double Live Garth Brooks
$15.95  13  Greatest Hits   Jimmy Buffet

I think the formatting of the output could be better and maybe the code could be written in a more cohesive manner to improve readability.

Answer 1

In addition to the valuable answer already given:

• use JavaDoc for comments describing method/class
• do not use float or double for monetary values (calculation inaccuracies!)
• use existing collection implementations of Java, as ArrayList or at least wrap them
• learn the advantages of Single Responsibility Principle (SRP)

Here some improvements on modelling and design:

• use BigDecimal for monetary values and store currency (at least in the names)
• use separate class for storing and calculating summary information (aggregation)
• use separate class for formatting (as everything UI related)

Monetary values

Monetary values (e.g. prices and costs) can be modeled in different, disputed ways in Java:

• using BigDecimal (see also Decimalisation
• using Integer (or int) by representing them as non-fractional numbers by their minor-units (e.g. cents). See also downsides.
• wrapping them in their own types (class) with attached currency-symbol (see Locale)
• using Java's new "Money & Currency API" (JSR-354). See tutorial.

Though for simple cases BigDecimal or even int will do a good job. Simple like single currency (no conversion, single representation) with calculating sums only (no division, tax, etc.).

Aggregation and Summary

Since the total costs or count of songs as well as average price are all aggregated facts of a collection of songs, they should be calculated (technical term: aggregated) separately (see SRP). Together they have the purpose of a "summary".

So extract them in a class SongCollectionSummary which holds all summarized information, and may also do all the calculation/aggregation.

public class SongCollectionSummary {
  private BigDecimal totalCostsInUSD;
  private int songsCount;
  private String description;

  public static summarize(Collection<Song> songs, String description) {
    // use only the factory method to create a summary
    SongCollectionSummary summary = new SongCollectionSummary(description);
    summary.songsCount = songs.size();
    // add each 
    for (Song song : songs) {
      summary.totalCostsInUSD.add(song.getPrice())
    }

    return summary;
  }

  private SongCollectionSummary(String description) {
    this.description = description;
    this.totalCostsInUSD = BigDecimal.ZERO;
    this.songsCount = 0;
  }

  // only getters for the 3 private fields

  public BigDecimal getAveragePriceInUSD() {
    // prevent division by zero
    if (songsCount > 0) {
      return totalCostsInUSD.divide(songsCount);
    }
    // could also throw an exception here, since no songs, no average
    return null;
  }

}

Formatting

This is part of the UI and belongs in a separate class, better in a separate module (pacakge). Responsibility of the class is to build a representation of the domain model (e.g. single Song or collection of songs).

To produce a textual representation there could be a class SongFormatter. This class should allow to format a Song or a collection of many (parameter) and return a String (textual representation).

public class {
  // used with static methods, must also be static
  // a constant named in UPPER_CASE
  public static final NumberFormat CURRENCY_FORMAT = NumberFormat.getCurrencyInstance();

  public static String format(Song song) {
    // using an adjustable template to arrange text
    return String.format("%s\t%d\t%s", CURRENCY_FROMAT.format(song.getPrice()), song.getTackId(), song.getTitle());
  }

  public static String format(Collection<Song> songs, String summaryText) {
    // using a builder to append songs (or summary) dynamically
    StringBuilder sb = new StringBuilder();
    sb.append("Song list: ");
    // helding the summary separately and inject customisable as parameter
    sb.append(summaryText);
    // format each song by calling dedicated method (if collection empty, end here)
    for (Song song : songs) {
      sb.append("\n").append(format(song));
    }

    return sb.toString();
  }

}

You will find some missing spots above:

• summaryText needs to be formatted by a separated class or method that may use calculated SongCollectionSummary (as explained before)
• if collection is empty, there will a line Song list: without any songs following, hence take care when calling the method and passing an empty ArrayList of songs (e.g. adjust summaryText to "[EMPTY] No songs added!"

Hope you will discover some benefits and make use valuable parts to improve your design.

Answer 2

Code Style

Use the standard JavaDoc markup when writing comments. You will be able to generate documentation and it allows you to use ready made tools for documenting stuff like method parameters and related classes. Also people expect to see JavaDoc style comments and are used to reading them, so by rolling out your own style you're adding unnecessary cognitive load to the reader.

/**
 * Creates a new Song with the specified information.
 *
 * @param name Song name
 * @param singer Singer name
 * @param price Song price, in USD.
 * @param numTracks I have no idea why a song would have several tracks. :)
 */
public Song(String name, String singer, double price, int numTracks) {

Maybe you meant the numTracks parameter to be trackNumber? But is it a track number in a recording the song was in or is it a sorting number in your collection? The name should describe the purpose of the parameter more accurately.

Fields that are not meant to be changed should be final. I would rather see each field on their own line.

private final String title;
private final String artist;

You should not use float or double types for currency. I would also rather attach the currency code to any monetary value too, but that's probably just me working in the banking sector.

/**
 * Song cost in cents (USD).
 */
private int cost;

When initializing fields in a constructor, you should use the same names for the constructor parameters as you have in the fields. That way the reader does not have to wonder what parameter goes to which field and you donä't have to spend time trying to figure out synonyms. And in case you were wondering, using "this." in this context is not code clutter.

public Song(String title, String artist, int cost, int tracks) {
    this.title = title;
    this.artist = artist;
    this.cost = cost;
    this.tracks = numTracks;
}

You are creating your own list data type in the SongCollection class. The class is now responsible for both being a song collection and implementing an extensible list data type. If you want to reinvent the wheel, you should extract the list data type into a separate class to maintain single responsibility principle. Otherwise just use the java.util.ArrayList class.

public class SongCollection {
    private List<Song> collection = new ArrayList<>();

You're duplicating the Song constructor in the SongCollection class by requiring the user to pass all the same parameters to addSong(...) as you have in the constructor for Song. Instead, have addSong accept just a Song object.

public void addSong(Song song) {
    ...

Song Formatting

The toString() method is usually nothing more than a debugging aid. As the JavaDoc says: the result should be a concise but informative representation that is easy for a person to read. You're placing the whole complex formatting logic into the toString method, which again breaks the single responsibility principle and makes the class hard to maintain (the SongCollection class has three responsibilities at the moment). Instead you should encapsulate the formatting into a separate SongFormatter and SongCollectionFormatter classes. The topic is also fairly complex, so it's probably worth another post once you get it done.