5
\$\begingroup\$

After reading about design patterns in general, I decided to try and incorporate them in an actual program. I chose a State machine for my first exercise, or, more concretely, an MP3 player. The player itself does not play any audio files, it just models the behavior of one.

The MP3Player behavior is as follows:

  • If the play button is pressed, play current song, if no song is currently playing
  • If the pause button is pressed:
    • If song is already paused, reset current to first song in the list
    • If songs are already reset, do nothing
    • Else, pause current song
  • If the forward button is pressed, pause song and move current to next in the list
  • If the rewind button is pressed, pause song and move current to the previous in the list

I'd like feedback on:

  • Correct implementation of State and Prototype patterns
  • Whether those two patterns are a right approach in this particular case

I have a State interface that includes all methods, and an AbstractState with the initial implementation of those methods.


State.java

interface State {
    public void play(MP3Player player);
    public void pause(MP3Player player);
    public void forward(MP3Player player);
    public void rewind(MP3Player player);
}

StateCache.java

class StateCache {
    private static HashMap<String, AbstractState> states = new HashMap<>();

    public static void loadCache() {        
        PlayState playState = new PlayState();
        states.put("PlayState", playState);

        PausedState pausedState = new PausedState();
        states.put("PausedState", pausedState);

        ResetState resetState = new ResetState();
        states.put("ResetState", resetState);
    }

    public static AbstractState getState(String shapeID) {
        AbstractState cachedState = states.get(shapeID);
        return cachedState.clone();
    }
}

AbstractState.java

abstract class AbstractState implements State, Cloneable {
    static {
        StateCache.loadCache();
    }

    @Override
    public abstract void play(MP3Player player);

    @Override
    public abstract void pause(MP3Player player);

    @Override
    public void forward(MP3Player player) {
        System.out.println("Forward...");
        player.increaseCurrent();

    }

    @Override
    public void rewind(MP3Player player) {
        System.out.println("Reward");
        player.decreaseCurrent();
    }

    @Override
    public AbstractState clone() {
        AbstractState result = null;
        try {
            result = (AbstractState) super.clone();
        }
        catch (CloneNotSupportedException e) {
            e.printStackTrace();
        }

        return result;
    }

}

PlayState.java

class PlayState extends AbstractState {

    @Override
    public void play(MP3Player player) {
        System.out.println("Song is already playing");
    }

    @Override
    public void pause(MP3Player player) {
        System.out.println(String.format("Song %d is paused", player.current));
        player.setState(StateCache.getState("PausedState"));
    }

    @Override
    public void forward(MP3Player player) {
        super.forward(player);
        player.setState(StateCache.getState("PausedState"));
    }

    @Override
    public void rewind(MP3Player player) {
        super.rewind(player);
        player.setState(StateCache.getState("PausedState"));
    }

}

PausedState.java

class PausedState extends AbstractState {

    @Override
    public void play(MP3Player player) {
        System.out.println(String.format("Song %d is playing", player.current));
        player.setState(new PlayState());
    }

    @Override
    public void pause(MP3Player player) {
        System.out.println("Songs are stopped");
        player.current = 0;
        player.setState(StateCache.getState("ResetState"));
    }

}

ResetState.java

class ResetState extends PausedState {

    @Override
    public void pause(MP3Player player) {
        System.out.println("Songs are already stopped");
    }
}

Song.java

public class Song {
    private String title;
    private String artist;

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

    public String title() {
        return title;
    }

    public String artist() {
        return artist;
    }

    @Override
    public String toString() {
        StringBuilder result = new StringBuilder();
        result.append(String.format("Song{title=%s, artist=%s}", title, artist));
        return result.toString();
    }
}

MP3Player.java

public final class MP3Player {
    private List<Song> songs;
    private State currentState;
    int current;


    protected void setState(State newState) {
        currentState = newState;
    }

    protected void increaseCurrent() {
        current++;
        if (current >= songs.size())
            current -= songs.size();
    }

    protected void decreaseCurrent() {
        current--;
        if (current < 0)
            current += songs.size();
    }

    public MP3Player(List<Song> songs) {
        this.songs = new ArrayList<>(songs);
        current = 0;
        currentState = new PausedState();
    }

    public void printCurrentSong() {
        System.out.println(songs.get(current));
    }

    public void pressPlay() {
        currentState.play(this);
    }

    public void pressPause() {
        currentState.pause(this);
    }

    public void pressFWD() {
        currentState.forward(this);
    }

    public void pressREW() {
        currentState.rewind(this);
    }

    @Override
    public String toString() {
        StringBuilder result = new StringBuilder();
        result.append(String.format("MP3Player{currentSong = %d, songList = %s}", current, songs.toString()));
        return result.toString();
    }
}
\$\endgroup\$
1
\$\begingroup\$

My annotations:

  1. Remove the interface State. It's not necessary. You already have an abstraction with "AbstractState"
  2. ResetState should not derive from PausedState they have nothing to do with each other. Inheritance is not for reducing code redundancy. It's modelling semantic. Maybe you mean that AFTER you made a Reset (go into ResetState) you are going to the PauseState.
  3. The same with the methods in AbstractState that were involved in the state machine. pull them down to the concrete states. Code is not necessarily redundant if it looks the same. In the AbstractState the methods involved in state-change should at most implemented empty.
  4. Remove all the super-calls
  5. Pass the Mp3Player through the constructor to the states and not through the methods
  6. As you are using the flyweight pattern right now for your states keep them immutable in the future.
  7. As you are using the flyweight pattern no prototype pattern is neccessary. Why would you create something out of a prototype if the result will always remain like the prototype?
  8. remove static initializer static {...} and introduce lazy initialization in your StateCache. As you have no access to the Mp3Player in the StateCache do the caching in the Mp3Player. The holder object and the state objects are high cohesional so do not pull them apart. You should not let other objects than the holder object or other states create them

Nevertheless: In this use case (an Mp3 Player) I would not cache any state object. They can be instantiated on demand. If you have a user of your Mp3Player who is clicking the Buttons million times a second we can talk about another solution.

But overall a good first implementation.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks. I was concerned about repetition of code, because I can't tell when it is acceptable or not. If I'm removing one abstraction, which one is more appropriate to remove, the interface or the abstract class? What are the differences between passing the player in the constructor and passing it via the method calls? \$\endgroup\$ Dec 19 '15 at 21:22
  • \$\begingroup\$ Code repetition is really an issue if it violates the single responsible principle (SRP). Because violating the SRP causes problems. There are four situations to consider: You may have no code repetition and violate the SRP. You may have code repetition and violate SRP. You may have no code repetition and do not violate the SRP. You may have code repetition and do not violate SRP. The problem with it is to identify the correct situation. This is possible by a semantic test. I would make up an artificial requirement to the repeated code and see if I would expect the other to be independent. \$\endgroup\$
    – oopexpert
    Dec 19 '15 at 21:24
  • \$\begingroup\$ The difference between passing Mp3Player through the constructor is to introduce OO with more state. Passing Mp3Player as a parameter is more functional with less state. But the standard state pattern is formulated for the OO world. One other clue that makes me doing more analysis is that you have a bunch of methods, that only differ in their names. That is a strong indication that the object passed as a parameter is the responsible objects for the implementations of this interface. \$\endgroup\$
    – oopexpert
    Dec 19 '15 at 21:38

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.