Memento pattern drawbacks

So, here is an typical implementation of Memento pattern (skipped getters and setters).

public class Employee {
    private String name;
    private String phone;

    public EmployeeMemento save() {
        return new EmployeeMemento(name, phone);
    }

    public void revert(EmployeeMemento memento) {
        this.name = memento.getName();
        this.phone = memento.getPhone();
    }
}


  public class EmployeeMemento {
        private final String name;
        private final String phone;

        public EmployeeMemento(String name, String phone) {
            this.name = name;
            this.phone = phone;
        }
    }

public class Caretaker {
    private Stack<EmployeeMemento> history;

    public Caretaker() {
        history = new Stack<>();
    }

    public void save(Employee employee) {
        history.push(employee.save());
    }

    public void revert(Employee employee) {
        employee.revert(history.pop());
    }
}

All implementations of this pattern that I found are more or less equal to the one above. But there are some problems about this kind of implementation, that I don't like:

It's possible to triger both employee.revert() and caretaker.revert(employee). I would like to have only one access point.
If we want to change EmployeeMemento, we have to make changes in Employee class also (because of revert method).

Is there a way to overcome this? Or maybe I pay too much attention, and this details are not so important?

Solution

1) Note that Caretaker is supposed to take care of holding Mementos, not necessarily take care of Undo/Redo. If you look at the various implementations on Internet (For example here), you'll see that Caretaker does not have revert() but usually something like getMemento(). So the class that takes care of Undoing, is someone else which calls getMemento() on Caretaker and then revert() on Subject.

Even if you want Caretaker to take care of Undoing, note that employee.revert() is a method that's solely created to be used by caretaker.revert(), because in this design, no one else has access to Mementos. You can reduce it's visibility to be visible by only Caretaker. (If this was C++, it would be easily done by use of friend, but in Java you have to be creative and use package visibility or some other way.)

2) In Memento pattern, a class and its Memento are tightly coupled. Actually it's only the class itself that has access to Memento's internals and no one else should see how Memento is composed. So it does not matter if a change to class, propagates to its Memento.

Then again If you want to isolate changes, you can be creative again. For example instead of duplicating name and phone in both Class and its Memento, you could extract another class which contains these fields (let's say by the name of State) and then use this State in both the original class and its Memento. This way, when you have changes to state of the class, you need only to modify State.

Side note: It's better to define Memento as a nested static class inside the Subject.

So my design, that addresses your issues, would be something like this:

public class Employee {
    private State state;

    public Memento save() {
        return new Memento(state);
    }

    public void revert(Memento memento) {
        this.state = memento.state;
    }

    public static class Memento {
        private final State state;

        public Memento(State state) {
            this.state = state;
        }
    }

    public static class State {
        private String name;
        private String phone;
    }
}

public class Caretaker {
    private Stack<Employee.Memento> history;

    public Caretaker() {
        history = new Stack<>();
    }

    public void addMemento(Employee.Memento memento) {
        history.push(memento);
    }

    public Employee.Memento getMemento() {
        return history.pop();
    }
}

public class UndoHandler {
    Employee employee;
    Caretaker caretaker;

    public void snapshot() {
        caretaker.save(employee.save());
    }

    public void undo() {
        employee.revert(caretaker.getMemento());
    }
}