Object Oriented Design of Juke Box

Question

Please review my code for JukeBox

• Jukebox is plays songs or playlists
• Each song has an Artist
• For playlist, the first song is played and other songs are added to the queue
• Relation between playlist and songs is many-to-many
• Relation between Song and Artist is Many-to-one

In particular, I am not sure about the relation between Playlist and Song. Suppose I want to find all the playlists that a particular song is present in..How do I change my design?. Please mention other issues too.

---

 public class Song {
    String songName;
    Artist artist;

    public Song(String songName, Artist artist) {
        this.songName = songName;
        this.artist = artist;
    }

    public void playSong(){
        System.out.println("Playing the song "+songName);
    }

    public String getSongName() {
        return songName;
    }

    public Artist getArtist() {
        return artist;
    }

    @Override
    public String toString() {
        return "Song{" +
                "songName='" + songName + '\'' +
                ", artist=" + artist +
                '}';
    }
  }

---

import java.util.List;

public class PlayList {
    String playlistName;
    List<Song> songsInPlaylist;
    public PlayList(String playlistName,List<Song> songsInPlaylist) {
        this.playlistName = playlistName;
        this.songsInPlaylist = songsInPlaylist;
    }
    public void addSong(Song song){
        songsInPlaylist.add(song);
    }

    public String getPlaylistName() {
        return playlistName;
    }

    public List<Song> getSongsInPlaylist() {
        return songsInPlaylist;
    }
}

---

import java.util.ArrayList;
import java.util.LinkedList;
import java.util.List;
import java.util.Queue;

public class JukeBox {
    List<Song> allSongs;
    List<PlayList> allPlayLists;
    Queue<Song> currentPlayQueue;

    public JukeBox(List<Song> allSongs,List<PlayList> allPlayLists) {
        this.allSongs = allSongs;
        this.allPlayLists = allPlayLists;
        currentPlayQueue = new LinkedList<Song>();
    }

    //Song related methods
    public Song selectSong(String songName){
       Song requiredSong = null;
       for(Song song : allSongs){
           if(song.getSongName().equals(songName)) {
               requiredSong = song;
               break;
           }
       }
       if(requiredSong == null){
           throw new IllegalArgumentException("Provided song not available");
       }
       return requiredSong;
    }


    public void playSong(String songName){
        Song requiredSong = selectSong(songName);
        requiredSong.playSong();
    }

    public void queueNextSong(String songName){
        Song requiredSong = selectSong(songName);
        currentPlayQueue.add(requiredSong);
    }

    //PlayList Related Methods
    public PlayList selectPlayList(String playListName){
        PlayList requriedPlayList = null;
        for(PlayList playList : allPlayLists){
            if(playList.getPlaylistName().equals(playListName)){
                requriedPlayList = playList;
            }
        }
        if(requriedPlayList == null){
            throw new IllegalArgumentException("Provided Playlist not available");
        }
        return requriedPlayList;
    }

    public void playPlayList(String playListName){
       PlayList playList =  selectPlayList(playListName);
       List<Song> allSongsInThePlayList = playList.getSongsInPlaylist();
       //Play First Song
        allSongsInThePlayList.get(0).playSong();
       //Add remaining songs to the queue
        Song currentSong;
        for(int i = 1 ; i < allSongsInThePlayList.size() ; i++){
            currentSong = allSongsInThePlayList.get(i);
            currentPlayQueue.add(currentSong);
        }
    }

}

---

public class Artist {
    String artistName;

    public Artist(String artistName) {
        this.artistName = artistName;
    }

    @Override
    public String toString() {
        return "Artist{" +
                "artistName='" + artistName + '\'' +
                '}';
    }
}

---

import java.util.ArrayList;
import java.util.List;

public class Main {
    public static void main(String[] args) {
        //In memory database of songs and play lists
        List<Song> allSongs = new ArrayList<Song>();
        Song song1 = new Song("Roses",new Artist("David"));
        Song song2 = new Song("Ride",new Artist("21Pilots"));
        allSongs.add(song1);
        allSongs.add(song2);
        //Playlists
        List<PlayList> allPlayLists = new ArrayList<PlayList>();
        List<Song> songsInThePlayList = new ArrayList<Song>();
        songsInThePlayList.add(song2);
        PlayList playList1 = new PlayList("Hello",songsInThePlayList);
        allPlayLists.add(playList1);

        JukeBox jukeBox = new JukeBox(allSongs,allPlayLists);

        jukeBox.playSong("Roses");
        jukeBox.playPlayList("Hello");
    }
}

Answer 1

First off, I really like this Juke Box idea as an OO design project, and it's looking quite good.

Write immutable classes when you can

String songName;
Artist artist;

Can become

private String songName;
private Artist artist;

And your Song class is a perfect candidate for an immutable one, so we can go one step further

private final String songName;
private final Artist artist;

Same goes for the Artist class in terms of immutability.

Naming

In your Song class you have playSong and getSongName methods. These are methods on a Song. there's no real need to specify song in the method name too!

We can simply have

song.play()
song.getName()

It's just as readable (if not more readable) than before!

Don't trust anyone

In your PlayList class, you provide a getter to a mutable object, in this case a list. If you're providing getters for mutable objects like this, you should provide a defensive copy. To make more robust code, it's better to be less trusting of people using your code (including yourself!)

So instead of

return songsInPlaylist;

we can use

return new ArrayList<>(songsInPlaylist)

If you return the member variable songsInPlaylist directly, the caller can do whatever they want with that same instance of the list.

(An alternative is to provide an unmodifiable version of the list. Note that if the objects inside the list are mutable, then you still have things to worry about.)

Similarly, in your PlayList constructor

 public PlayList(String playlistName,List<Song> songsInPlaylist) {
        this.playlistName = playlistName;
        this.songsInPlaylist = songsInPlaylist;
    }

If someone passes in that list, they can add/remove stuff from it and it will effect the list in your playlist object! Try this instead

 public PlayList(String playlistName,List<Song> songsInPlaylist) {
        this.playlistName = playlistName;
        this.songsInPlaylist = new ArrayList<>(songsInPlaylist);
    }

Now they can do whatever they want with the list they passed in, and it won't effect your object at all!

Efficiency

at the moment your selectSong method has O(n) time complexity, because you need to (worst case) iterate through the entire list to find your song. A better data structure to use here would be a Map. This allows you for O(1) or constant time access.

Instead of your JukeBox being backed by a list, you could have it backed by a HashMap. So you could have O(1) access to any song given its name.

Or maybe a Map, or even both.

your selectSong method could look something like this

public Song selectSong(String name){
    if(!songMap.containsKey(name)){
         throw new IllegalArgumentException("Provided song not available");
     }

     return songMap.get(name);
}

(we no longer need a getSongName() method now!)

if you had a Map, your selectPlayList would almost be identical.

In particular, I am not sure about the relation between Playlist and Song. Suppose I want to find all the playlists that a particular song is present in.

If you really wanted to, you could have a Song know about what playlists it's on, but I think that it's perfectly fine to just iterate through the playlists until you find it. I don't think a song should know about what playlist it's on. But a playlist should know what songs are on it.

you could have a method like,

List<PlayList> playLists = jukeBox.getPlaylistsWith(song);

it could maybe look like this

public List<PlayList> getPlaylistsWith(Song song){
     List<PlayList> playLists = new ArrayList<>();
     for(PlayList playList : playListMap.values()){
         if (playList.hasSong(song)){
             playLists.add(playList);
         }
     }
     return playLists;
}

(bonus points for using streams instead :D)

Stylistic Choices

I see that in some places you use a for each loop

 for(PlayList playList : allPlayLists){
        if(playList.getPlaylistName().equals(playListName)){
            requriedPlayList = playList;
        }
 }

And in other places you use

    for(int i = 1 ; i < allSongsInThePlayList.size() ; i++){
        currentSong = allSongsInThePlayList.get(i);
        currentPlayQueue.add(currentSong);
    }

I would stick with one (especially in the same project) just for consistency. My personal preference would be to go with the first one.

Where possible, I would avoid checking for null.

Hopefully this was useful!

Answer 2

You have a Song object, why are you playing songs with "Strings" in Jukebox?

Isn 't it better to play a song as the method suggests? So instead of:

public void playSong(String songName) {
    Song requiredSong = selectSong(songName);
    requiredSong.playSong();
}

Why not?

public void playSong(final Song song) {
    Song requiredSong = selectSong(songName);
    requiredSong.playSong();
}

Because you know, there may be several songs with same name.. So it would be better to override and rely on equals(Object o) in class Song and use it in Jukebox. Songs can be considered equal if name of the songs are same AND the artist is same and maybe even record date is same!

You can make methods such as selectSong private, no need to expose.

And why do not you handle the following situation:

if (requiredSong == null) {
    throw new IllegalArgumentException("Provided song not available");
}

but rather throwing a Runtime Exception? It would be better to throw a checked exception, it is a recoravable situation here. Well, song is not found, flash the red light, show a message to user, give back his/her coin etc.. No need to throw a Runtime Exception, Runtime Exceptions indicate programming errors.

So I would aim for a design as below for class JukeBox:

public class JukeBox {

    private final List<Song> allSongs;
    private final List<PlayList> allPlayLists;
    private final Queue<Song> currentPlayQueue;

    public JukeBox(List<Song> allSongs, List<PlayList> allPlayLists) {
        this.allSongs = allSongs;
        this.allPlayLists = allPlayLists;
        currentPlayQueue = new LinkedList<Song>();
    }

    public void playSong(final Song song) {
        //.. implementation here
    }

    public void playPlayList(final PlayList playList) {
        //.. implementation here
    }

    private Song selectSong(final Song song) {
        // implementation here..
    }

    //PlayList Related Methods
    private PlayList selectPlayList(String playListName) {
        // implementation here..
    }
}

Please note how I stashed private methods at the lower part and did not expose them with public modifier.