3
\$\begingroup\$

This is a simple Java application, which adds songs to a playlist, removes songs from playlist based on indexes. A database is not used in this. I am creating dummy data through a Utility class. I want to get feedback on design and code, so that I can refactor it.

MusicApp main class, creates a random playlist. Adds/Removes Tracks from Playlist.

public class MusicApp {
    public static void main(String[] args) {

        PlaylistBusinessBean app = new PlaylistBusinessBean(new PlaylistDaoBean(new PlaylistUtils()),
                new PlaylistUtils());

        /* dummy tracks to add into Playlist */
        List<Track> tracksToAdd = new ArrayList<Track>();
        for (int i = 0; i < 20; i++) {
            tracksToAdd.add(PlaylistUtils.getTrack());
        }

        int toIndex = 10;

        String playlistUUID = UUID.randomUUID().toString();
        app.addTracks(playlistUUID, tracksToAdd, toIndex);
        System.out.println("Tracks added to playlist: " + playlistUUID);

        System.out.println("Fetching playlist uuid: " + playlistUUID);
        Playlist list = app.getPlaylist(playlistUUID);
        System.out.println("No. of tracks: " + list.getNrOfTracks());

        System.out.println("Removing songs from playlist:...");
        List<Integer> indexes = Arrays.asList(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
        app.removeTracks(playlistUUID, indexes);
        System.out.println("Playlist updated...");
        System.out.println("No. of tracks after removing: " + list.getNrOfTracks());

    }
}

PlaylistBusinessBean is a service class, provides implementation to add/remove tracks from Playlist.

public class PlaylistBusinessBean {

    private PlaylistDaoBean playlistDaoBean;

    private PlaylistUtils playlistUtils;

    @Inject
    public PlaylistBusinessBean(PlaylistDaoBean playlistDaoBean, PlaylistUtils playlistUtils) {
        this.playlistDaoBean = playlistDaoBean;
        this.playlistUtils = playlistUtils;
    }

    /**
     * Add tracks to the index
     * 
     * @param uuid
     * @param tracksToAdd
     * @param toIndex
     * @return
     * @throws PlaylistException
     */
    public List<PlaylistTrack> addTracks(String uuid, List<Track> tracksToAdd, int toIndex) throws PlaylistException {

        if (StringUtils.isNotBlank(uuid) && CollectionUtils.isNotEmpty(tracksToAdd)) {

            try {

                Playlist playList = playlistDaoBean.getPlaylistByUUID(uuid);

                if (playList != null) {

                    // We do not allow > 500 tracks in new playlists
                    if (playList.getNrOfTracks() + tracksToAdd.size() > 500) {
                        throw new PlaylistException("Playlist cannot have more than " + 500 + " tracks");
                    }

                    // The index is out of bounds, put it in the end of the list.
                    int size = playList.getPlayListTracks() == null ? 0 : playList.getPlayListTracks().size();
                    if (toIndex > size || toIndex == -1) {
                        toIndex = size;
                    }

                    if (!(playlistUtils.validateIndexes(toIndex, playList.getNrOfTracks()))) {
                        return Collections.EMPTY_LIST;
                    }

                    List<PlaylistTrack> original;
                    Set<PlaylistTrack> originalSet = playList.getPlayListTracks();

                    if (originalSet == null || originalSet.size() == 0) {
                        original = new ArrayList<PlaylistTrack>();
                    } else {
                        original = new ArrayList<PlaylistTrack>(originalSet);
                        Collections.sort(original);
                    }
                    List<PlaylistTrack> added = new ArrayList<PlaylistTrack>(tracksToAdd.size());

                    for (Track track : tracksToAdd) {

                        PlaylistTrack playlistTrack = new PlaylistTrack();
                        playlistTrack.setId(track.getId());
                        playlistTrack.setTrackPlaylist(playList);
                        playlistTrack.setIndex(toIndex + 1);
                        playlistTrack.setDateAdded(new Date());
                        playlistTrack.setTrackId(track.getId());
                        playlistTrack.setTrack(track);

                        playList.setDuration(playlistUtils.addTrackDurationToPlaylist(playList, track));
                        original.add(toIndex, playlistTrack);
                        added.add(playlistTrack);
                        toIndex++;
                    }

                    setPlaylistTracks(playList, original);
                    playlistDaoBean.savePlaylistByUUID(uuid, playList);
                    return added;
                } else {
                    throw new PlaylistException("Playlist not found error");
                }
            } catch (Exception e) {
                e.printStackTrace();
                throw new PlaylistException("Generic error");
            }
        } else {
            throw new PlaylistException("Bad input data error");
        }
    }

    /**
     * Remove the tracks from the playlist located at the sent indexes
     * 
     * @param uuid
     * @param indexes
     * @return
     * @throws PlaylistException
     */
    public List<PlaylistTrack> removeTracks(String uuid, List<Integer> indexes) throws PlaylistException {
        // TODO
        if (StringUtils.isNotBlank(uuid) && CollectionUtils.isNotEmpty(indexes)) {
            try {

                Playlist playList = playlistDaoBean.getPlaylistByUUID(uuid);

                if (playList != null) {

                    List<PlaylistTrack> original;
                    Set<PlaylistTrack> originalSet = playList.getPlayListTracks();

                    if (originalSet == null || originalSet.size() == 0) {
                        original = new ArrayList<PlaylistTrack>();
                    } else {
                        original = new ArrayList<PlaylistTrack>(originalSet);
                        Collections.sort(original);
                    }
                    List<PlaylistTrack> removed = new ArrayList<PlaylistTrack>(indexes.size());

                    for (int index : indexes) {

                        if (index >= 0 && index <= original.size()) {
                            original.removeIf(track -> index == track.getIndex());
                        }
                    }

                    setPlaylistTracks(playList, original);
                    playlistDaoBean.savePlaylistByUUID(uuid, playList);
                    return removed;
                }

            } catch (Exception e) {
                e.printStackTrace();
                throw new PlaylistException("Generic error");
            }
        }
        return Collections.EMPTY_LIST;
    }

    /**
     * 
     * @param uuid
     * @return
     */
    public Playlist getPlaylist(String uuid) {
        if (StringUtils.isEmpty(uuid)) {
            throw new PlaylistException("Bad UUID input");
        } else {
            Optional<Playlist> playlist = Optional.ofNullable(playlistDaoBean.getPlaylistByUUID(uuid));

            if (playlist.isPresent()) {
                return playlist.get();
            } else {
                throw new PlaylistException("Playlist not found");
            }
        }
    }

    /**
     * This method set new index in updated {@link List} of {@link PlaylistTrack}
     * and also add {@link List} of {@link PlaylistTrack} into original
     * {@link Playlist}
     * 
     * @param playList
     * @param original
     */
    private void setPlaylistTracks(Playlist playList, List<PlaylistTrack> original) {

        int i = 0;
        for (PlaylistTrack track : original) {
            track.setIndex(i++);
        }

        playList.getPlayListTracks().clear();
        playList.getPlayListTracks().addAll(original);
        playList.setNrOfTracks(original.size());
    }

}

PlaylistDaoService defines DAO layer methods

public interface PlaylistDaoService {

    public void savePlaylistByUUID(String uuid, Playlist newPlaylist);

    public Playlist getPlaylistByUUID(String uuid);

}

PlaylistDaoBean provides implementation to DAO layer methods.

/**
 * This class provides implementation to {@link PlaylistDaoService} methods
 * 
 * @author root
 *
 */
public class PlaylistDaoBean implements PlaylistDaoService {

    private final Map<String, Playlist> playlists = new HashMap<String, Playlist>();

    private PlaylistUtils playlistUtils;

    @Inject
    public PlaylistDaoBean(PlaylistUtils playlistUtils) {
        this.playlistUtils = playlistUtils;
    }

    @Override
    public Playlist getPlaylistByUUID(String uuid) {

        return Optional.ofNullable(playlists.get(uuid)).orElse(playlistUtils.createPlaylist(uuid));
    }

    @Override
    public void savePlaylistByUUID(String uuid, Playlist newPlaylist) {

        playlists.put(uuid, newPlaylist);
    }

}


PlaylistUtils contains utility methods to support playlist operations

/**
 * 
 * This utility class creates dummy {@link Track}, add them to
 * {@link PlaylistTrack} and return the fake {@link Playlist}. And, also
 * provides utility methods for validation and other tasks
 * 
 * @author root
 *
 */
public class PlaylistUtils {

    private static ArrayList<Integer> numberList = new ArrayList<Integer>();

    private static int nrOfTracks = 1;

    public Playlist createPlaylist(String uuid) {

        Playlist trackPlayList = new Playlist();

        trackPlayList.setDeleted(false);
        trackPlayList.setDuration((float) (60 * 60 * 2));
        trackPlayList.setId(49834);
        trackPlayList.setLastUpdated(new Date());
        trackPlayList.setNrOfTracks(nrOfTracks);
        trackPlayList.setPlayListName("Collection of great songs");
        trackPlayList.setPlayListTracks(getPlaylistTracks());
        trackPlayList.setUuid(uuid);

        return trackPlayList;
    }

    private static Set<PlaylistTrack> getPlaylistTracks() {

        Set<PlaylistTrack> playListTracks = new HashSet<PlaylistTrack>();

        for (int i = 0; i < nrOfTracks; i++) {
            PlaylistTrack playListTrack = new PlaylistTrack();
            playListTrack.setDateAdded(new Date());
            playListTrack.setId(i + 1);
            playListTrack.setIndex(i);
            playListTrack.setTrack(getTrack());
            playListTrack.setTrackId(playListTrack.getTrack().getId());
            playListTracks.add(playListTrack);
        }

        return playListTracks;
    }

    public static Track getTrack() {

        Track track = new Track();
        Random randomGenerator = new Random();

        while (track.getId() == 0) {

            int num = randomGenerator.nextInt(500);

            if (!numberList.contains(num)) {
                numberList.add(num);
                track.setId(num);
            }
        }

        track.setTitle("Track no: " + track.getId());
        track.setArtistId(randomGenerator.nextInt(10000));
        track.setDuration(60 * 3);

        return track;
    }

    public boolean validateIndexes(int toIndex, int length) {
        return toIndex >= 0 && toIndex <= length;
    }

    public float addTrackDurationToPlaylist(Playlist playList, Track track) {
        return (track != null ? track.getDuration() : 0)
                + (playList != null && playList.getDuration() != null ? playList.getDuration() : 0);
    }
}

PlaylistException exception class

public class PlaylistException extends RuntimeException {

    private static final long serialVersionUID = 759495431208011733L;

    public PlaylistException(String s) {
        super(s);
    }
}

Playlist POJO

public class Playlist {

    private Integer id;
    private String playListName;
    private Set<PlaylistTrack> playListTracks = new HashSet<PlaylistTrack>();
    private Date registeredDate;
    private Date lastUpdated;
    private String uuid;
    private int nrOfTracks;
    private boolean deleted;
    private Float duration;

    public Playlist() {
        this.uuid = UUID.randomUUID().toString();
        Date d = new Date();
        this.registeredDate = d;
        this.lastUpdated = d;
        this.playListTracks = new HashSet<PlaylistTrack>();
    }

...getter setter...
}

PlaylistTrack POJO

public class PlaylistTrack implements Serializable, Comparable<PlaylistTrack> {

    private static final long serialVersionUID = 5464240796158432162L;

    private Integer id;
    private Playlist playlist;
    private int index;
    private Date dateAdded;
    private int trackId;
    private Track track;

    public PlaylistTrack() {
        dateAdded = new Date();
    }

...getter setter...
}

Track POJO

public class Track {

    private String title;
    private float duration;
    private int artistId;
    private int id;

    public Track() {
    }

...getter setter...
}
```
\$\endgroup\$
3
\$\begingroup\$

Overall it looks like your code should work. Some of the things I'm going to say here are mere nitpicking. Other things should really be done differently if you're planning to write code professionally. Here's my ideas in no particular order (other than the order I thought of them while looking at your code):

POJO with default constructor + setter for everything

There's really no good reason to do this ... ever (except if a certain framework forces you to provide it this way and someone else is forcing you to use said framework, see also my last point "bean overkill").

Immutability is a nice property to have for data objects. Java objects should only be mutable if it makes sense for them to be changed. A good example is your playlist that allows adding or removing tracks. It makes sense to do so on a playlist. What doesn't make sense is changing a title or worse the ID of a track. The easiest way to fix this is to require the unchangeable fields as parameters during construction and remove the setters.

public class Track {

    private final String title;
    private final float duration;
    private final int artistId;
    private final int id;

    public Track(String title, float duration, int artistId, int id) {
        this.title = title;
        this.duration = duration;
        this.artistId = artistId
        this.id = id;
    }

    ... only getters here, no setters!
}

PlaylistTrack fields ???

Other than making this immutable similar to Track example in my previous point, this class has some questionable fields. The most obvious one is the trackId. You can remove this field entirely and modify the getter as follows to not lose the functionality:

getTrackId() {
    return track.getId();
}

When would you ever get the playlist from a playlisttrack? Isn't this class meant to be used inside a playlist to hold extra information like when it was added to that list? The rest of your application should be dealing with the playlist directly and get tracks from that playlist, not the other way around. I suggest just removing this field entirely along with the getter (and setter).

The index is another weird field. I would expect a playlist to have some list of PlaylistTracks. That list inherently has an index for each of the tracks. What happens if we remove a certain track from that list? Are you going to update all the PlaylistTracks after it to keep their index field up to date? That sounds like a lot of work that you're most likely going to forget with hard bugs to find later on. Remove this field as well.

The new constructor could look like this:

public PlaylistTrack (int id, Track track) {
    this.id = id;
    this.track = track;
    this.date = now(); //notice how this isn't passed in as parameter since it's always "now"
}

Note that your only example usage of this class has the id the same as trackId if this is always going to be the case you might as well remove this id entirely too.

Do you really need to know when a track was added to a playlist? Most music apps I've used don't provide this functionality. This is a design choice for your app that I'm not going to make for you. If you think it's not really needed afterall, you might as well just remove the PlaylistTrack class and use Tracks directly instead.

null handling

You have a lot of if(something == null) statements in your code. Most of those can be removed if you pass in a meaningful result instead of null. The most obvious one is when you request a list of tracks. If a playlist is empty, this should just return an empty (not null!) list instead. That way you can simplify thise piece of code:

List<PlaylistTrack> original;
Set<PlaylistTrack> originalSet = playList.getPlayListTracks();

if (originalSet == null || originalSet.size() == 0) {
    original = new ArrayList<PlaylistTrack>();
} else {
    original = new ArrayList<PlaylistTrack>(originalSet);
    Collections.sort(original);
}
List<PlaylistTrack> added = new ArrayList<PlaylistTrack>(tracksToAdd.size());

to this:

List<PlaylistTrack> original = new ArrayList<PlaylistTrack>();
original.addAll(playList.getPlayListTracks());
Collections.sort(original);
//note, you can sort an empty list, this doesn't matter much performance wise

playList.setDuration(playlistUtils.addTrackDurationToPlaylist(playList, track));

This is just asking for trouble. Someone else using your playlist class is really likely to forget using this setter. A far better way to update the duration of a playlist is inside the Playlist class. More specifically when you add or remove a track from this playlist.

public void addTrack (PlaylistTrack track, int index) {
    hmmm ...

Now I noticed why your PlaylistTrack has an index field. Your playlist itself is keeping the tracks in an unordered set. This seems like an odd design choice. Any time you want to just play the tracks in order you'll have to get the tracks from the playlist, put them into a different data structure and order them. Why can't the playlist be responsible for providing some ordering of the tracks?

public class PlayList { 
    ...
    private List<PlaylistTrack> tracks = new ArrayList<>();
    ...

    public void addTrack(PlaylistTrack track) {
        tracks.add(track);
        duration += track.getDuration();
    }

    /**
    * inserts the track at a given position in the playlist.
    * if the position is larger than the number of tracks currently in the playlist
    * it will be added at the back of the playlist instead.
    */
    public void addTrack(PlaylistTrack track, int position) {
        if(position > tracks.size()-1) {
            position = tracks.size()-1;
        }
        if(position < 0) {
            position = 0;
        }
        tracks.add(position, track);
        duration += track.duration();
    }

    public void removeTrack(int index) {
        duration -= tracks.get(index).getDuration(); //TODO handle out of bounds error?
        tracks.remove(index);
    }

guard clause

Instead of writing

if(some precondition ) {
    big code block
} else {
    throw error
}

It's also possible to write it as follows

if ( ! precondition ) {
    throw error
}
big code block

This form of checking preconditions first and handling the odd cases is called a "guard clause". It's preferable because you don't have to keep the special cases in your head while reading through the big code block. Because they're handled first and consise it's a lot less mentaly draining to read the code while looking for bugs. Another minor added advantage is that the big code block doesn't need that extra indentation. This becomes more usefull the more special cases you have to handle first.

runtime exceptions

A runtime exception should only ever be used if it no longer makes sense to keep using the application. For a music app, this could be when you can't play sounds because there's no speaker available. Or if you can't access the file system to load in the songs. In such cases the user should be notified that there is a major problem before shutting down the app.

If on the other hand there's a minor UI bug that causes a song to be removed twice from the list, it would be really anoying if this causes the entire app to shut down. A much nicer behaviour would be to give a small popup saying you're trying to remove a track that isn't in the playlist (it's still a bug, so should be fixed) and allow the user to still continue listening to songs when he ignores that popup.

The problem with runtime exceptions is that programmers using your classes don't know (or just forgot) to check for these exceptions. It's better to make them excplicit so the compiler will complain for you if you forget to check for those.

Alternatively, you can try to do the next best thing instead. Like in my example earlier where adding an track to an index that is too big, would just add the track at the end of the list instead of throwing an error. It's often a good idea to add these kinds of decisions into a comment so other programmers who use your class will know what to expect without reading through the entire implementation.

PlaylistUtils

On the one hand I like specific utility classes (unlike some puritarian coders). But they should make sense. Your PlaylistUtils class is questionable at best.

The index validity check should just be placed inside the Playlist class. This is an implementation detail of said class that might change when said implementation changes (like in my example of dealing with the too large index).

addTrackDurationToPlaylist should be removed (see earlier).

What should I expect the getTrack() method to do? Is it fetching some random track? Is it createing a new track (should be called create instead of get)? Is it a valid track that can be used in actual production code outside of testing?

The main issue I think is that you're using this class for testing purposes (which in itself isn't a bad thing) but also for actual checks in your production code (which makes the testing part a problem). With the production methods removed as I suggested this problem is mostly fixed, but the class should be renamed to make it obvious it's meant for testing purposes only. That way it doesn't really matter too much that the code inside doesn't make sense (you're creating invalid playlists!).

bean overkill

Your playlist app looks like it would mainly be used as a standalone, batteries-included application. This means you should keep it as simple as possible and follow the normal java conventions. It doesn't read like any typical parse to JSON to communicate with other applications backend kind of app.

To me this implies that you really shouldn't be using any frameworks that require you to provide beans/POJO's that have a default constructor and setters for everything. Those are useful if you're writing a backend microservice that takes in a non-java specific format (JSON for example) and needs an easy way to parse that into java, do something with it and pass it on to the next microservice or respond with some universal parsable (again JSON?) format.

What you want is easy to maintain code that you can extend without worrying too much about someone else using it in an unintended way. To achieve this, immutability, required fields in constructor, explicit error handling and other best java practices are your friend.

As a final remark I'd like to point out 2 principles

  • YAGNI which tells you to only write code that you actually need, when you need it. Don't write code that might be useful in the future (because it rarely is).
  • nobody will reuse code they don't understand. This one is my own that i've learned from experience (and that many others have figured out as well). If there exists 20 frameworks to deal with a certain problem but they're all hard to understand/use, then the next developper that runs into said problem will design "a better framework" to replace those! Now there are 21 frameworks that are all hard to understand/use.
| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

This is a partial review, since I went over only some of the classes/methods

PlaylistTrack

there are several problems with this class:

  1. Copy of properties
    if the class already holds a reference to Track, why does it hold a copy of the track's id in a separate property? it requires you to maintain that property via a separate setter. you can access the same info from the track instance variable.

  2. dateAdded
    Don't use the obsolete date/time classes from java.util. use the newer java.time package. In your case, you probably want to define dateAdded with type LocalDateTime and assign it the returned value of LocalDateTime.now().

  3. playlist
    I question the necessity of this reference. It seems to me like a direct translation of DB many-to-many model to Java. However, this relation is represented differently in Java, since we have the construct of collections. usually items inside a collection do not maintain a reference to the POJO that holds the collection, since it is not needed: when you access a collection of PlaylistTrack it is through a reference to a particular PLaylist. If you want to know which playlists have a particular track, you would maintain a collection of playlists in the Track class - this is how many-to-many relation is represented in Java - two independent one-to-many relations. I also do not understand why addTracks() returns a list of PlaylistTrack. the method is a special case of setter method, and we know what is the proper return value of this type of method.

  4. setters vs constructor
    instances of PlaylistTrack class are initialized by calling multiple setter methods. this is an error prone process. what if you forgot to set a property? moreover, dateAdded is initialized to a default now() value that is not dependent on the track. Having an "all args" constructor forces the caller to supply values to all the properties that rely on external values. the constructor should NOT require a value for dateAdded as it can initialize this property internally. If you fell like the constructor contains too many args, or that some properties can have default values, use the Builder pattern to supply callers with method-chaining instance creation.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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