Author: Tim Barclay

This is the 5th part of a series about 67 Bricks’s coding principles. The previous posts are: 1, 2, 3 and 4.

The principle

All code should be reviewed before it is merged into the main branch. All project members, however junior, should be involved in code reviewing.

This is another – I hope – uncontroversial principle.

At 67 Bricks we generally use Gitlab for our code hosting and use their merge request feature for signalling that a change is ready to review and then for checking the changes and leaving comments for the author. All the other big git hosting platforms have equivalent tools that are just as good, so there’s no excuse not to do code reviews when working in a team.

Code reviewing is beneficial for the quality of the codebase because a reviewer may spot edge cases, mistakes, issues or potential problems that the original author didn’t consider. They may also be able to suggest improvements, based on their own knowledge of the project or of the relevant technologies.

We all make mistakes, so having another pair of eyes looking over your work makes it more likely that those mistakes get noticed before they cause a real problem. We also all have different knowledge, experiences, strengths and weaknesses, so code reviewing is a way of bringing the experience of a second person to bear on a problem.

Another benefit is that the reviewer comes to the code at some distance from the detailed problems the developer had to wrangle with, and this can be useful when seeing the complete set of changes as a whole. This is distance that the author will probably gain themselves over the next days and weeks, but it’s useful to have it immediately from another person.

Knowing that your code will be reviewed also encourages you to be more thorough. This is just human behaviour, especially when we are busy and keen to be seen to be making progress.

Slightly less obviously, code reviewing also has benefits for the reviewer because it exposes them to areas of the codebase they may not have worked on before and encourages them to engage with, and constructively criticise, others’ code. This gives them the opportunity to be exposed to and learn from others’ approaches.

And I should emphasise constructive criticism. When it works well, code reviewing can lead to a closer team built on trust. When we, as reviewers, suggest changes, we need to do so without implying criticism. And when receiving review comments, we need to understand that the health of the codebase (and the project) is more important than our egos.

As much as it takes effort to do a good, thorough code review, the benefit is huge. I’m sure I can’t be the only person who has been guilty of waving through a review with minimal attention – perhaps because I know the author is sensible and writes good code – only to find later that it caused some bug that I could have prevented if I’d engaged with it a bit more. Skimping on the effort to review properly is a false economy because the mistakes you miss will just need to be fixed later, leading to more reviews.

Generally speaking we at 67 Bricks think only one person need review each change, but there may be cases where it makes sense for more than one person to be involved, for example to get the input of a particular subject matter expert.

I don’t think anyone would pretend that reviewing code changes is their favourite part of the job, but there are things we can do when putting our code up for review that make everyone’s lives easier.

• we can aim to keep merge requests small and focussed (spoiler alert: this is the focus of a future principle)
• we can provide any necessary context, descriptions and (if applicable) screenshots when opening the merge request to give the reviewer the best chance of understanding what they’re looking at
• we can aim to keep each commit focussed on a single, meaningful change rather than lumping lots of unrelated changes in each commit. This makes it easier to review commit by commit, which can be preferable in some cases
• we can be available to answer the reviewer’s questions. It can even be helpful to quickly walk a reviewer through your change so they fully understand the context and your intentions before fully reviewing it themselves

Code reviews can unfortunately lead to a bottleneck in the development process where a number of changes sit unreviewed and becoming stale while the team works on other things, so it’s worth trying to keep on top of them. It often works to have a team policy that, upon finishing a piece of work, members should review at least one open merge request before picking up something new.

Code reviewing generally isn’t what gets anyone up in the morning, but it’s immeasurably valuable for the overall quality of the codebase. And slacking on it is likely to lead to costlier problems later on, so it’s worth trying to do well.

Resources

https://leanpub.com/whattolookforinacodereview

https://conventionalcomments.org/

This is the 4th part of a series about 67 Bricks’s coding principles. The previous posts are: 1, 2 and 3.

The principle

Test at multiple levels

I don’t think it’s controversial to say that tests are A Good Thing.

Functionality should be well tested so that we can be confident that it works correctly. Tests at different levels bring different benefits and should be combined to provide a high level of confidence in the software’s quality.

A rough common rule of thumb is that there should be:

• lots of unit tests
  • these focus on individual units of code
  • they should be small, focused and quick to run
• slightly fewer integration tests
  • these focus on testing multiple units together, potentially including external systems like databases
  • they tend to be a bit slower to run and more involved to set up
• fewer again end-to-end tests
  • these test the whole stack
  • they generally test from the point of view of an end user or client of the system, so they might run via a headless browser or via a REST API
  • they tend to be comparatively slow and complex so they should be used sparingly and where they add real value
• a small number of smoke tests
  • these are very basic end-to-end tests that can be run post-deployment or at regular intervals to check that the service is healthy

There is much that sensible people can disagree on in the above, like where the line sits between unit and integration tests; how much value there is in mocking in unit tests and much more. But I think the broader point that there is value in having tests at multiple levels stands.

By writing good tests at multiple levels, and running them often, it is possible to have a high level of confidence that a piece of software is in good working order.

Well written tests can bring a huge number of benefits, some of which are perhaps less obvious than others.

Tests verify that a piece of functionality works as intended

This is perhaps the most obvious benefit of tests: they test that some code does what you think it does.

While at 67 Bricks we are fairly agnostic to TDD (you’re welcome to use it, but you don’t have to), we do advocate for interleaving writing code with writing tests, rather than writing all the code first and leaving the tests till the end. Writing tests throughout the process can be hugely helpful in writing code that does what you intend it to with minimal bugs.

Tests encourage good, clean, modular code

It is a good rule of thumb that if a unit of code is hard to test, it’s probably an indication of a problem that you should fix. If it’s hard to test, perhaps it’s too tightly coupled or it’s making some undue assumptions or it has a confusing interface or it’s relying on hard-to-reason-about side effects… Wherever the difficulty springs from, the fact that it’s hard to test is a useful warning sign.

Tests act as specifications of behaviour

Each of your tests can act as an encapsulated description of how this unit of code is intended to act in particular circumstances. This is great as a way of documenting the developers’ intentions. If I come to a method and find myself wondering what to expect if null is passed into it, then my life will be made a lot easier if there’s a corresponding test like

it('throws an error when null is passed in', () => {

This example uses jest, a popular testing framework in the Javascript/Typescript world that allows you to write very spec-friendly, descriptive test names. In languages or frameworks that require you to use function names rather than strings for test names, I advocate making those function names as long and descriptive as possible, like

void throwsAnErrorWhenNullIsPassedIn()

Tests act as examples of how to use units of code

Related to the above point, they also act as written examples to future developers of how to use or consume the module under test.

Tests guard against regressions and other unintended changes

One of the most valuable things about adding tests as new features develop is that they remain in the codebase indefinitely as guards against unintended behaviour changes in the future. When working on a large system – particularly when developers come and go over time – it’s invaluable to be able to get instant feedback that a change you’ve made has caused a test to fail. This is especially true if that test is descriptively named, as recommended above, because it will help you understand what to do to fix the failure. Perhaps your change has had unintended consequences, or perhaps the test simply needs updating based on your change – a well named and well written test will help you make that call.

For this reason, it can sometimes be useful to test fairly trivial things that wouldn’t be worth testing if the only benefit were checking that your code works. Sometimes it’s valuable to simply enshrine something in a test to prevent accidental changes to important behaviour.

Tests help you refactor with confidence

When refactoring, tests are indispensable. If the code is well covered by good tests, and those tests pass after you’ve finished the refactor, you can have a high degree of confidence that you haven’t inadvertently changed any behaviour.

Resources

https://martinfowler.com/articles/practical-test-pyramid.html

This is the 3rd part of a series about 67 Bricks’s coding principles. The previous posts are: 1 and 2.

The principle

Aim for simplicity over complexity. This applies to everything from overarching architectural decisions down to function implementations.

This principle is a close cousin of the previous one – aim for clear, readable code – but emphasises one particular aspect of what makes code clear and readable: simplicity.

Simpler solutions tend to be easier to implement, to maintain, to reason about and to discuss with colleagues and clients.

It can be tempting to think that for software to be good or valuable it must be complicated. There can be an allure to complexity, I think partly because we tend to equate hard work with good work. So if we write something labyrinthine and hard to understand, it’s tempting to think it must also be good. But this is a false instinct when it comes to software. In code, hard does not equal good. In general complexity for its own sake should be avoided. It’s important to remember that there’s absolutely nothing wrong with a simple solution if it does what’s needed.

There’s also value in getting a simple solution working quickly so that it can be demoed, reviewed and discussed early compared to labouring for a long time over a complex solution that might not be correct. Something we emphasise a lot working at 67 Bricks is the value of iteration in the agile process. It can be extremely powerful to implement a basic version of a feature, site or application so that stakeholders can see and play with it and then give feedback rather than trying to discuss an abstract idea. Here, simplicity really shines because often getting a simple thing in front of a stakeholder in a week can be a lot more valuable than getting a complicated thing in front of them in a month.

This principle applies at every level at which we work, from designing your architectural infrastructure, down through designing the architecture of each module in your system, down to writing individual functions, frontend components and tests. At every level, if you can achieve what you need with fewer moving parts, simpler abstractions and fewer layers of indirection, the maintainability of your whole system will benefit.

Of course there are caveats here. Some code has to be complicated because it’s modelling complicated business logic. Sometimes there must be layers of abstraction and indirection because the problem requires it. This principle is not an argument that code should never be complicated, because sometimes it is unavoidable. Instead, it is an argument that simplicity is a valuable goal in itself and should be favoured where possible.

Another factor that makes this principle deceptively tricky is that it is the system (the architecture, the application, the class etc) that should be simple, not necessarily each individual code change. A complex system can very quickly emerge from a number of simple changes. Equally, a complicated refactor may leave the larger system simpler. It’s important to see the wood for the trees here. What’s important isn’t necessarily the simplicity of an individual code change, but the simplicity of the system that results from it.

There’s also subjectivity here: what does “simple” really mean when talking about code? A good example of an overcomplicated solution is the FizzBuzz Enterprise Edition repo – a satirical implementation of the basic FizzBuzz code challenge using an exaggerated Enterprise Java approach, with layers of abstraction via factories, visitors and strategies. However, all the patterns in use there do have their purpose. In another context, a factory class can simplify rather than obfuscate. But it’s important not to bring in extra complexity or indirection before it’s necessary.

Resources

The Wrong Abstraction – Sandi Metz

The Grug Brained Developer

Simplicity is An Advantage but Sadly Complexity Sells Better

This is the 2nd part of a series about 67 Bricks’s coding principles. The first post, containing the introduction and first principle is here.

The principle

Aim for clear, readable code. Write clear, readable comments where necessary

You should make it a priority that your work be readable and understandable to those who might come to it after you. This means you should aim to write code that is as clear and unambiguous as possible. You should do this by:

• using clear variable, function and class names
• avoiding confusing, ambiguous or unnecessarily complicated logic
• adhering to the conventions and idioms of the language or technology you’re using

What can’t be made clear through code alone should be explained in comments.

Comments should focus on “why” (or “why not” explanations) far more than “how” explanations. This is particularly true if there is some historical context to a coding decision that might not be clear to someone maintaining the code in the future.

Note however that just like code, comments must be maintained and can become stale or misleading if they don’t evolve with the code, so use them carefully and only where they add value.

It is important to recognise that your code will be read far more times that it is written, and it will be maintained by people who don’t know everything you knew when you wrote it; possibly including your future self. Aim to be kind to your future self and others by writing code that conveys as much information and relevant context as possible.

I expect we’ve all had the experience of coming to a piece of code and struggling to understand it, only to realise it was you who wrote it a few months or weeks (or even days?) ago. We should learn from this occasional experience and aim to identify what we could have changed about the code the first time that would have prevented it. Better variable names? More comments? More comprehensive tests?

“You’re not going to run out of ink,” is something a colleague once commented on a pull request of mine to say that I could clarify the purpose of a variable by giving it a longer, more descriptive name. I think that’s a point worth remembering. Use as many characters as you need to make the job of the next person easier.

Of course, there’s some subjectivity here. What you see as obscure, someone else might see as entirely clear and vice versa. And certainly there’s an element of experience in how easily one can read and understand any code. The point really is to make sure that at least a thought is spared for the person who comes to the code next.

Examples

Here is an example that does not follow this principle:

const a = getArticles('2020-01-01');
a && process(a);

This example is unclear because it uses meaningless variable names and somewhat ambiguous method names. For example, it’s not clear without reading further into each method what they do – what does the date string parameter mean in getArticles? It also uses a technique for conditionally executing a method that is likely to confuse someone trying to scan this code quickly.

Now, here’s an example attempts to follow the principle:

// The client is only interested in articles published after 1st Jan
// 2020. Older articles are managed by a different system.
// See <ticket number>
const minDate = '2020-01-01';

const articlesResult = getArticlesSince(minDate);
if (articlesResult) {
  ingestArticles(articlesResult);
}

It provides a comment to explain the “why” of the hardcoded date, including relevant context; it uses much more meaningful names for variables and functions; and it uses a more standard, idiomatic pattern for conditionally executing a method.

Resources

Naming is Hard: Let’s Do Better (Kate Gregory, YouTube)

Introduction to the principles

When I started working at 67 Bricks in 2017, in a small Oxford office already slightly struggling to contain about 15 developers, I found a strong and positive coding culture here. I learnt very quickly over my first few weeks what kind of code and practices the company valued. Some of that learning came via formal routes like on-boarding meetings and code review comments, but a lot of it came just by being in the office among many excellent developers and chatting or overhearing chats about opinions and preferences.

While there’s something very nice about this organic, osmosis-like way of ingesting a company’s values, practices and principles, it has been forced to evolve by a few factors over the last year. First we switched to home-working during the Covid lockdowns of 2020 and 2021 and then settled into a hybrid working model in which home-working is the default for most of us and the office is used somewhat less routinely. Secondly, we’ve increasing our technical team quite significantly over the last several years. Thirdly, that growth has partly involved a focus on bringing in and developing more junior developers. Each of these changes has made the “osmosis” model for new starters to pick up the company’s values a bit less tenable.

So over recent months, the tech leads have undertaken a project to distil those unwritten values and principles into a set of slightly more formal statements that new starters and old hands alike can refer to to help guide our high level thinking.

We came up with 9 of these principles. This and the following 8 posts in this series will go through each principle describing it and explaining why we think it is important in our ultimate goal of producing good, well-functioning products that run robustly, meet customer needs and are easy to maintain. 67 Bricks’s semi-joking unofficial motto is “do sensible things competently”; these principles aim to formalise a little what we mean by “sensible” and “competent”.

Generally I’ve used Typescript to write any code examples. The commonality of Typescript and Javascript should mean that examples are understandable to a good number of people.

About the principles

Before diving into the first principle, it’s worth briefly describing what these principles are and what they’re not.

These are high-level, general principles that aim to guide approaches to writing code in a way that is language/framework/technology agnostic. They should be seen more as rules of thumb or guidelines with plenty of room for exceptions and caveats depending on the situation. A good comparison might Effective Java by Joshua Bloch where a statement like “Favor composition over inheritance” doesn’t rule out ever using inheritance, but aims to guide the reader to understand why – in some cases – inheritance can cause problems and composition may provide a more robust and flexible solution.

These principles are not a style guide – our individual project teams are self organising and perfectly capable of enforcing their own code style preferences as they see fit – nor a dogmatic, stone-carved attempt at absolute truth. They’re also not strongly opinionated hot takes that are likely to provoke flame wars. They are simply what we see as sensible guidelines towards good, easy-to-write, easy-to-maintain code, and therefore robust software.

That was a lot of ado, so without any further let’s get on with the first principle.

The principle

Favour functional, immutable code over imperative, mutable code

Functional code emphasises side effect free, pure, composable functions that deal with immutable objects and avoid mutable state. We believe this approach leads to more concise, more testable, more readable, less error-prone software and we advise that all code be written in this way unless there is a good reason not to.

Code written in this way is easier to reason about because it avoids side effects and state mutations; functions are pure, deterministic and predictable. This approach promotes writing small, modular functions that are easy to compose together and easy to test.

67 Bricks has a history of favouring Scala as a development language – which may be clear from browsing back through the history of this blog. While these days C# has become a more common language for the products we deliver, the functional-first spirit of Scala is still woven into the fabric of 67 Bricks development. I believe Martin Odersky’s Coursera course: Functional Programming Principles in Scala is an excellent starting point for anyone wanting to understand the functional programming mindset regardless of your interest in Scala as a language.

As an interesting aside, the implementations of many of the Scala collections library classes – such as ListMap and HashMap – use mutable data structures internally in some methods, presumably for purposes of optimisation. This illustrates the caveat mentioned above that there may be sensible, situation-specific reasons to override this principle and others. It’s worth noting however that while the internals of some functions may be implemented in an imperative way, those are implementation details that are entirely encapsulated and irrelevant to users of the API.

I think “functional programming” is better seen as a continuum than a black and white dichotomy. While certain languages – like Haskell and F# – may be strictly functional, most languages – including C#, Javascript/Typescript, Python and (increasingly) Java – have many features that allow you to write in a more functional way if you choose to use them.

Examples

There are many books describing and teaching functional programming and the various principles that make it up, so I don’t intend to go into too much detail, but I think a couple of examples may help illustrate what functional code is and why it’s useful.

The following is an example of some code that does not follow this principle:

let onOffer = false;

function applyOffersToPrices(prices: number[]) {
  onOffer = isOfferDate(new Date());
  if (onOffer) {
    for (let i = 0; i < prices.length; i++) {
      prices[i] /= 2;
    }
  }
  return onOffer;
}

const prices: number[] = await retrievePricesFromSomewhere();
const onOffer = applyOffersToPrices(prices)
if (onOffer) {
  // ... what values does `prices` contain here?
} else {
  // ... how about here?
}

This code is hard to reason about because applyOffersToPrices mutates one of its arguments in some instances. This makes it very hard to be sure what state the values in the prices array are in after that function is called.

The following is an example that attempts to follow the principle:

function discountedPrices(prices: number[], date: Date) {
  if (!isOfferDate(date)) {
    return prices;
  }
  return prices.map(price => price / 2)
}

const prices: number[] = await retrievePricesFromSomewhere();
const todayPrices = discountedPrices(prices, new Date());

In this example, applyOffersToPrices is a pure function that does not mutate its input, but returns a new array containing the updated prices. It is unambiguous that prices still contains the original prices while todayPrices contains the prices that apply on the current date with the offer applied as necessary.

Note also that discountedPrices has everything it needs – the original prices and the current date – passed into it as arguments. This makes it very easy to test with different values.

Resources

Functional Programming Principles in Scala – Martin Odersky on Coursera

Why Functional Programming