5 outcomes for great code review

As a technical lead, senior engineer, or technology director, code review can be one of the most valuable and force-multiplying tasks during your day.

Code review is often thought of by new managers or leaders as simply about identifying bugs, but I think it’s more than that. Especially when you’re working with junior developers, code review is an educational tool. It’s a chance to help your team learn best practises and write code that remains stable and reliable for years to come.

In my code reviews, I aim to:

  1. Identify bugs. As noted before, this is the most common thing people think about during code review, and of course it’s true: it’s an opportunity to identify mistakes or bugs before they make it into the code. As a technical lead, you might not have time to personally QA every pull request, but you can at least spot typos, mistakes, and obvious errors.
  2. Reduce fragility. As you gain more experience and move into senior and leadership roles, that should also come with a better understanding of where code is most fragile. Fragile code isn’t easy to identify for less experienced developers, but will likely jump out at you. Maybe it’s a particularly complex regex, a function with multiple return types, or an interaction with a finicky third-party API; you probably have a sense of where things might fall apart, and this is your chance to prevent that.
  3. Increase clarity. Clear, well-documented, easy-to-read code isn’t just a matter of preference: it’s an insurance policy against future change. However that manifests for you (I like to encourage inline comments, sufficient white-space, and clearly named methods and variables), it’s important to set the standard and
  4. Reduce complexity. This goes hand-in-hand with reducing fragility, but takes it slightly further. Junior developers can fall into the easy trap of writing code that’s too complex. That might mean a few too many conditionals in a method, or choosing a solution that is clever, but perhaps a little too intricate. Not all fragile code is complex, but all complex code is fragile. Be mindful of where you introduce complexity, and limit it where you can.
  5. Enforce consistency. As a lead engineer, you are one of the few people with a comprehensive, 360º view of your codebase. As such, it’s implicitly your job to make sure the code that enters the codebase is consistent. You can get some help with this from automated linting tools and good documentation, but code review also gives you the opportunity to add context, so you can explain whyyou enforce those requirements.

With these outcomes in mind, your code reviews will not only be a defensive tactic centred around avoiding mistakes and bugs, but they will also help you stay on the offensive by actively building reliability, predictability, and consistency into your code.

Great code review ensures you avoid the bugs of today while preventing the bugs of the future.

Goals for 2019

(In no particular order)

  1. Read more
  2. Write more
  3. Research more
  4. Move to Brooklyn
  5. Sketch more
  6. Slow down
  7. Regain my focus
  8. Meditate consistently
  9. Practise yoga consistently
  10. Read The Field Study Handbook
  11. Apply The Field Study Handbook
  12. Cook more
  13. Eat more vegetables
  14. Think bigger
  15. Write a newsletter
  16. Build a product
  17. Do more with Notion
  18. Travel to 3 continents
  19. Pay down debt
  20. Spend less time in Slack
  21. Act asynchronously
  22. Be online less

Notes from WordCamp for Publishers 2018

This week, I’m attending/speaking at WordCamp for Publishers 2018 in Chicago. It’s my first time in Chicago and my first time at an “enterprise-level” WordCamp, so I am beyond excited.

My talk was titled “Decisions, not Options” in the Age of Gutenberg, which covered some general design principles for building block UIs. As part of the talk I also open-sourced a new project: Gutenberg Human Interface Guidelines (HIG). These guidelines should help block developers and designers build more consistent and usable interfaces.

Below, you’ll find my slides from the talk, and I’ll post the video from WordPress.tv when it goes live.

Slides

If I missed any links, please let me know in the comments, and be sure to follow me on Twitter for more Gutenberg fun!

Thoughts on Composer, dependency management, and “practical” autoloading in WordPress

As Tomodomo (my company that builds websites for magazines) grows, I’m spending more and more of my time thinking through “bigger picture” technical architecture decisions, as opposed to actual implementation. I try hard to make sure our technical architecture follows broad industry best practices, while leveraging the best that WordPress has to offer.

In the broader world of PHP development, dependencies are typically managed with a tool called Composer. Composer is awesome; like many similar dependency managers (Bundler, npm, yarn, etc.) it allows you to define a list of dependencies your project requires and quickly install them from a number of sources (package repositories, source control repos, direct downloads of zip files, etc).

There are even ways you can leverage Composer in a WordPress workflow. WordPress can be installed into a particular folder in your project (we use public/wp/), plugins can be installed from your own Git repos or the WordPress Plugin Directory via wpackagist.org, etc.

For many sites that want a “standard” WordPress experience, while being able to lock versions of their plugins and Core (capital-C, meaning WordPress Core), this is typically enough. However, to truly borrow and learn from the rest of the PHP ecosystem, you probably also want to bring in third party code libraries.

In our work, we regularly want to pull in third-party libraries to use in our code. I’m a Stringy fanatic, for instance. I love Carbon for handling dates and date-related math. QueryPath has saved me from DOMDocument hell on multiple occasions.

If you’re not too worried about distributing any custom plugins on a project, you can probably stop reading here. We’ve been in this position almost exclusively since we started adopting Composer (which is why I haven’t given it much thought until recently). We include Composer’s autoloader at the top of our wp-config.php file in a project, and whenever we need to reference a dependency in a custom plugin embedded within a plugin, we just reference it. We know the autoloader was already active in wp-config.php, and we knew the plugins weren’t intended for use outside the context of this project and its repo, so who cared?

We sure didn’t, but over the past few months I’ve started looking at ways that we could release more open source code — particularly WordPress plugins — which has revealed a complex binary between situations where you want to resolve dependencies at the project level (great when building a website where your plugins and Core are also managed via Composer) and when you want to resolve dependencies at the plugin level (when you want to bundle a WordPress plugin for distribution).

How it’s usually done

Typically, WordPress plugins that require third-party dependencies optimise their workflow for packaging and distribution. They will either…

  1. Download the dependencies, possibly stripping out the parts they don’t need and maybe re-namespacing them, and adding them alongside their own plugin code
  2. Managing the dependencies in development with Composer, but committing the vendor folder to their repo (diffs be damned)
  3. Adding an extra build step before pushing their plugin to the WordPress Plugin Directory (or anywhere else for distribution)

For us, Option 1 was never viable. While it probably works fine in practice much of the time, having to make sure your plugin’s embedded dependencies are properly incorporating upstream changes sounds (to me at least) like a nightmare… not to mention a giant waste of time. This is the exact problem that dependency managers are intended to solve, after all!

Option 2 has its own downsides too. Even if you isolate your dependency update commits and just ignore those diffs, committing the vendor folder means your git repo’s file size will grow beyond your control. The vendor folder in one of our projects is around 50 MB; imagine if we had to store deltas and track history for all those files… woof.

Option 3, adding a build step to prepare a plugin for “normal” distribution, was reasonable for us — but the “normal” distribution method is only a fallback for us. The default, preferred approach is to have the plugin define dependencies in its composer.json, require the plugin in our project’s composer.json, and let the project-level composer.json resolve the dependencies that different plugins (or our “app”) might have.

We need to be able to accommodate cases where a plugin might go through a build step, but also might have its dependencies managed at a project level.

How do we do it? I don’t have the answer, but I might have an answer…

Conditionals

After messing around with “intelligent” approaches to detecting whether or not Composer is loaded, I came up with a simpler — and comparatively dumber — idea. What if we just define a constant after including Composer’s autoloader in our project’s wp-config.php? And then in each plugin, simply check if the constant is set and conditionally load the plugin’s Composer autoloader based on the result of that check?

It might look a little something like this:

wp-config.php

<?php

require_once( ABSPATH . '../../vendor/composer/autoload.php' );
define( 'IS_AUTOLOADING', true );

// More wp-config stuff

my-plugin.php

<?php
/**
 * Name: My Plugin
 * ...
 */

if ( defined( 'IS_AUTOLOADING' ) && ! IS_AUTOLOADING ) {
    require_once plugin_dir_path( __FILE__ ) . 'vendor/composer/autoload.php';
}

// More plugin stuff

There are some obvious benefits to this approach:

  • You can easily bundle your plugin’s vendor folder for distribution, and if someone installs it via “normal” WordPress mechanisms (uploading the zip or adding it through the WordPress Plugin Directory), everything should, theoretically, Just Work™.
  • You don’t waste resources actually checking to see if a Composer autoloader is running. Trying to find if a Composer autoloader is active can be tedious, can require you to know where the autoloader is actually located (which is configurable in a project’s composer.json), etc.

And of course, there are downsides:

  • The IS_AUTOLOADING constant is not a standard. If someone is attempting to load your plugin via a project-wide composer.json but isn’t setting that constant, your plugin could try to load its own autoloader and get confused. (It might be worth adding an additional file_exists check for your autoloader, if you don’t mind the slight overhead.)
  • Corollary: IS_AUTOLOADING being true does not guarantee that autoloading is in fact happening. A bit of trust is required there.
  • If someone has multiple plugins installed the “normal” way, with multiple autoloaders and vendor folders in parallel, there could be dependency issues because there’s no project-wide dependency conflict resolution to sort out the situation. This isn’t really different than the current state of play in WordPress, but just to be clear: this solution doesn’t solve that problem either.

If you’re developing a plugin that you intend to begin by developing within a project (a common way to start when you know you want to open source a particular plugin you’re building for a specific project, but you’re not ready just yet) you can add your plugin-specific composer.json and then use Composer’s path repository method to require your nested plugin code in your project-wide composer.json. Nice and easy!

Will it work…?

To be honest, this blog post isn’t a “this is how we solved this!” blog post, but more of a “what if this solved this?” type of post. We haven’t tried this in a production environment yet, and there may be other issues I haven’t considered. (There likely are.)

But I think it’s definitely worth considering, and I’d love to hear the community’s thoughts. If you’re interested in autoloading, dependency management, WordPress, and tying it all together in a way that preserves the separate boundaries that are key for WordPress, feel free to shoot me an email (chris@tomodomo.co) or leave a comment on this post. I’d love to get your feedback!