Matthew Daly's Blog

I'm a web developer in Norfolk. This is my blog...

2nd January 2019 11:00 pm

Why Bad Code Is Bad

This may sound a little trite, but why is it bad to write bad code?

Suppose you’re a client, or a line manager for a team of developers. You work with developers regularly, but when they say that a code base is bad, what are the consequences of that, and how can you justify spending time and money to fix it? I’ve often heard the refrain “If it works, it doesn’t matter”, which may have a grain of truth, but is somewhat disingenuous. In this post, I’ll explain some of the consequences when your code base is bad. It can be hard to put a definitive price tag on the costs associated with delivering bad code, but this should give some idea of the sort of issues you should take into account.

Bad code kills developer productivity

Bad code is harder to understand, navigate and reason about than good code. Developers are not superhuman, and we can only hold so much in our heads at one time, which is why many of the principles behind a clean and maintainable code base can essentially be boiled down to “break it into bite-sized chunks so developers can understand each one in isolation before seeing how they fit together”.

If one particular class or function gets too big and starts doing too much, it quickly becomes very, very hard to get your head around what that code does. Developers typically have to build a mental model of how a class or function works before they can use it effectively, and the smaller and simpler you can keep each unit of code, the less time and effort it takes to do so. The mark of a skilled developer is not the complexity of their code bases, but their simplicity - they’ve learned to make their code as small, simple, and readable as possible. A clean and well laid-out code base makes it easy for developers to get into the mental state called “flow” that is significantly more productive.

In addition, if an application doesn’t conform to accepted conventions in some way, such as using inappropriate HTTP verbs (eg GET to change the state of something), then quite apart from the fact that it won’t play well with proxy servers, it imposes an additional mental load on developers by forcing them to drop a reasonable set of assumptions about how the application works. If the application used the correct HTTP verbs, experienced developers would know without being told that to create a new report, you’d send a POST request to the reports API endpoint.

During the initial stages of a project, functionality can be delivered quite quickly, but if the code quality is poor, then over time developer velocity can decrease. Ensuring a higher quality code base helps to maintain velocity at a consistent level as it gets bigger. This also means estimates will be more accurate, so if you quote a given number of hours for a feature, you’re more likely to deliver inside that number of hours.

Bad code is bad for developer welfare

A code base that’s repetitive, badly organised, overly complex and hard to read is a recipe for stressed developers, making burnout more likely. If a developer suffers burnout, their productivity will drop substantially.

In the longer term, if developer burnout isn’t managed correctly, it could easily increase developer turnover as stressed developers quit. It’s also harder to recruit new developers if they’re faced with the prospect of dealing with a messy, stressful code base.

Bad code hampers your ability to pivot

If the quality of your code base is poor, it can mean that if functionality needs to be changed or added, then more work is involved. Repetitive code can mean something has to be updated in more than one place, and if it becomes too onerous, it can make it too time-consuming or expensive to justify the changes.

Bad code may threaten the long-term viability of your project

One thing that is certain in our industry is that things change. Libraries, languages and frameworks are constantly being updated, and sometimes there will be potentially breaking changes to some of these. On occasion, a library or framework will be discontinued, making it necessary to migrate to a replacement.

Bad code is often tightly coupled to a particular framework or library, and sometimes even to a particular version, making it harder to migrate if it becomes necessary. If a project was written with a language or framework version that had a serious issue, and was too tightly coupled to migrate to a newer version, it might be too risky to keep it running, or it might be necessary to run an insecure application in spite of the risks it posed.

Bad code is more brittle

A poor code base will break, a lot, and often in ways that are clearly visible to end users. Duplicate code makes it easy to miss cases where something needs to be updated in more than one place, and if the code base lacks tests, a serious error may not be noticed for a long time, especially if it’s something comparatively subtle.

Bad code is hard, if not impossible, to write automated tests for

If a particular class or function does too much, it becomes much harder to write automated tests for it because there are more variables going in and more expected outcomes. A sufficiently messy code base may only really be testable by automating the browser, which tends to be very slow and brittle, making test-driven development impractical. Manual testing is no substitute for a proper suite of automated tests, since it’s slower, less consistent and not repeatable in the same way, and it’s only sufficient by itself for the most trivial of web apps.

Bad code is often insecure

A bad code base may inadvertently expose user’s data, or be at risk from all kinds of attacks such as cross-site scripting and SQL injection attacks that can also potentially expose too much data.

For any business with EU-based users, the risks of exposing user’s data are very serious. Under the GDPR, there’s a potential fine of up to €20 million, or 4% of turnover. That’s potentially an existential risk for many companies.

In addition, a bad code base is often more vulnerable to denial-of-service attacks. If it has poor or no caching, excessive queries, or inefficient queries, then every time a page loads it will carry out more queries than a more optimised site would. Given the same server specs, the inefficient site will be overwhelmed quicker than the efficient one.

Summary

It’s all too easy to focus solely on delivering a working product and not worry about the quality of the code base when time spent cleaning it up doesn’t pay the bills, and it can be hard to justify the cost of cleaning it up later to clients.

There are tools you can use to help keep up code quality, such as linters and static analysers, and it’s never a bad idea to investigate the ones available for the language(s) you work in. For best results they should form part of your continuous integration pipeline, so you can monitor changes over time and prompt developers who check in problematic code to fix the issues. Code reviews are another good way to avoid bad code, since they allow developers to find problematic code and offer more elegant solutions.

I’m not suggesting that a code base that has a few warts has no value, or that you should sink huge amounts of developer time into refactoring messy code when money is tight, as commercial concerns do have to come first. But a bad code base does cause serious issues that have financial implications, and it’s prudent to recognise the problems it could cause, and take action to resolve them, or better yet, prevent them occurring in the first place.

27th December 2018 6:37 pm

Improving Search in Vim and Neovim With FZF and Ripgrep

A while back I was asked to make some changes to a legacy project that was still using Subversion. This was troublesome because my usual method of searching in files is to use Tim Pope’s Fugitive Vim plugin as a frontend for git grep, and so it would be harder than usual to navigate the project. I therefore started looking around for alternative search systems, and one combination that kept on coming up was FZF and Ripgrep, so I decided to give them a try. FZF is a fuzzy file finder, written in Go, while Ripgrep is an extremely fast grep, written in Rust, that respects gitignore rules by default. Both have proven so useful they’re now a permanent part of my setup.

On Mac OS X, both are available via Homebrew, so they’re easy to install. On Ubuntu, Ripgrep is in the repositories, but FZF isn’t, so it was necessary to install it in my home directory. There’s a Vim plugin for FZF and Ripgrep integration, which, since I use vim-plugged, I could install by adding the following to my init.vim, then running PlugUpdate from Neovim:

" Search
Plug '~/.fzf'
Plug 'junegunn/fzf.vim'

The plugin exposes a number of commands that are very useful, and I’ll go through the ones I use most often:

  • :Files is for finding files by name. I used to use Ctrl-P for this, but FZF is so much better and quicker that I ditched Ctrl-P almost immediately (though you can map :Files to it if you want to use the same key).
  • :Rg uses Ripgrep to search for content in files, so you can search for a specific string. This makes it an excellent replacement for the Ggrep command from Fugitive.
  • :Snippets works with Ultisnips to provide a filterable list of available snippets you can insert, making it much more useful
  • :Tags allows you to filter and search tags in the project as a whole
  • :BTags does the same, but solely in the current buffer
  • :Lines allows you to find lines in the project and navigate to them.
  • :BLines does the same, but solely in the current buffer.

In addition to being useful in Neovim, FZF can also be helpful in Bash. You can use Ctrl-T to find file paths, and it enhances the standard Ctrl-R history search, making it faster and more easily navigable. The performance of both is also excellent - they work very fast, even on the very large legacy project I maintain, or on slower machines, and I never find myself waiting for them to finish. Both tools have quickly become an indispensible part of my workflow.

6th December 2018 6:34 pm

Decorating Service Classes

I’ve written before about using decorators to extend the functionality of existing classes, in the context of the repository pattern when working with Eloquent. However, the same practice is applicable in many other contexts.

Recently, I was asked to add RSS feeds to the home page of the legacy project that is my main focus these days. The resulting service class looked something like this:

<?php
namespace App\Services;
use Rss\Feed\Reader;
use App\Contracts\Services\FeedFetcher;
class RssFetcher implements FeedFetcher
{
public function fetch($url)
{
return Reader::import($url);
}
}

In accordance with the principle of loose coupling, I also created an interface for it:

<?php
namespace App\Contracts\Services;
interface FeedFetcher
{
public function fetch($url);
}

I was recently able to add dependency injection to the project using PHP-DI, so now I can inject an instance of the feed fetcher into the controller by typehinting the interface and having it resolve to the RssFetcher class.

However, there was an issue. I didn’t want the application to make multiple HTTP requests to fetch those feeds every time the page loads. At the same time, it was also a bit much to have a scheduled task running to fetch those feeds and store them in the database, since many times that would be unnecessary. The obvious solution was to cache the feed content for a specified length of time, in this case five minutes.

I could have integrated the caching into the service class itself, but that wasn’t the best practice, because it would be tied to that implementation. If in future we needed to switch to a different feed handler, we’d have to re-implement the caching functionality. So I decided it made sense to decorate the service class.

The decorator class implemented the same interface as the feed fetcher, and accepted another instance of that interface in the constructor, along with a PSR6-compliant caching library. It looked something like this:

<?php
namespace App\Services;
use App\Contracts\Services\FeedFetcher;
use Psr\Cache\CacheItemPoolInterface;
class FetcherCachingDecorator implements FeedFetcher
{
protected $fetcher;
protected $cache;
public function __construct(FeedFetcher $fetcher, CacheItemPoolInterface $cache)
{
$this->fetcher = $fetcher;
$this->cache = $cache;
}
public function fetch($url)
{
$item = $this->cache->getItem('feed_'.$url);
if (!$item->isHit()) {
$item->set($this->fetcher->fetch($url));
$this->cache->save($item);
}
return $item->get();
}
}

Now, when you instantiate the feed fetcher, you wrap it in the decorator as follows:

<?php
$fetcher = new FetcherCachingDecorator(
new App\Services\RssFetcher,
$cache
);

As you can see, this solves our problem quite nicely. By wrapping our feed fetcher in this decorator, we keep the caching layer completely separate from any one implementation of the fetcher, so in the event we need to swap the current one out for another implementation, we don’t have to touch the caching layer at all. As long as we’re using dependency injection to resolve this interface, we’re only looking at a little more code to instantiate it.

In addition, this same approach can be applied for other purposes, and you can wrap the service class as many times as necessary. For instance, if we wanted to log all the responses we got, we could write a logging decorator something like this:

<?php
namespace App\Services;
use App\Contracts\Services\FeedFetcher;
use Psr\Log\LoggerInterface;
class FeedLoggingDecorator implements FeedFetcher
{
protected $fetcher;
protected $logger;
public function __construct(FeedFetcher $fetcher, LoggerInterface $logger)
{
$this->fetcher = $fetcher;
$this->logger = $logger;
}
public function fetch($url)
{
$response = $this->fetcher->fetch($url);
$this->logger->info($response);
return $response;
}
}

The same idea can be applied to an API client. For instance, say we have the following interface for an API client:

<?php
namespace Foo\Bar\Contracts;
use Foo\Bar\Objects\Item;
use Foo\Bar\Objects\ItemCollection;
interface Client
{
public function getAll(): ItemCollection;
public function find(int $id): Item;
public function create(array $data): Item;
public function update(int $id, array $data): Item;
public function delete(int $id);
}

Now, of course any good API client should respect HTTP headers and use those to do some caching itself, but depending on the use case, you may also want to cache these requests yourself. For instance, if the only changes to the entities stored by the third party API will be ones you’ve made, or they don’t need to be 100% up to date, you may be better off caching those responses before they reach the actual API client. Under those circumstances, you might write a decorator like this to do the caching:

<?php
namespace Foo\Bar\Services;
use Foo\Bar\Contracts\Client;
use Psr\Cache\CacheItemPoolInterface;
class CachingDecorator implements Client
{
protected $client;
protected $cache;
public function __construct(Client $client, CacheItemPoolInterface $cache)
{
$this->client = $client;
$this->cache = $cache;
}
public function getAll(): ItemCollection
{
$item = $this->cache->getItem('item_all');
if (!$item->isHit()) {
$item->set($this->client->getAll());
$this->cache->save($item);
}
return $item->get();
}
public function find(int $id): Item
{
$item = $this->cache->getItem('item_'.$id);
if (!$item->isHit()) {
$item->set($this->client->find($id));
$this->cache->save($item);
}
return $item->get();
}
public function create(array $data): Item
{
$this->cache->clear();
return $this->client->create($data);
}
public function update(int $id, array $data): Item
{
$this->cache->clear();
return $this->client->update($id, $data);
}
public function delete(int $id)
{
$this->cache->clear();
return $this->client->delete($id);
}
}

Any methods that change the state of the data on the remote API will clear the cache, while any that fetch data will first check the cache, only explicitly fetching data from the API when the cache is empty, and caching it again. I won’t go into how you might write a logging decorator for this, but it should be straightforward to figure out for yourself.

The decorator pattern is a very powerful way of adding functionality to a class without tying it to a specific implementation. If you’re familiar with how middleware works, decorators work in a very similar fashion in that you can wrap your service in as many layers as you wish in order to accomplish specific tasks, and they adhere to the single responsibility principle by allowing you to use different decorators for different tasks.

20th October 2018 2:48 pm

Simplify Your Tests With Anonymous Classes

Anonymous classes were added in PHP7, but so far I haven’t made all that much use of them. However, recently I’ve been working on building a simple dependency injection container for learning purposes. This uses the PHP Reflection API to determine how to resolve dependencies. For instance, if it’s asked for a class for which one of the dependencies required by the constructor is an instance of the DateTime class, it should create an instance, and then pass it into the constructor automatically when instantiating the class. Then it should return the newly created class.

Mocking isn’t really a suitable approach for this use case because the container needs to return a concrete class instance to do its job properly. You could just create a series of fixture classes purely for testing purposes, but that would mean either defining more than one class in a file (violating PSR-2), or defining a load of fixture classes in separate files, meaning you’d have to write a lot of boilerplate, and you’d have to move between several different files to understand what’s going on in the test.

Anonymous classes allow you a means to write simple classes for tests inline, as in this example for retrieving a very basic class. The tests use PHPSpec:

<?php
namespace spec\Vendor\Package;
use Vendor\Package\MyClass;
use PhpSpec\ObjectBehavior;
use Prophecy\Argument;
use DateTime;
class MyClassSpec extends ObjectBehavior
{
function it_can_resolve_registered_dependencies()
{
$toResolve = new class {
};
$this->set('Foo\Bar', $toResolve);
$this->get('Foo\Bar')->shouldReturnAnInstanceOf($toResolve);
}
}

You can also define your own methods inline. Here we implement the invoke() magic method so that the class is a callable:

<?php
class MyClassSpec extends ObjectBehavior
{
function it_can_resolve_registered_invokable()
{
$toResolve = new class {
public function __invoke() {
return new DateTime;
}
};
$this->set('Foo\Bar', $toResolve);
$this->get('Foo\Bar')->shouldReturnAnInstanceOf('DateTime');
}
}

You can also define a constructor. Here, we’re getting the class name of a newly created anonymous class that accepts an instance of DateTime as an argument to the constructor. Then, we can resolve a new instance out of the container:

<?php
class MyClassSpec extends ObjectBehavior
{
function it_can_resolve_dependencies()
{
$toResolve = get_class(new class(new DateTime) {
public $datetime;
public function __construct(DateTime $datetime)
{
$this->datetime = $datetime;
}
});
$this->set('Foo\Bar', $toResolve);
$this->get('Foo\Bar')->shouldReturnAnInstanceOf($toResolve);
}
}

For classes that will extend an existing class or implement an interface, you can define those inline too. Or you can include a trait:

<?php
class MyClassSpec extends ObjectBehavior
{
function it_can_resolve_dependencies()
{
$toResolve = get_class(new class(new DateTime) extends Foo implements Bar {
public $datetime;
public function __construct(DateTime $datetime)
{
$this->datetime = $datetime;
}
use MyTrait;
});
$this->set('Foo\Bar', $toResolve);
$this->get('Foo\Bar')->shouldReturnAnInstanceOf($toResolve);
}
}

In cases where the functionality is contained in a trait or abstract class, and you might need to add little or no additional functionality, this is a lot less verbose than creating a class the conventional way.

None of this is stuff you can’t do without anonymous classes, but by defining these sort of disposable fixture classes inline in your tests, you’re writing the minimum amount of code necessary to implement your test, and it’s logical to define it inline since it’s only ever used in the tests. One thing to bear in mind is that anonymous classes are created and instantiated at the same time, so you can’t easily create a class and then instantiate an instance of it separately. However, you can instantiate one, then use the get_class() function to get its class name and use that to resolve it, which worked well for my use case.

Another use case for anonymous classes is testing traits or abstract classes. I generally use Mockery as my mocking solution with PHPUnit tests, but I’ve sometimes missed the getMockForTrait() method from PHPUnit. However, another option is to instantiate an anonymous class that includes that trait for testing purposes:

<?php
$item = new class() {
use MyTrait;
};

This way, your test class is as minimal as possible, and you can test the trait/abstract class in a fairly isolated fashion.

16th October 2018 9:00 am

Adding React to a Legacy Project

The project I’m currently working on is a textbook example of what happens when a project uses jQuery when it really ought to use a proper Javascript framework, or it starts out just using jQuery and grows out of all proportion. It’s also not helped by the fact that historically it’s just been worked on when new functionality needs to be added, meaning that rather than refactoring the code base, it’s been copied-and-pasted. As a result, there’s lots of repetitive code in desparate need of refactoring, and huge reams of horrible jQuery spaghetti code.

When I first took over responsibility for the project, I integrated Laravel Mix into it so that I had the means to refactor some of the common functionality into separate files and require them during the build process, as well as use ES6. However, this was only the first step, as it didn’t sort out the fundamental problem of repetitive boilerplate code being copied and pasted. What I needed was a refactor to use something more opinionated. As it happened, I was asked to add a couple of modals to the admin, and since the modals were one of the worst parts of the admin in terms of repetitive code, they were a strong candidate for implementing using a more suitable library.

I looked at a few options:

  • I’ve used Angular 1 quite successfully in the past, but I didn’t really want to use a framework that was being killed off, and it would be difficult to retrofit into a legacy application
  • Angular 2+ is actively maintained, but it would again be difficult to retrofit it into a legacy application. In addition, the need for TypeScript would make it problematic.
  • Vue was a possibility, but it did a bit too much for this use case, and it wasn’t all that clear how to retrofit it to an existing application

Eventually, I settled on React.js, for the following reasons:

  • It has a preset in Laravel Mix, making it easy to get started with it.
  • It has a very limited target - React is closely focused on the view layer, dealing only with rendering and event handling, so it does just what I needed in this case.
  • It has a strong record of use with legacy applications - after all, it was created by Facebook and they added it incrementally.
  • It’s easy to test - Jest’s snapshot tests make it easy to verify the rendered content hasn’t changed, and using Enzyme it’s straightforward to test interactions with the component
  • Higher order components provide a straightforward way to share functionality between components, which I needed to allow different modals to deal with another modal in the same way.
  • By creating a series of components for common user interface elements, I could then re-use those components in future work, saving time and effort.

However, it wasn’t entirely clear how I might go about integrating React into a legacy application. In the end, I managed to figure out an approach which worked.

Normally, I would create a single root for my application, something like this:

import React from 'react';
import ReactDOM from 'react-dom';
import App from './components/App';
ReactDOM.render(
<App />,
document.getElementById('root')
);

However, that wasn’t an option here. The existing modals were using jQuery and Bootstrap, and the new modals had to work with them. I therefore needed to have only certain parts of the UI managed with React, and the rest wouldn’t be touched. Here’s an example of how I rendered the modal in the end:

import React from 'react';
import ReactDOM from 'react-dom';
import higherOrderComponent from './components/higherOrderComponent';
import modalComponent from './components/modalComponent';
const Modal = higherOrderComponent(modalComponent);
window.componentWrapper = ReactDOM.render(
<Modal />,
document.getElementById('modalTarget')
);
window.componentWrapper.setState({
foo: 'bar'
});

By extracting the duplicate functionality into a higher order component, I could easily wrap the new modals in that component and share that functionality between the modals. I could then render each component in a different target element, and assign it to a variable in the window namespace. The div with a ID of modalTarget needed to be added in the appropriate place, but otherwise the HTML didn’t need to be touched, since the required markup was in the React component instead.

Then, when I needed to change a value int the statee of the component, I could just call window.componentWrapper.setState({}), passing through the values to set, and these would propogate down to the child components as usual. I could also render multiple different modal components on the page, and refer to each one separately in order to set the state.

This isn’t an approach I’d recommend on a greenfield project - state isn’t really something you should be setting from outside a component like this, and normally I wouldn’t do it. However, it seemed to be the easiest way for this particular use case. Over time I’ll port more and more of the UI over to React, and eventually it won’t be necessary as I’ll be storing the application state in something like Redux.

Recent Posts

Skipping Environment Specific Phpunit Tests

Powering Up Git Bisect With the Run Command

Writing Golden Master Tests for Laravel Applications

How Much Difference Does Adding An Index to a Database Table Make?

Searching Content With Fuse.js

About me

I'm a web and mobile app developer based in Norfolk. My skillset includes Python, PHP and Javascript, and I have extensive experience working with CodeIgniter, Laravel, Zend Framework, Django, Phonegap and React.js.