PHP Memento design pattern implementation

Question

I am a self-taught programmer. I decided to learn design patterns by reading about them, and then coming up with their implementation in PHP. I know that learning would be virtually impossible without code review.

I will be grateful if you could review my implementation of the Memento design pattern and point to any shortcoming in the implementation of the pattern and general OOP principles.

My implementation tries to imitate the behaviour of a code repository (e.g. Git), where you can commit and checkout code based on previous commits. Below I present how a client can interact with the code.

/**
* Memento - a class that 'remembers' previous states
*/
class Repository
{
    /**
    * @var Commit[]
    */ 
    private $commits = [];

    /**
    * Commit objects are pushed into an array
    */
    public function saveCommit(Commit $commit)
    {
        array_push($this->commits, $commit);
    }

    /**
    * Rewind code to a selected commit by the commit key
    *
    * @param integer $commitKey The index key of a commit  to rewind code to
    * @return string Rewinded code
    */
    public function rewindCode($commitKey)
    {
        $rewindedCode = '';
        for($i=0; $i <= $commitKey; $i++) {
            $rewindedCode .= $this->commits[$i]->getCode();
        }       
        return $rewindedCode;
    }

    public function getCommits()
    {
        return $this->commits;
    }

    /**
    * Get all commited code so far
    * @return string
    */
    public function getCommitedCode()
    {
        $commitedCode = '';
        foreach($this->commits as $commit) {
            $commitedCode .=  $commit->getCode();
        }
        return isset($commitedCode) ? $commitedCode : '';
    }
}

-

/**
* A class/data structure that represents a single commit
*/
class Commit
{
    private $timestamp;
    private $code;

    public function __construct($code)
    {
        $this->code = $code;
        $this->timestamp = time();
    }

    public function getCode()
    {
        return $this->code;
    }
}

-

/**
* Originator - a class that can save itself and retrieve states from the Memento object (Repository)
*/
class CodeBase
{
    private $repository;
    private $code;

    public function __construct(Repository $repo)
    {
        $this->repository = $repo;
    }

    public function writeCode($code)
    {
        $this->code = $this->code . $code;
    }

    /**
    * Extract code added since the last commit
    * @return string Code that has not yet been commited
    */
    private function getNewCode()
    {
        $commitedCode = $this->repository->getCommitedCode();
        return $newCode = str_replace($commitedCode, '', $this->code);
    }

    /**
    * Save state (code) to Memento (Repository)
    */
    public function commitToRepository()
    {
        $newCode = $this->getNewCode();
        $this->repository->saveCommit(new Commit($newCode));
    }

    /**
    * Rewind code to the selected commit (Retrieve saved state)
    */
    public function checkoutFromRepository($commitKey)
    {
        $this->code = $this->repository->rewindCode($commitKey);
    }

    public function getCode()
    {
        return $this->code;
    }

}

-

$repo = new Repository();
$codeBase = new CodeBase($repo);

$codeBase->writeCode('First day code');
$codeBase->writeCode('Second day code');
$codeBase->commitToRepository();
$codeBase->writeCode('Third day code');
$codeBase->commitToRepository();

$codeBase->checkoutFromRepository(0); // Roll back to the first commit
echo $codeBase->getCode(); // Prints: "First day code Second day code"

$codeBase->checkoutFromRepository(1); // Roll back to the second commit
echo $codeBase->getCode(); // Prints: "First day code Second day code Third day code"

Answer 1

After a bit of experimenting, I get there

I'm also a self-taught programmer, but I've been at if for 33 years. I've seen a lot of things, but I never heard of a Memento design pattern. So, eventhough it is quite clear from your explanation and code what it is, the first thing to to is to look up what it exactly is. Here for instance:

https://sourcemaking.com/design_patterns/memento

There are just too many fancy names for these things, but I understand what they get at. I think we can also agree that you've tried to make the absolute simplest implementation of this design pattern; Just keeping track of one string, nothing more.

Let us discuss the code, starting from the beginning, which is your last piece of source code.

Your CodeBase class, the originator, internally needs a Repository class, the memento, to keep track of its state. Why do you declare it outside the CodeBase class and pretent it a argument to the construction of it? It is not. So the constructor of CodeBase should be:

public function __construct()
{
    $this->repository = new Repository();
}

Now declaring repository private actually makes sense.

Within CodeBase you have a method called getNewCode(). It works by removing the committed code from all the code in the CodeBase class. Moreso, you use this to commit new code with commitToRepository(). That just doesn't feel right. The state of your originator is basically a single string. You would want to remember that state, so you should commit the whole string, not a part of it. Remember that currently you only append the code strings at the end, but that's probably not how it will stay. If you edit the code string in the middle your method will not work anymore. That's why I get a bad feeling looking at that code. So CodeBase should look like this:

class CodeBase
{
    private $repository;
    private $code = '';

    public function __construct()
    {
        $this->repository = new Repository();
    }

    public function getCode()
    {
        return $this->code;
    }

    public function setCode($code)
    {
        return $this->code = $code;
    }

    public function appendCode($code)
    {
        $this->newCode = $this->newCode . $code;
    }

    public function commitCode()
    {
        $this->repository->commit($this->code);
    }
}

Which looks a lot cleaner. Yes, I have ommitted the roll-back method. More on that later. The point is that you can extend CodeBase to be a full fletched code editor and all these methods will still work.

I will agree that it does look as if we just shifted the problem to the Repository class, and you might be right. But the design pattern is about externalizing the object's internal state, and that's what I've done.

Here's is how I would define the Repository class:

class Repository
{
    private $commits = [];

    public function commit($state)
    {
        $this->commits[], $state);
    }

    public function commitCount()
    {
      return sizeof($this->commits);
    }

    public function rollBack($steps,$permanent = TRUE)
    {
       if ($stepsBack <= 0) throw new Exception('Invalid step back value');
       $commitCount = $this->commitCount();
       if ($steps > $commitCount)) return '';
       $commitIndex = $commitCount-$steps;
       $state       = $this->commits[$commitIndex];
       if ($permanent) $this->commits = array_slice($this->commits, 0, $commitIndex);
       return $state;
    }
}

It's a very basic implementation, mainly to show you the rollBack() method and what's going on with the externalized state of CodeBase. I've left out the Commit class for simplicity. So, yes, I store the complete code on every commit. Not efficient, I know, but necessary. The rollBack() method is what is important here. It normally does a real roll back, in other words, it is permanent. So a roll back method in the CodeBase class would look like this:

public function rollBackCode($steps)
{
   $this->repository->rollBack($steps);
}

Nevertheless this would not be the way I would inplement the Memento Design Pattern. The main thing that I don't like is that the Repository is part of the state of the CodeBase, which is your originator. Moreso, you only use one Memento object to store everything, whereas I think it would be more logical to have one of those objects per state you want to remember. This leads me to the following code:

class CodeState // memento
{
    private $code;

    function __construct(CodeBase $codeBase)
    {
      $this->setCode($codeBase);
    }

    public function getCode(CodeBase $codeBase)
    {
      $codeBase->setCode($this->code);
    }

    public function setCode(CodeBase $codeBase)
    {
      $this->code = $codeBase->getCode();
    }

}

And:

class CodeBase // orginator
{
    private $code;

    function __construct($code = '')
    {
      $this->setCode($code);
    }

    public function getCode()
    {
      return $this->code;
    }

    public function setCode($code)
    {
      $this->code = $code;
    }

    public function appendCode($code)
    {
      $this->code .= $code;
    }
}

And that is all that is needed. I still have to show how the 'caretaker' can use these two classes.

$commits  = array();              // I cannot use '[]' yet
$codeBase = new CodeBase();

$codeBase->appendCode('First day code');
$codeBase->appendCode('Second day code');
$commits[1] = new CodeState($codeBase);
$codeBase->appendCode('Third day code');
$commits[2] = new CodeState($codeBase);

$commits[1]->getCode($codeBase);  // Roll back to the first commit
echo $codeBase->getCode().'<br>'; // "First day code Second day code"

$commits[2]->getCode($codeBase);  // Roll back to the second commit
echo $codeBase->getCode().'<br>'; // "First day code Second day code Third day code"

Now this is much cleaner. You might not completely agree with the names of the methods, but I think you can see my basic idea here. You can add in timestamps in the CodeState class and roll back to any commit. There are no akward array or string manipulations.

Let's be clear, there's not much wrong with your implementation, or my first attemp, it's just that they don't comply with the design pattern you were trying to follow. All three ways work. Purists would say that only the last one is correct and the other two are feeble attempts at writing code by amateurs. So be it. Just write the code you are happy with. Following exact design patterns can be useful but it should never hinder you from creating code that does something useful.

Having said all that, I would also like to refer you to this blog posting:

https://blog.svpino.com/2015/04/22/the-thing-with-code-clarity-you-cant-be-proud-of-something-I-cant-read

Yes, this post has become too long, sorry for that.