Memento Pattern implementation

I had just finished work on the Memento Pattern, and now I want to hear your feedback on it.

public class Caretaker {

    List snapshots;

    public Caretaker() {
        this.snapshots = new ArrayList();
    }

    public Caretaker(List snapshots) {
        this.snapshots = snapshots;
    }

    public void addSnapshot(Originator.Memento memento) {
        snapshots.add(memento);
    }

    public List getSnapshots() {
        return snapshots;
    }

    public Originator.Memento find(Criteria criteria, Object key) {
        switch (criteria) {
            case ByCode:
                for (Originator.Memento state : snapshots) {
                    if (state.getCode().equals(key)) {
                        return state;
                    }
                }
                break;
            case ByState:
                for (Originator.Memento state : snapshots) {
                    if (state.getState().equals(key)) {
                        return state;
                    }
                }
                break;
            case ByParameterKey:
                for (Originator.Memento state : snapshots) {
                    if (state.getParameters().containsKey(key)) {
                        return state;
                    }
                }
        }
        return null;
    }
}

And of course the Originator class:

```
public class Originator {

private String state;
private Integer code;
private Map parameters;

/**
* We have only setter here for security purposes (encapsulation). I mean we don't need
* to provide any access to the secret state for anybody.
*/
public void setState(String state) {
this.state = state;
}

public void setCode(Integer code) {
this.code = code;
}

public void setParameters(Map parameters) {
this.parameters = parameters;
}

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

public void restore(Memento memento)

I think your implementation is fine, you followed the wiki well, but I would suggest some improvements.

Unit testing

Before we touch anything, let's convert AppRunner to a proper unit test, so we can make changes and check easily that we're not breaking anything:

public class MementoTest {
    @Test
    public void test() {
        Originator originator = new Originator();

        originator.setCode(500);
        originator.setState("active");
        originator.setParameters(new HashMap() {{
            put("key1", "value1");
            put("key2", "value2");
        }});

        Caretaker caretaker = new Caretaker();
        caretaker.addSnapshot(originator.save());
        assertEquals(
                "Originator{state='active', code=500, parameters={key1=value1, key2=value2}}",
                originator.toString());

        originator.setCode(1500);
        originator.setState("pending");
        originator.setParameters(new HashMap() {{
            put("key1", "value1+");
            put("key2", "value2+");
        }});

        caretaker.addSnapshot(originator.save());
        assertEquals(
                "Originator{state='pending', code=1500, parameters={key1=value1+, key2=value2+}}",
                originator.toString());

        assertEquals(
                "Originator{state='pending', code=1500, parameters={key1=value1+, key2=value2+}}",
                originator.toString());

        Originator.Memento m = caretaker.find(Criteria.ByCode, 500);
        originator.restore(m);
        assertEquals(
                "Originator{state='active', code=500, parameters={key1=value1, key2=value2}}",
                originator.toString());
    }
}

I added more set* actions because the original method was testing only the code. In particular, the parameters map is the most interesting.

This is by no means a great unit test, it's just for the sake of keeping the current implementation intact.

You don't need the Cloner

Due to the way you implemented the setters on Originator, you don't need to clone anything. Consider for example the potentially most tricky one:

public void setParameters(Map parameters) {
    this.parameters = parameters;
}

You are replacing this.parameters. Had you implemented it this way:

public void setParameters(Map parameters) {
    this.parameters.clear();
    this.parameters.putAll(parameters);
}

This way you would have needed the deep cloning, but this is not the case.

The same goes for state and code as well.

So in fact you could simplify the Memento constructor using simple assignments:

public Memento(Originator o) {
    this.state = o.state;
    this.code = o.code;
    this.parameters = o.parameters;
}

Getters or not


But the main question is do I really need to provide the getters in Memento class or maybe I can use another approach?

Another approach can be to get rid of the getters, and make all fields public final in Memento:

public final String state;
public final Integer code;
public final Map parameters;

Since the fields are final, there's no harm in making them public, and it's simpler to access them directly. The downside is if you ever want to extend this class and override these fields. Then you would have to bring back the getters.

Naming

case ByCode:
    for (Originator.Memento state : snapshots) {
        if (state.getState().equals(key)) {
            return state;
        }
    }

It's a minor thing, but instead of calling the loop variable state, perhaps snapshot would be better, especially since the Originator has a state field:

case ByState:
    for (Originator.Memento snapshot : snapshots) {
        if (snapshot.getState().equals(key)) {
            return snapshot;
        }
    }

Implement searching elsewhere

As you yourself have noticed, the find method doesn't seem to fit well with the rest of the class. It seems it just doesn't belong there. As much as possible, it's best when a class does only one thing. In this case the Caretaker should store snapshot, and do nothing else.

If you want to search in the snapshots, you could move that functionality to another class. It will be a more extensible configuration, because you will be able to rewrite the searching logic in different ways without changing the Caretaker.

More unit tests

The Memento pattern is not easy. Add more unit tests to cover your back.

StackExchange Code Review Q#60232, answer score: 3