Why and How to Avoid the Memory Lock

When you close the door of my home, they're closed and you need a key to get in. But what if your door has door handle? You have to also lock them.

Instead of just closing the door you have to close the door and that one more thing. Why is that a bad thing in the code and how to avoid it?

I started refactoring easybook package last week and I found a few interesting snippets there.

Let's start with a simple question: which of following 7 code snippets opens space for potential bug that will take your new colleague 2-3 hours to solve?


1.

<?php

// ...

foreach ($this->publishingItems as $item) {
   $item['content'] = $this->decorateContent($item['content']);
}


2.

<?php

use Symfony\Component\EventDispatcher\EventSubscriberInterface;

final class SomeEventSubscriber implements EventSubscriberInterface
{
    /**
     * @return string[]
     */
    public static function getSubscribedEvents(): array
    {
         return ['processEvent'];
    }

    public function processEvent(ItemAwareEvent $itemAwareEvent)
    {
        $item = $itemAwareEvent->getItem();
        $item['content'] = 'new content';
    }
}


3.

<?php

// ...

$this->bookGenerator->setBookDirectory($bookDirectory);
$this->bookGenerator->generate();


4.

<?php

// ...

protected function setUp(): void
{
    $this->epub2Publisher = $this->container->get(Epub2Publisher::class);
}


5.

<?php

class SomeController
{
    public function someAction()
    {
        // ...

        $product = ...;

        $this->entityManager->persist($product);
        $this->entityManager->flush();
    }
}

Don't mind the presence of Doctrine in Controller here. That's a code smell we don't look for right now.


6.

<?php

use Symfony\Component\Console\Command\Command;

class SomeCommand extends Command
{
    /**
     * @var SomeDependency
     */
    private $someDependency;

    public function __construct(SomeDependency $someDependency)
    {
        $this->someDependency = $someDependency;
    }

    // ...
}


7.

<?php
use PhpParser\NodeTraverser;

class SomeNodeTraverser extends NodeTraverser
{
    /**
     * @var SomeDependency
     */
    private $someDependency;

    public function __construct(SomeDependency $someDependency)
    {
        $this->someDependency = $someDependency;
    }

    public function process()
    {
        foreach ($this->visitors as $visitor) {
            // ...
        }
    }
}

Do you have your favorite number? Good, try to remember it.

The memory-lock is very difficult to spot. We owe that to author blindness and thinking heuristics (brain short-cuts), that limits our ability to work effectively in the chaos or under pressure. If you've read Thinking, Fast and Slow (must have for anyone curious about his or her brain false positives) or any other book about social psychology, you know where I'm heading.

When Expectations are Met

Why it's difficult to spot? It depends on other code and it might code work if our expectations are met. What does it mean?

Let's take code snippet 7. Will this work or not? Under what condition?

<?php

foreach ($this->visitors as $visitor) {
    // ...
}

This pull-request reveals the answer.

Now back to your coding:

I don't. I want to be effective and create a valuable bullet-proof code that can be used only one way. A code that doesn't put the burden on the developer to investigate my code first. A code that is safe to use and cannot be broken (if possible).


So which of those 7 code snippets above are dangerous?


Drum rolls...


Yes, all of them! I knew you'd reveal my poor confusion.

Don't make the Programmer Think

I'm taking the title from my favorite intro UX book because it really punches the line. There are 3 groups of memory-locks that could be done better in the code snippets above.

1. Change Array Return

The memory lock: after I change array value, I have to put it back into an original set of an array or return it.

Snippets 1 and 2.

This code would change the $item['content] value, but $this->publishingItems remains unchanged:

<?php

// ...

foreach ($this->publishingItems as $item) {
    $item['content'] = $this->decorateContent($item['content']);
}

How to Solve it?

 <?php

 // ...

-foreach ($this->publishingItems as $item) {
+foreach ($this->publishingItems as $key => $item) {
     $item['content'] = $this->decorateContent($item['content']);
+    $this->publishingItems[$key] = $item;
 }

Use object instead:

 <?php

 // ...

 foreach ($this->publishingItems as $item) {
-     $item['content'] = $this->decorateContent($item['content']);
+     $item->changeContent($this->decorateContent($item['content']));
 }

Objects consume less memory anyway and you are safe - more importantly, anyone extending this code ever after is safer.

2. Double-Method Call

The memory lock: After I call this method I have to call that method to make it really work.

Snippets 3 and 5.

You also have to think about the C - the order:

$this->bookGenerator->setBookDirectory($bookDirectory);
$this->bookGenerator->generate();

vs.

$this->bookGenerator->generate();
$this->bookGenerator->setBookDirectory($bookDirectory);

vs.

$this->bookGenerator->generate();

How to Solve it?

Use one method that handles it both:

 <?php

 // ...

-$this->bookGenerator->setBookDirectory($bookDirectory);
-$this->bookGenerator->generate();
+$this->bookGenerator->generateFromDirectory($bookDirectory);

Same applied to code snippet 5:

 <?php

 // ...

-$this->entityManager->persist($product);
-$this->entityManager->flush();
+$this->productRepository->save($product);

I dare you to generate the book or save the product the wrong way now!

3. Parent Logic

Snippets 4, 6 and 7.

The memory lock: After I call anything in parent method, I have check if I need to call the constructor to prepare some logic.

This would actually fail:

<?php

foreach ($this->visitors as $visitor) {
    // ...
}

Why? Because in parent::__construct() is set default value for property with null:

<?php

// ...

public function __construct()
{
    $this->visitors = [];
}

How to Solve it?

In this case just add default value to property itself:

-private $visitors;
+private $visitors = [];

There is even coding standard fixer for that.

Also, do not add any logic to constructor apart dependency injection. Constructor injection is the main reason to use the constructor in 99 %, so most people probably don't expect any extra logic there. Create extra method instead or decouple a class, put it into the constructor and call the method on it.

Try to avoid these patterns in code, in door design or in architecture of the application as a whole. One day some programmer will silently thank you when he or she will find what memory lock is (the painful way).


Do you know any other memory locks we should watch out for? Share them in the comment!



Happy brain cells liberation!


Typo? Fix it, please  and join 29 people who build this website

GitHub RSS @votrubaT Runs on Statie Hosted on GitHub Build by 30 people

Like what I write about? Hire me & we can work together