Sharing my Memento Exercise

Design Patterns
1

Hello everyone!
I have a slightly different implementation than Mosh’s and wondering if anyone can give me feedback on mine? So what I did was I am treating Document to hold current state and only add prevStates to the Caretaker class. With Mosh’s implementation, it feels as though we are saving the current state to the Caretaker (History) class so when we first call restoreState(), it’s actually restoring the current state which confused me…

Document Class

public class Document {
    private String content;
    private String fontName;
    private int fontSize;

    public Document() {
        /**
         * sets the default value when a new document is created to represent the real world
         * document where initially the document is empty
         **/
        this.content = "";
        this.fontName = "Arial";
        this.fontSize = 12;
    }

    // restoring the internal state of Document to the previous state
    public void undo(DocumentState docState) {
        this.content = docState.getContent();
        this.fontName = docState.getFontName();
        this.fontSize = docState.getFontSize();
    }

    public String getContent() {
        return content;
    }

    public DocumentState setContent(String content) {
        DocumentState state = new DocumentState(this);
        this.content = content;
        return state;
    }

    public String getFontName() {
        return fontName;
    }

    public DocumentState setFontName(String fontName) {
        DocumentState state = new DocumentState(this);
        this.fontName = fontName;
        return state;

    }

    public int getFontSize() {
        return fontSize;
    }

    public DocumentState setFontSize(int fontSize) {
        DocumentState state = new DocumentState(this);
        this.fontSize= fontSize;
        return state;
    }

    @Override
    public String toString() {
        return "Document{" +
                "content='" + content + '\'' +
                ", fontName='" + fontName + '\'' +
                ", fontSize=" + fontSize +
                '}';
    }

    public void printCurrentState() {
        System.out.println(this.toString());
    }
}

DocumentState (Memento)

public class DocumentState {

    private String content;
    private String fontName;
    private int fontSize;

    public DocumentState(String content, String fontName, int fontSize) {
        this.content = content;
        this.fontName = fontName;
        this.fontSize = fontSize;
    }

    public DocumentState(Document doc) {
        this.content = doc.getContent();
        this.fontName = doc.getFontName();
        this.fontSize = doc.getFontSize();
    }

    public String getContent() {
        return content;
    }

    public String getFontName() {
        return fontName;
    }

    public int getFontSize() {
        return fontSize;
    }

}

StateManager (Caretaker)


public class StateManager {
    private Stack<DocumentState> states = new Stack<>();

    public void push(DocumentState state) {
        states.add(state);
    }

    public DocumentState pop() {
        DocumentState prevState = states.peek();
        states.pop();
        return prevState;
    }
}

Driver class:

package com.codewithmosh.memento;

public class Driver {
    public static void main(String[] args) {
        Document document = new Document();
        StateManager history = new StateManager();

        history.push(document.setContent("hello how are you?"));
        history.push(document.setFontName("calibri"));

        document.printCurrentState();

        document.undo(history.pop());

        document.printCurrentState();

        document.undo(history.pop());

        document.printCurrentState();
    }
}
2

Not sure what sort of feedback you were looking for. The implementation seems fine, but there are some mild things I could nitpick.

You have a circular relationship between Document and DocumentState that seems unnecessary - there is no need for DocumentState to depend on Document, just pass the parameters to its constructor (or if there were many, I would recommend using the Builder pattern to fix that).

Some additional minor details:

  • DocumentState’s fields should be final so that it is clear this is an immutable object
  • Stack.pop() returns the item that was removed from the stack so you could replace your current implementation with a one liner: return states.pop();
  • Not that Mosh fixed this, but the current implementation does not handle the empty history case (nor does it provide visibility to check if the stack is empty).

Personally, I would rather have the Document class compose the History class so that it can directly push and pop from history in the appropriate methods. It would also allow us to have an undo() method without parameters which is pretty nice to have. It would also mean you do not need to clutter the API with methods that return DocumentState for no reason other than to allow it to get passed to the StateManagement class.

Cheers!