DEV Community: Aleksi Kauppila

Save time and money by collaborating

Aleksi Kauppila — Sat, 04 Jan 2020 14:41:09 +0000

I'm sure everyone who has ever done code reviews will have bumped into a situation where the design of the code is not good enough. Even worse, many times the code took a couple days to develop and we've already invested quite a lot of time to build a solution.

What do you do when this happens?

Let it slide and allow the bad design eat the productivity in little pieces for the coming months?

No.

You can point out those design problems and hope the author is able to rewrite the design in a timely manner. But that requires you to check up on the pull request later which causes context switching between your own development tasks and the review ultimately destroying your productivity for a time being.

Even better would be to call a meeting and solve the problems by pair programming with the author. Fix the issues immediately, commit, approve, merge and move on with our lives. But this is not always easy if there's no prior experience working with the other developer. Maybe the developer rejects the ideas of improving the design.

Whatever we decide, we're already in an awkward situation.

This doesn't have to happen though.

As developers we have to have a sense of smell for bad design. We have to be able to see the trade offs in the choices that we make. That's the first step.

If i find myself in a situation where i can see that my design has problems but i don't see the alternatives i must raise my hand and ask my coworkers to collaborate with me. The worst thing i could do is try to solve this problem alone. This would most likely take a lot more time and the result would still suck.

When i'm 1-2 hours in to development:

I already know something about the problem i'm solving.
I haven't yet written that much code. We can try alternative approaches and experiment.
Changing the direction is possible.

When i'm 3 days in to development:

I'm already feeling the pressure to deliver.
I'm already commited into the poor design i wrote.
Changing direction may require me to discard the work done for the last 3 days.

Identifying the limits of my own knowledge is crucial. But also having that sense of smell to see that there has to be better ways and not ignore that feeling. Taking initiative early on to collaborate with other developers will save time and money. That's how real teams develop software.

Composition over encapsulation

Aleksi Kauppila — Thu, 08 Aug 2019 19:17:02 +0000

A big problem with inheritance is that it easily gets out of hand a becames an unmaintainable mess. It very quickly becomes a tree that reaches out to all different directions. Also, when you change a class, it may have unexpected side-effects on inheriting objects.

Many times you hear that you should prefer composition over inheritance. I definitely get that. It tackles a lot of problems what inheritance causes.

But what about encapsulation?

abstract class Report
{
    abstract protected function title(): string;
    abstract protected function contents(): string;

    public function display(): string
    {
        return sprintf('%s\n%s', this.title(), this.contents());
    }
}

class SalaryReport extends Report
{
    private string title;
    private array salaries;

    public function constructor(string title, array salaries)
    {
        this.title = title;
        this.salaries = salaries;
    }

    protected function title(): string
    {
        return this.title;
    }

    protected function contents(): string
    {
        return this.salaries.join('\n');
    }
}

If i'd choose composition over inheritance, i would have to loosen the encapsulation of SalaryReport.title() and SalaryReport.contents() to public. The code would turn into this.

interface ReportData
{
    public function title(): string;
    public function contents(): string;
}

class Report
{
    private ReportData reportData;

    public function constructor(ReportData reportData)
    {
        this.reportData = reportData
    }
    public function display(): string
    {
        return sprintf('%s\n%s', this.reportData.title(), this.reportData.contents());
    }
}

class SalaryReportData implements ReportData
{
    private string title;
    private array salaries;

    public function constructor(string title, array salaries)
    {
        this.title = title;
        this.salaries = salaries;
    }

    public function title(): string
    {
        return this.title;
    }

    public function contents(): string
    {
        return this.salaries.join('\n');
    }
}

Now SalaryReport isn't a real object anymore since it really hasn't got any notable behaviour, it's just data. Of course you could argue that Report doesn't do much either, but for the sake of the example let's go with this.

How can i use composition over inheritance without weakening encapsulation and turning my objects into data structures? It seems both approaches have severe drawbacks.

Using isset() and empty() hurts your code

Aleksi Kauppila — Sat, 08 Jun 2019 18:34:03 +0000

It's a known flaw in PHP that functions in standard library are inconsistent. Some of them have a terrible API that may return anything from object to null and false. Some of them take arguments by reference, like for instance array sorting functions do. In short there's quite a bit features that work in a complicated fashion that makes the code worse.

A part of being a good programmer is to identify features in the language that hurt the code, the software we're building. Once those things are identified, a good programmer will minimize the impact of using those features or completely avoid using them if possible.

So, i've been stumbling lately quite a lot on isset and empty functions. These functions tap really well in to the weakly typed and procedural side of PHP. If you've already been riding the wave of modern PHP for a while, you've learned to hate that side of PHP.

isset

isset function in PHP docs:

isset — Determine if a variable is declared and is different than NULL"

And by example:

php > var_dump(isset($foo));
bool(false)

php > $foo = 'bar';
php > var_dump(isset($foo));
bool(true)

php > $foo = null;
php > var_dump(isset($foo));
bool(false)

php > $list = ['foo' => 'bar'];
php > var_dump(isset($list['baz']));
bool(false)

php > var_dump(isset($list['foo']));
bool(true)

Looking at the definition in PHP docs we can see that the function does two things.

Determine if variable is declared
Determine if variable is different than null

On the example there's three usages that i've witnessed as use cases for isset. Null checking and testing that variable is declared, but also checking if array has a key. First two match the definition in PHP docs and the third is just something developers have started using.

Now, the thing why using isset should be avoided is exactly the thing it promises. It does two things. This causes confusion to the reader.

We don't know if a variable is declared? How did this happen?

Either we are doing something very clever, which as we know is an awful habit, or we are working with a completely procedural legacy system.

If we do know that the variable is declared, then the only reason we'd use isset is for null checking. And for that, i'd recommend explicit null checks. In this code you obviously see easily where $customer came without any mutations. IRL there could be a lot more logic between these statements.

$customer = Customer::find($customerId);
if ($customer === null) {
    // throw
}

An alternative using the same solution that would be even more explicit and clear to the reader would be to null check with instanceof. In this case we know exactly what we're dealing with when we work with $customer.

$customer = Customer::find($customerId);
if (!$customer instanceof Customer) {
    // throw
}

Third situation i've seen, and personally used quite a lot, is checking if an array has a specific key:

if (isset($payload['username'])) {}

This is also another case where there's already a more explicit way to handle the problem.

if (array_key_exists('username', $payload')) {}

I admit, using isset is not the most serious offense. But it still is usually used for either null checking or testing if variable is declared, almost never for both. Being more explicit in these cases will help readers of your code.

empty

empty function in PHP docs:

Returns FALSE if var exists and has a non-empty, non-zero value. Otherwise returns TRUE.

The following values are considered to be empty:
"" (an empty string)
0 (0 as an integer)
0.0 (0 as a float)
"0" (0 as a string)
NULL
FALSE
array() (an empty array)

Wow, so i might be dealing with a string, integer, float, boolean, null or an array. If isset was bad for doing two things, empty is a completely different beast. It does seven things! It answers to seven different questions.

First step to resolve the problem is to use type hints everywhere. Use argument type hints and return type hints in every method and function you write. NOTE: This applies to those developers who work with PHP7 or higher.

Second step is to use more explicit conditionals. This means that the readers of the code don't have to dig deep to boring details when trying to understand what you're trying to accomplish. Using strict comparison (===) is very important.

if (strlen($foo) === 0) {} // Clearly a string
if ($foo === 0) {} // Clearly a number
if ((int) $foo === 0) {} // Clearly expected to be treated as a number
if ($foo === false) {} // Clearly a boolean
if (count($foo) === 0) {} // Clearly an array
if ($foo === null) {} // Clearly null or something useful

As a side note, using return type hints is a very good practice. It will result to more clear interfaces. Once you have a return type hint in place, there's no way you can return multiple different things from your method (assuming you don't use nullable type hints, as you should not).

Are you ready to throw isset() and empty() in to history's trash bin? Tell me what you think!

Dependency injection: PHP edition

Aleksi Kauppila — Sun, 03 Mar 2019 13:36:32 +0000

"Dependency injection" is used in everyday programming jargon with two different meanings. A lot of the time it refers to "container technology". Other times it refers to the design pattern, which of course, is a source for confusion. Making this distinction to "DI the design pattern" and "DI the container" helps us understand the concept better.

The pattern

If i'd have to choose a very limited list of must-know design patterns, dependency injection would certainly be on that list. The pattern itself is incredibly simple and enables you to write more testable and configurable applications.

Wikipedia lists three types of dependency injections: constructor-, setter- and interface injection. We will be focusing on constructor injection as other types produce inferior design.

It's really this simple:

// Without dependency injection
class MyService
{
    private $myLogger;

    public function __construct()
    {
        $this->myLogger = new MyLogger();
    }
    //...
}

// With dependency injection
class MyService
{
    private $myLogger;

    public function __construct(MyLogger $myLogger)
    {
        $this->myLogger = $myLogger;
    }
    //...
}

Instead of creating the instance of MyLogger in MyService, it's created in client code and passed in via constructor. This is really all there is to dependency injection design pattern.

This allows us to unit test this class. We can stub, mock or fake the behaviour of MyLogger. This is especially important if the injected object uses filesystem, database or connects to an external service.

Already, with this small modification we've achieved better testability of MyService. To achieve differing behaviour at runtime and overall better maintainability, we would have to depend on an interface instead of a concrete class.

The container

The container is just an object that provides access to instances of our services. I actually really like the term "service container" as these containers usually work best when they build services. I don't recommend defining DTO's, DAO's, forms, value objects or others in the container.

The basic idea is simple:

You write instructions on how to create services. These are called service definitions.
You give these instructions to a Container or ContainerBuilder.
You ask the Container to provide an instance of some class: $container->get(MyService::class);.

There are also technologies that provide autowiring which will do some magic and allow you to have a lighter configuration. I don't have anything against autowiring, but i do prefer explicit service definitions.

I've used two vendors for dependency injection containers: symfony/dependency-injection and php-di/php-di. They can be introduced to your project even though you may use other framework, or even no framework at all. I won't go into details here how to setup these technologies as they've good documentations.

When it comes to using the container finally there's one VERY important gotcha: you shouldn't have access to the container in your application logic layer or any higher layers! The access should be limited to only on lower levels of the application where the components that run the application are built. In older versions of Symfony, there used to be a convention to summon services from container inside the controllers, but that's fortunately in the past now.

PHP-DI's "best practices" guide provides us with these important rules. I urge you to read the whole document as it contains helpful advice to make your life easier with containers.

never get an entry from the container directly (always use dependency injection)

more generally, write code decoupled from the container

type-hint against interfaces, configure which implementation to use in the container's configuration

With the last point, we can continue where we left off with our simple example of dependency injection pattern.

To really make the code maintainable we have to adhere to dependency inversion principle which says:

High-level modules should not depend on low-level modules. Both should depend on abstractions.

Abstractions should not depend on details. Details should depend on abstractions.

First, we make sure MyLogger implements Psr\Log\LoggerInterface. Then we use LoggerInterface as type-hint for our logger. If MyLogger is a third party dependency, you can refer to techniques presented here.

class MyService
{
    private $myLogger;

    public function __construct(LoggerInterface $logger)
    {
        $this->myLogger = $logger;
    }
    //...
}

After this change, our code is more testable, configurable and more robust against changes.

Protect your system from changes in 3rd party dependencies

Aleksi Kauppila — Mon, 04 Feb 2019 10:01:14 +0000

There's always risks when using third party code. An open source library that i chose for my project may not be maintained anymore. Or, upgrading another dependency may have backward incompatible changes. The library may have an awful interface but the service it provides outweighs the negatives.

Everyday work is usually tasked with solving business related problems. Having to develop and maintain general purpose tools or libraries is usually not something that the customer is willing to pay. So we use open source libraries.

We also want to be smart and protect our application against changes in third party dependencies. We wan't to keep our dependencies up-to-date, like we keep our operating systems.

First, let's look at a bad example of how to deal with third party dependencies. I'll use the popular HTTP client guzzlehttp/guzzle as an example here. We want to use a HTTP client to make requests to an external system.

use GuzzleHttp\Client;

class ExternalService
{
    private $httpClient;

    public function __construct()
    {
        $this->httpClient = new Client();
    }
    // ...
}

In this example we are liberally declaring a dependency inside the constructor. This will prevent us from unit testing this class. We should be using dependency injection here.

The other problem in this example is that now we've tied ourselves to a concrete dependency. We should be considering dependency inversion principle which states that we should not depend on implementation details of lower level classes.

How do we make this right?

Design the interface you want to use

We'll define a HTTP client interface.

use Psr\Http\Message\ResponseInterface;

interface HttpClient
{
    public function get(string $uri, array $options): ResponseInterface;
    public function post(string $uri, array $options): ResponseInterface;
    public function put(string $uri, array $options): ResponseInterface;
    public function patch(string $uri, array $options): ResponseInterface;
    public function delete(string $uri, array $options): ResponseInterface;
}

Write an adapter for Guzzle's HTTP client

We'll make use of Guzzle's capabilities but hide it behind our own interface.

use GuzzleHttp\ClientInterface;
use MyApp\HttpException;

final class GuzzleHttpClientAdapter implements HttpClient
{
    private $client;

    public function __construct(ClientInterface $client)
    {
        $this->client = $client;
    }

    public function get(string $uri, array $options): ResponseInterface
    {
        return $this->request("GET", $uri, $options);
    }

    public function post(string $uri, array $options): ResponseInterface
    {
        return $this->request("POST", $uri, $options);
    }

    // ... other required methods

    private function request(string $method, string $uri, array $options): ResponseInterface
    {
        try {
            return $this->client->request($method, $uri, $options);
        catch (\Exception $exception) {
            // This is an exception we defined in our application
            throw new HttpException($exception->getMessage(), $exception->getCode(), $exception);
        }
    }
}

This way we've effectively restricted the usage of a third party library to a single method call in the whole application. Changing or upgrading the library will not cause ripple effect through our whole application.

* Adapter pattern

Use the new interface in client code

Finally, when we need an HTTP client in our application we must declare the dependency using the interface. If we designed the interface well, changing the dependency will require us only to write a new adapter and inject it where it's needed. The users of HttpClient will stay completely unaware of - and unaffected to - these changes.

use MyApp\HttpClient;

class ExternalService
{
    private $httpClient;

    public function __construct(HttpClient $httpClient)
    {
        $this->httpClient = $httpClient;
    }
}

// ...

$extService = new ExternalService(new GuzzleHttpClientAdapter(new Client()));

How you can reduce usage of getter methods in your code

Aleksi Kauppila — Mon, 24 Sep 2018 07:35:50 +0000

This is a subject that has been written A LOT, so i'm not going to repeat all those arguments in this post. You can read about them in here and here. Instead i'm going to give some examples how you can at least reduce usage of getter methods in your code.

In my view, having to use getting methods in objects is a symptom caused by extensive usage of setting methods. (You can see my thoughts on setters here.) When we encounter an object whose state may not be valid we have to ask questions to determine if we can work with the object. We depend on object properties and that makes our daily work a struggle.

Sorting a list

Sorting a list of employees would maybe need a getId() method that you could make sure you only have unique employees. Solution: write an equals() method. This allows you to encapsulate the problem of making a unique distinction of two instances of a class.

Ordering a list

Maybe you need to order a list of employees by some criteria. Perhaps by name? So you'd need a getLastName() and getFirstName() methods. Maybe not. If theres a default way to order a list of objects then write a compareTo() method. This will put the logic in one place where it naturally belongs.

Determine if an object is a part of some other object

Do you need to know if an employee belongs to a company? Write a belongsTo() method for the employee. This will allow you to hide the company in the employee object.

Represent an object in a view

Do you need to print employee names in a view? Simple, write a __toString() method. Now you don't have to expose $firstName or $lastName to the public. At least for this case...

class Employee
{
    private $id;
    private $firstName;
    private $lastName;

    /** @var Company */
    private $company;
    //...
    public function equals(Employee $employee): bool
    {
        return $this->id === $employee->id;
    }

    public function compareTo(Employee $employee): int
    {
        if (($this->lastName <=> $employee->lastName) === 0) {
            return $this->firstName <=> $employee->firstName;
        }
        return $this->lastName <=> $employee->lastName;
    }

    public function belongsTo(Company $company): bool
    {
        return $this->company->equals($company);
    }

    public function __toString(): string
    {
        return sprintf("%s, %s", $this->lastName, $this->firstName);
    }

}

Of course there are other situations where it may be tempting to write a public accessor. Yes, sometimes the simplest way to solve an issue is to write a getter. But it's very important to review all other options before writing that get-method. If your client code starts relying on a piece of data that an object provides, you're stuck with it.

None of those methods i presented expose internal properties. One other thing that makes them useful is that they return very explicit values. equals() and belongsTo() return a boolean value. A sort of "yes" or "no" answer. I find it extremely useful to provide explicit methods that return boolean values to common questions like $contract->isTerminated() or $contract->isCurrentlyValid() etc. This is much better than the implicit querying of termination dates and statutes. And of course, it allows us to do proper data hiding.

BTW, PHP allows asking private attributes inside a class of same object. This comes in very handy when we want to keep our public API clean and minimal. If you know how this can be handled in other languages, please write a comment! Also, if you know other use cases where some concept can help reducing getters, please let me know!

Discarding setters

Aleksi Kauppila — Sat, 08 Sep 2018 09:29:47 +0000

Setters, or mutators are used very commonly when working with classes. Almost every person who's ever written a class has also written some setter methods. I at least have written many! They are so common that even most IDEs provide a way to generate getter and setter methods from class properties. It's almost as if you're encouraged to write these sort of methods.

Newsflash: it's a terrible habit. Writing setter methods is a sure way to invite confusion and instability to the codebase. Setter methods are A SPAWN OF SATAN! (See, i avoided the usual "is evil" idiom!)

So, what's so bad about mutators? I need to be able to change the properties in my class somehow anyway, right?

The way i view this, is that setting stuff is all about configuration. I configure objects to some state by changing it's properties. The object is a computer program and i try to think like a computer.

This is a very normal approach in the field of software development. We have total control of these kinds of objects because we can change their inner state at will. But can we really handle the responsibility that comes with tracking that state in an application? This is a huge burden on any developer.

When we have setters, we pretty much also need getters to be able to check that state in different parts of the application. Checking stuff is great and all... Oh wait, i meant it's awful and i pretty much want to hand in my resignation every time i encounter an object whose state can be any combination of n properties.


/**
 * @param JobContract[] $contracts
 * @return Money
 */
public function totalSalaries(array $contracts): Money
{
    $total = new Money();
    foreach ($contracts as $contract) {
        if ($contract->getStatus() !== "active") {
            // continue, throw exception, anything
        }
        if ($contract->getType() !== "monthly") {
            // continue, throw exception, anything
        }
        if ($contract->getEmployee() === null) {
            // continue, throw exception, anything
        }
        if ($contract->getSalary() === null) {
            // continue, throw exception, anything
        }
        $total->add($contact->getSalary()->getAmount());
    }
    return $total;
}

Have you seen code like this in a code base you've worked? There's a huge mess of code that concerns only in deciding if it's possible to safely work with an object of JobContract.

Does setting things really happen a lot in the problem space we're modeling? I doubt it. This is an important shift in thinking. We shouldn't be thinking our models through their attributes. Instead, we should be thinking about their behaviour. When we understand how the objects should behave, we can then start to think about what they need to know to perform their advertised behaviour.

Consider terminating a JobContract:

class JobContract
{
    //...
    private $endDate;
    private $status;
    //...
    public function setEndDate(DateTime $endDate): void
    {
        $this->endDate = clone $endDate;
    }

    public function setStatus(string $status): void
    {
        if (!in_array($status, self::STATUSES)) {
            throw InvalidArgumentException("Invalid status {$status}");
        }
        $this->status = $status;
    }
}

There's no behaviour, just properties. State changes in uncontrolled manner. The business operation of terminating a contract is offloaded somewhere else. Some service decides that configuring the object this way represents a termination of a contract.

class JobContract
{
    //...
    private $endDate;
    private $status;
    //...
    public function terminate(DateTime $terminationDate): void
    {
        $this->endDate = clone $terminationDate;
        $this->status = self::STATUS_TERMINATED;
    }
}

Public API is simpler and reflects intent. State changes are encapsulated. Nobody knows or cares that the class even has a $status property. The business operation is explicit.

There are a lot of cases where the first instantiated status of an object is not the the final status. And there's always a way to change that status in some natural way that does not involve using a setter method.

Setter injection. That's a cool name for a bad practice. Setter injection basically is a form of configuring some object after it has been instantiated.

Something like this:

class HttpTransfer implements LoggerAwareInterface
{
    private $httpClient;
    private $logger;

    public function __construct(ClientInterface $httpClient)
    {
        $this->httpClient = $httpClient;
    }

    public function setLogger(LoggerInterface $logger): void
    {
        $this->logger = $logger;
    }
    //...
}

The purpose of setter injection is to provide a way to inject optional dependencies in to an object. There's for example a PHP-FIG approved PSR that uses setter injection (see LoggerAwareInterface). Or another one: Symfony's ContainerAwareInterface. Developers look up to these standards and frameworks as authorities for good practices so it's a shame that these kinds of things slip through the cracks.

Everything that may or may not be there when you need it adds a lot of unnecessary confusion. Once again, you have to null-check that property to make sure you can use the service. Are those dependencies really optional? Maybe there's some design considerations to do if that really is the case.

How to fix this? Inject everything via constructor. If in some environments you don't need logging, then configure so that you pass a fake instead. These "aware"-interfaces come also with interfaces representing that service. Such as ContainerInterface or LoggerInterface. So go ahead and implement your fakes. It's really that simple.

Services need to be stateless. Entities and value objects on the other hand need to be in a valid state ALL THE TIME. Programming is hard enough without us sabotaging ourselves. I know the subject of setters (and getters) have been written extensively but this issue still seems to persist. Thank you for reading.

Don't return associative arrays!

Aleksi Kauppila — Wed, 15 Aug 2018 10:30:08 +0000

I hate dealing with associative arrays when i'm writing client code. The
problem with arrays is that there is no context. There is no special knowledge. They are usually just data packaged in an inconvenient format. The worst part is that they tie me to a particular implementation.

Now, i don't mean that you shouldn't return arrays that have uniform data. Returning an array of certain type of objects is fine(ish). Although you still cannot promise through your API that it will contain only certain type of data.

Arrays may work as an internal representation of data for some class, but that is the way it should stay - internal.

What i'm talking about is those nasty multidimensional arrays with hardcoded strings as keys.

Consider this:

class AcmeFileSender implements FileSender
{
    public function sendFiles(FileCollection $files): array
    {
        $sent = 0;
        $errors = [];
        foreach ($files as $file) {
            try {
                $this->send($file);
                $sent++;
            } catch (SenderException $exception) {
                $errors[] = $exception->getMessage();
            }
        }
        return [
            "sent" => $sent,
            "failed" => count($errors),
            "errors" => $errors
        ];
    }

    public function send(File $file): void {//...}
}

Doesn't look that bad right? It's just three keyed values in an associative array.

The problem in this case is, that the array cannot promise us anything about it's structure. This means that when we write our client code we are dependent of sendFiles()'s implementation. We have to know exactly what the keys are and what sort of data is behind those keys.

In OOP we should not be concerned about implementation details of other classes. We care only about the interfaces.

Lately i've been bumping in to something that might be an even more serious offense:

public function processStuff($stuff): array
{
    //...
    return [$someArray, $someObject];
}

//... in client code
list($someArray, $someObject) = $this->processor->processStuff($stuff);

Now, it's one thing to return this kind of abominations from private methods. But when you start designing your public API like this all hope is lost. This is pretty much a dead give away that you are dealing with a procedural code base. In procedural code bases everything is data and can (and will) be manipulated.

In the case above the issue is the lack of context. What does it mean when you receive someArray and someObject together? Why are they returned together? It's clear that the implementation of processStuff() knows about how it is used. So it's not really designed as much as glued together with some client code. The client code is very tightly coupled to implementation of processStuff().

In many cases there's some confusion about how to define responsibilities. Code does something and returns some indication of it's success to the client. There is not much trust between the client and the server. The client is paranoid about server's quality of work.

What can we do to make the first example better? Value objects. sendFiles() would be a lot more expressive by using value objects.

interface SenderReport
{
    public function sent(): int;
    public function failures(): int;
    public function errors(): Iterator;
    public function print(): string;
}

We will update FileSender and it's implementations.

class AcmeFileSender implements FileSender
{
    //...
    public function sendFiles(FileCollection $files): SenderReport
    {
        //...
        return new FileSenderReport($sent, count($errors), $errors);
    }
}

Now we can rely on the interface of SenderReport instead of being painfully aware of the implementation of FileSender::sendFiles(). Now we have context. Also, we can change the implementations of FileSender freely.

Don't return associative arrays from the public API. Evaluate if you really even have to return a value. And, if you must, return something that has an interface that you and users of your code can rely on.