Forem: Niklas Lampén

Interfaces and exceptions

Niklas Lampén — Mon, 07 Aug 2023 07:47:16 +0000

Using interfaces – or protocols in some languages – is something we as programmers definitely want to do. Interfaces should be used to define dependencies between classes as it comes with quite a few benefits such as following the Dependency Inversion Principle.

I'm using PHP in my examples, but this applies to other languages, too.

TL;DR

An interface is a contract, defining dependencies with interfaces makes it easy to replace an implementation, and you should only throw specified exceptions from them.

What is an interface?

An interface is a specification that the implementing class has to follow. This means that no matter what concrete class we operate with, we know what to expect.

A simple example

Let’s start with a simple example. In this example we want to store user data to a storage. Because we’re looking well ahead, we understand to not lock ourselves with our current storage solution, which is MySQL. As the first step we went through the requirements and realized that we need to be able to store users to the storage, and fetch users from the storage based on user’s ID.

After thinking about it for a while, we came up with a definition of how we want to communicate to the storage, a.k.a. interface, a.k.a. UserDriverInterface:

interface UserDriverInterface
{
    /**
     * Store User to a database
     *
     * @param User $user The user to store to the storage
     *
     * @throws UserException Thrown if storing of the User fails.
     * @return bool
     */
    public function store(User $user): bool;

    /**
     * Find a User from the database by the ID
     *
     * @param string $id ID of the user to find from the storage
     *
     * @return User|null Returns an instance of User if user is found from the storage, NULL otherwise.
     */
    public function findById(string $id): ?User;
}

As said, our current storage solution is MySQL, so we wrote our first implementation, which connects to a MySQL database and uses that as the storage. As you can see, it implements UserDriverInterface:

class MysqlUserDriver implements UserDriverInterface
{
    protected ?PDO $connection;

    /**
     * @param string $username User name for the MySQL server
     * @param string $password Password for the MySQL server
     * @param string $server   MySQL Server URL
     * @param int    $port     MySQL Server port to use
     * @param string $database Name of the MySQL database/schema
     *
     * @throws UserException
     */
    public function __construct(string $username, string $password, string $server, int $port, string $database)
    {
        $connectionString = sprintf('mysql:host=%s;port=%d;dbname=%s', $server, $port, $database);

        try {
            $this->connection = new PDO($connectionString, $username, $password);
        }
        catch (\PDOException $e) {
            throw new UserException(
                'Connecting to MySQL server failed: ' . $server,
                0,
                $e
            );
        }
    }

    /**
     * @inheritDoc
     */
    public function store(User $user): bool
    {
        $stmt = $this->connection->prepare('INSERT INTO users (id, name) VALUES (:id, :name)');
        $stmt->bindValue(':id', $user->getId(), PDO::PARAM_STR);
        $stmt->bindValue(':name', $user->getName(), PDO::PARAM_STR);

        if ($stmt->execute() === false) {
            throw new UserException("Couldn't store user to MySQL database");
        }

        return true;
    }

    /**
     * @inheritDoc
     */
    public function findById(string $id): ?User
    {
        $stmt = $this->connection->prepare('SELECT id, name FROM users WHERE id=:id');
        $stmt->bindValue(':id', $id, PDO::PARAM_STR);
        $stmt->execute();
        $userData = $stmt->fetch(PDO::FETCH_OBJ);

        if ($userData === false) {
            return null;
        }

        /**
         * This should rather be done in separate converter
         */
        $user = new User();
        $user->setId($userData->id)
            ->setName($userData->name)
        ;

        return $user;
    }
}

How do we use this?

When we're writing our code, the only thing we refer to is the interface. One way could be fetching this driver from a DI-container with something like $container->get(UserDriverInterface::class). Or maybe you want to abstract it more and add a UserService to the stack. In the latter case only the service would know about the driver interface, and the code using the service would just use the service and not know what happens inside it.

As we're not really referencing to the concrete classes anywhere, it’s easy for us to change the concrete implementation from MySQL to something else. Maybe it’s for testing, or maybe we just decided that using plain text files is the way to go.

Another implementation

Let’s assume that we want to use a in-memory storage for testing our main implementation. We could write something like this:

class MemoryUserDriver implements UserDriverInterface
{
    /**
     * @var array The data storage
     */
    protected array $data = [];

    /**
     * @inheritDoc
     */
    public function store(User $user): bool
    {
        if (empty($user->getId())) {
            throw new UserException('Can\'t store User without ID');
        }

        $this->data[$user->getId()] = $user;

        return true;
    }

    /**
     * @inheritDoc
     */
    public function findById(string $id): ?User
    {
        return $this->data[$id] ?? null;
    }
}

All of the code you have written for MysqlUserDriver will work just as fine with MemoryUserDriver as both are implementing the UserDriverInterface.

If you wanted to use the memory storage in your development environment, and to use MySQL storage in production, you could have something like this in your factory:

class MyUserDriverFactory implements FactoryInterface {
    public function create(ContainerInterface $container): UserDriverInterface {
        $env = $container->get('environment');

        if ($env === System::ENV_PROD) {
            // Return MysqlUserDriver
        }

        // Return MemoryUserDriver
    }
}

Interface and exceptions

As a reminder, here’s the definition of store() method of the UserDriverInterface:

    /**
     * Store User to a database
     *
     * @param User $user The user to store to the storage
     *
     * @throws UserException Thrown if storing of the User fails.
     * @return bool
     */
    public function store(User $user): bool;

As you can see, the only exception it should throw is UserException. But what if our implementation is using a library that might throw an \AwesomeLibrary\SpaceTimeContinuumException?

What you should do is catch the \AwesomeLibrary\SpaceTimeContinuumException, and throw the UserException instead.

Why bother with catching and throwing?

Because, if we don’t, the code using the interface needs to know about the exact implementation of the interface – which pretty much defeats the purpose of an interface. The code using the interface shouldn’t know about the implementation.

When we are working on an interface that’s internal (used only inside the project), it’s easy to alter the interface each time you modify one of the implementations – but you shouldn’t do this. You should think about your interface as being defined somewhere else. Could you then add your own exceptions to it’s PHPDoc in that case? Well, no.

Or, what if there were 100 implementations of the interface? To list all of the possible exceptions, the interface should know about each implementation, and change the description of the interface for each of the implementations. Sounds sane? I didn’t think so.

And more importantly: what about the existing implementations? Let’s assume there’s this code:

try {
    $driver->store($user);
} catch (UserException $exception) {
    $logger->log('Storing user failed: ' . $exception->getMessage());

    return false;
}

$driver->store() is expected to thrown a UserException but no other exceptions. If someone decided that now the store() method can also throw \AwesomeLibrary\SpaceTimeContinuumException, the code wouldn’t handle it.

Summary

When defining an interface you should think about things going wrong as well as the happy paths, and you should have a plan how these issues are communicated to the class/function using the interface. Your mindset should be that you don't know anything about the concrete classes implementing the interface, and the concrete classes don't know anything about where they are being used.

supermetrics.com/careers/engineering

Avoiding feature creep as a developer

Niklas Lampén — Wed, 22 Mar 2023 07:55:46 +0000

TLDR: Make sure your code is extendable and do the bare minimum.

Feature creep is the excessive ongoing expansion or addition of new features in a product, especially in computer software, video games and consumer and business electronics. These extra features go beyond the basic function of the product and can result in software bloat and over-complication, rather than simple design.
Wikipedia: Feature Creep

We've all been there

We've all estimated an issue to be completed in a couple of hours, and days later, we're not sure when it'll be finished, but it's "almost there, just needs this one thing done". Just like yesterday and the day before that. One of the reasons why this might happen is feature creep: "I'm gonna do this one other thing too while I'm at it".

Simple example

Let's assume the requirement is "Set a value inside a multi-dimensional array using a simple JSONPath as path to data". We'd want to use something like $.path.to.data to set value of a 5 to our array. So, we'd do:

$array['path']['to']['data'] = 5;

The problem here is that we don't know the path in advance, so we need to be more creative. It's not a complex problem, and you can solve it with a simple solution like:

function setArrayValue(array $array, string $path, $value): array {
    $path    = preg_replace('/^\$\./', '', $path);
    $keys    = explode('.', $path);
    $current = &$array;

    foreach ($keys as $key) {
        $current = &$current[$key];
    }

    $current = $value;

    return $current;
}

Writing this code won't take days (especially as I pretty much copy-pasted it from StackOverflow). But couldn't we make it a bit better?

What if the path was something more complex? JSONPath allows paths like $.actions[action_type==video_view].foo. What if someone wants to do that instead? It's not included in the original requirement, but it might become handy. So, let's go ahead and implement it, right?

Stop!

…and this is where you should stop. Was the more complex path a requirement? No. It's not a "simple JSONPath". If you're unsure, ask the Product Manager for clarification. As it's not a requirement (and the PM confirmed that), you don't implement it.

Instead, you ensure that such a feature could be implemented in the future if needed. Basically, you're making sure that your code is extendable. And yes, looking at the function signature, nothing prevents you from doing this. Your code is extendable. You're good.

Why did you stop?

If you chose to continue and add the more complex path, you'd be guessing future needs. If you guessed wrong and we never needed the feature, implementing it would only have wasted time and effort, and delayed the features we actually needed. Implementing the more complex logic takes more work, as does testing it.

Explore engineering careers at Supermetrics at supermetrics.com/careers/engineering.

Getting rid of anti-patterns of development work

Niklas Lampén — Tue, 28 Feb 2023 13:27:37 +0000

In engineering, it's normal that teams sometimes struggle to get stories/tasks/bugs done. We, too, were facing this issue, so we sat down with the tech leads and managers, and discussed the most common reasons for issues we've had in our development work.

The top 6 reasons we identified are (in no particular order):

Refactoring code as part of another task.
The story's scope is unclear.
The story's scope is too wide.
Implementation hasn't (really) been planned.
Doing other "small" changes as part of another task.
Arguing over opinions in PRs.

We obviously wanted to tackle these issues and decided to lay down a few ground rules for development work. Here are the rules we agreed on:

1. Never refactor code as a part of another task

"Why not? I can do it in this small PR while I'm doing this other feature!"

– Random Developer

Refactoring changes the internals of existing code. Suppose we find a new bug after deployment. It may be hard to identify if the faulty behavior is caused by the new feature or bug fix, or by the code that was refactored simultaneously.

So, when should you refactor code? Here's what we think:

Good reasons to refactor code

The requested change can't be done on the existing code.
The existing code is very difficult and/or risky to extend.
Not refactoring it will definitely cause us problems.
Refactoring will provide us with business value.

Bad reasons to refactor code:

I don't like the code.
The code is legacy.
It's implementing a paradigm X, and I'd want it to use Y.
I'd like to refactor.
It could be written better.
I don't understand the code.

How to progress to refactoring

We decided that refactoring must be decided together with the tech lead for a good reason and it must be a separate task. No more refactoring just because someone felt like it! Our job isn't to write perfect code but to bring value to our clients.

Requirements of refactoring

When refactoring, if the code isn't tested, you must write the tests for the to-be-refactored code before the refactoring. After refactoring, you must ensure the code gives the exact same results. The bugs and all. Really.

Some other code might rely on very specific behavior of the code you're refactoring, so you must refactor it to do also the not-so-obvious stuff. Changing the behavior of the code is another task — read on.

2. Know what you're doing, part 1

You must have a clear vision of the outcome of what you're going to be working on. If you don't really know the outcome of the story, ask for more information.

3. Know what you're doing, part 2

The story/task must be split into workable chunks (subtasks). One chunk should be one PR, and one PR should be a stand-alone thing. It can be a new helper, and the second PR will use that helper. Use feature branches as the target for your PRs related to a single issue if you need multiple PRs.

Remember, you can add as many subtasks as you want to the issue you're working!

4. Know what you're doing, part 3

Have an implementation plan before you implement, and don't only have inside your own head. Talk to other people about your plan and ask for feedback. We've all been guilty of just starting to implement something because it's so simple. Have you ever ended up telling yourself "I should have realized this in advance" because of poor planning? Yeah, me too.

For new features (and bugs turning into adding something new), you must plan the architecture before you start implementing it. With sound architecture, the implementation can easily be replaced.

5. Stay within the scope

Avoid out-of-scope changes in your PRs. In the worst case, the out-of-scope changes require quite a bit of attention, delaying the completion of the actual task. If you're thinking, "is this inside the scope?" it likely isn't.

Again, as a reviewer, you're expected to reject any PRs that are doing too much, even if it seems ok.

6. Know when to stop

If the PR requires more than a couple of approval rounds, one or more of the previous "Know what you're doing" steps were likely skipped.
Stop! Go back to the drawing board, and don't try to force the current code to be approved.

As a reviewer: if the PR has so many problems that you don't know where to start reviewing, you can simply reject the PR and require additional planning.

Extra: A quick guide to reviewing a PR

Reviewing code is one of the final quality assurance tools before the code is deployed. It's the last chance for us to pick out any issues, making it a very important part of our development process.

Your task as a reviewer is to offer a second pair of eyes for the big picture, not to take responsibility over the details of the code being delivered. Even if you disagree with some of the original developer's design preferences, you should still approve the PR — as long as you agree the reviewed code is working, tested, and not going to cause problems in the foreseeable future.

For example, if you disagree on naming, ensure you aren't complaining about opinions. That being said, if the naming is clearly wrong, comment on it. Also, keep in mind that the PRs must follow the coding style guide.

Self-review

Just like you review other developers' code before approving it, you should review your own code. Self-review decreases the number of review cycles and shows consideration for other people's time and effort. It's a great opportunity to catch accidental changes, forgotten comments, typos, and more.

If you take a short break before reviewing for your code, you should get better results. If you doubt any part of your own code, others will too!

Summary

You'll be able to avoid quite a few pitfalls and delays by following these ground rules of development work.

Know what you're expected to achieve.
Plan how you're going to get there.
Stick with the plan.
Refactoring is a stand-alone task.
Review your own code.
Be strict but fair with your PR reviews.

Learn more about Supermetrics as a workplace at supermetrics.com/careers.