Object Oriented Design of Juke Box

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 songsInPlaylist;
    public PlayList(String playlistName,List songsInPlaylist) {
        this.playlistName = playlistName;
        this.songsInPlaylist = songsInPlaylist;
    }
    public void addSong(Song song){
        songsInPlaylist.add(song);
    }

    public String getPlaylistName() {
        return playlistName;
    }

    public List getSongsInPlaylist() {
        return songsInPlaylist;
    }
}

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

public class JukeBox {
List allSongs;
List allPlayLists;
Queue currentPlayQueue;

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

//

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 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 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 playLists = jukeBox.getPlaylistsWith(song);

it could maybe look like this

public List getPlaylistsWith(Song song){
     List 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!

StackExchange Code Review Q#161859, answer score: 2