Why and How to Avoid the Memory Lock

2018-08-27

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:

• Do you want to create a code that works under certain assumption?
• Do you want to remember to do the B after each A?
• Do you want to keep over 50 of such A-B pairs in your head every time you open your main project?

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

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

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.

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!

"90% of the bugs I produced were for one of the two reasons:
1. Doing multiple things at one place
2. Doing one thing at multiple places" - @pseudo_coder

— Programming Wisdom (@CodeWisdom) 20. května 2018

Happy brain cells liberation!