The best tool for the job is the tool you know how to use

The best tool for the job is the tool you know how to use

A recurring cliche at tech companies is that they use the “right tool for the job”. This is meant to show that the company is pragmatic, and not dogmatic, about the technology they use. It’s supposed to be a “good thing”.

for example – “We’re predominantely a nodeJS shop, but we also have a few microservices in golang”. Or (worse), “We let each team decide the best technology for them”.

I don’t agree with that approach. There are benefits realised by using, say, golnag in the right context. But, they are dwarfed by some not-so-obvious problems.

A “bad tool” used well is better than a “good tool” used badly

In most cases, an organization has a deep understanding, experience, and tools in a specific technology.

Suppose a use case arises where that specific technology isn’t the best fit. There’s a better tool for that class of problems.

I contend that the existing tech stack would still perform better than an “optimal”, but less known, technology.

there are two sides to this argument –

1. The “bad” tool isn’t all that bad

Most tech stacks these days are extremely versatile.

You could write embedded systems in javascript, websites with rust, even IoT in ruby..

It wouldn’t work as well as the the tools that are uniquely qualified for that context. But it can take you 80% of the way there. And, in 80% of cases – that’s good enough.

2. The “good” tool isn’t all that good

I mean – the tool probably is good. Your understanding of it, is not.
How to use it, best practices, common pitfalls, tooling, eco-system, and a million and one other things, that are only learned through experience.

You would not realise the same value from using that tool as someone who’s proficient in it.

Even worse – you’ll likely make some beginner mistakes.

And you’ll make them when they have the most impact – right at the beginning, when the system’s architecture is being established.

After a few months, you’ll gain enough experience to realise the mistakes you’ve made. But By then it’ll be much harder, or even infeasible, to fix.

There are some other issues with using a different technology than your main one:

Splitting the platform

Your organization has probably built (or bought) tooling around your main tech stack. They help your teams deliver faster, better, safer.

These tools will not be available for a new tech stack.

New tools, or ports of existing tools, will be required for the new tech stack.

The choice would be to either:
Invest the time and resources in (re)building (and maintaining) ports of the existing tools for that new technology, OR
Let the team using the new technology figure it out on their own.

In either case, this will result in a ton of extra work. Either for the platform / devX team (to build those tools), or for the product team (to solve boilerplate problems that have already been solved for the main tech stack).

Splitting the people

There’s a huge advantage to having a workforce that are all focused on a single tech stack. They can share knowledge, and even code, very easily. They can support each other. Onboarding one team member into a different team is much easier.

That means that there’s a lot of flexibility whereby people are able to move around teams. Maybe even on temporary basis, if one team is in need of extra support.
This is made much more difficult when there are different technologies involved.

Hiring may become more difficult also, if different teams have vastly different requirements.

What can I do if my main tech stack really is unsuitable for this one particular use case?

A former colleague of mine, in a ruby shop, had a need to develop a system that renders pixel-prefect PDFs.
They found that ruby lacked the tools and libraries to do that.
On the other hand – java has plenty of solid libraries for PDF rendering.

So they did something simple (but genius)- they ran their ruby system on the JVM.
This allowed them to use java libraries from within ruby code.
Literally the best of all worlds.

This is not unique to my colleague’s case, though.
You can run many languages on the JVM, and benefit from the rich java echosystem.
You can call performant C or Rust code from ruby, python, .NET, etc.

It’s possible to use the right tool at just the right place where it’s needed, without going ‘all-in’.

What can I do if I can’t get away with using my familiar tools?

Your existing tools probably cover 80% of all cases. But there will always be those 20% where you simply have to use “the right tool”. So let’s think about how to mitigate the above drawbacks of using an unfamiliar tool.

The most obvious option is to buy that familiarity: Bring in someone from the outside who’s already proficient with this tool. This can be in the form of permanent employees, or limited-time consultancy / contractors.

There’s a problem with any purchased capability, though.
They may be an expert in using the tool, but they are complete novices in using it in your specific context.
While they won’t make the beginner mistakes with the tool, as mentioned above, they’ll likely make beginner mistakes regarding your specific domain and context.

For this reason, I’d try and avoid using the consultancy model here. Firstly – they won’t have enough time to learn your domain. Secondly- your team won’t have enough time to learn the tool, to see where it doesn’t fit well with your domain.

Even hiring in full-time experts should be done with caution. They, too, will have no knowledge of your specific business context to begin with.
It may seem like a good idea to hire a whole team of experts, that can get up and running quickly. But consider pairing them with existing engineers with good understanding of your product and domain. The outside experts can level-up the existing engineers on the technology. The existing engineers can help the experts with context and domain knowledge.

It may seem slower to begin with, but can help avoid costly mistakes. And has the benefit of spreading the knowledge of the new tech stack, raising its bus factor.

Exceptions to the “rule”

Like any made-up-internet-advice, the position I outlined above is not a hard and fast rule.
There are cases where it would make complete sense to develop expertise in a technology that is not your core competency.

The most obvious example would be a new delivery method for your services: If you want to start serving your customers via a mobile app, for example. Then building the knowledge and tools around mobile development makes perfect sense.
Or creating an API / developer experience capability, if you want to start exposing your service via a developer API / SDK.

Or, if you’re a huge organization with thousands of developers. You’ll naturally have so many employees that have prior experience with different technologies. In that case you may find many of the issues outlined here do not apply.

In summary

Going all-in on a technical capability can have many benefits.
Richness of tools, flexibility of developers being able to move around different codebases, knowledge sharing, and more.
It makes sense to try and preserve that depth of expertise, and not to dilute it by bringing in more technologies and tools into the mix. Today, with every technology being so broad and multi-purpose, it’s easier to do than ever.

And remember – “select isn’t broken“: Many times I thought that some technology “cannot do” some task. Only to find out that it can, actually do that. It was just that I couldn’t.

Stop lying to yourself – you will never “fix it later”

Stop lying to yourself – you will never “fix it later”

Recently I approved a pull request from a colleague, that had the following description: “That’s a hacky way of doing this, but I don’t have time today to come up with a better implementation”.
It got me thinking about when this “hack” might be fixed.
I could recall many times when I, or my colleagues, shipped code that we were not completely happy with (from a maintainability / quality / cleanliness aspect, sub-par functionality, inferior user experience etc.).
On the other hand, I could recall far far fewer times where we went back and fixed those things.

I’ve read somewhere (unfortunately I can’t find the source) that “The longer something remains unchanged, the less likely it is to change in the future”.
Meaning – from the moment we shipped this “hack”, it then becomes less and less likely to be fixed as time goes on.
If we don’t fix it today, then tomorrow it’ll be less likely to be fixed. And even less so the day after, the week after, the month after. I observed this rule to be true, and I think there are a few reasons for it.

Surprisingly, it isn’t because we’re bad at our jobs, unprofessional, or simply uncaring.
It’s not even because of evil product managers who “force” us to move on to the next feature, not “allowing” us to fix things.

There are a few, interconnected, reasons:

Loss of context and confidence

The further removed you are from the point where the code was written, the less you understand it. You remember less about what it does, what it’s supposed to do, how it does it, where it’s used, etc.
If you don’t understand all its intended use cases, then you’re not confident that you can test all of them.
Which means you’re worried that any change you make might break some use case you were unaware of. (yes, good tests help, but how many of us trust their test suites even when we’re not very familiar with the code?)

This type of thinking leads to fear, which inhibits change.
The risk of breaking something isn’t “worth” the benefit of improving it.

Normalization

The more you’ve lived with something, the more used you are to it.
It feels like less and less of a problem with time.

For example – I recently moved house. In the first few days of unpacking, we didn’t have time or energy to re-assemble our bed frame.
It wasn’t a priority – we can sleep just as well on a mattress on the floor. There are more important things to sort out.
We eventually did get round to assembling it. SIX MONTHS after we moved in.
For the first few days, it was weird walking past the different bits and pieces of bed; laying on the floor.
But we got used to it. And eventually, barely thought about it.

Higher priority

This is a result of the previous two reasons.
On the one hand, we have something that we’re used to living with, which we are afraid to change.
We perceive it as high risk, low reward.
On the other hand, we have some new thing that we want to build / improve. There’s always a new thing.
The choice seems obvious. Every single time.

You’re now relying on that bad code

Even though we know that this code is “bad”, we need to build other features on top of it.
And we need to do it quickly, before there’s a chance to fix this “bad” code.
So now we have code that depends on the “bad” code, and will probably break if we change the bad code.

For example, we wrote our data validation at the UI layer. But we know that data validation should happen at the domain layer. So we intend to move that code “later”.
But after a while, we wrote some domain-level code, assuming that data received from the UI is already valid.
So moving the validation out of the UI will break that new code.

A more serious, and “architectural” example:
“We’re starting out with a schema-less database (such as mongoDB), because we don’t know what our data will look like. We want to be able to change its shape quickly and easily. We can re-evaluate it when our data model is more stable”.
I’ve worked at 3 different companies that used this exact same thinking. What I found common to all of them is:
1. They’re still using mongoDB, years later. 2. They’re very unhappy with mongoDB.
But they can’t replace it, because they built so much functionality on top of it!

What’s the point, then?

So, we realise that if we don’t fix something right away, we’re likely to never fix it. So what? why is that important?

Because it allows us to make informed decisions. Up until now, we thought that our choice was “either fix it now, or defer it to some point in the future”.
Now we can state our options more truthfully – “either fix it now, or be OK with it never being fixed”. That’s a whole new conversation. One which is much more realistic.

We can use this knowledge to inform our priorities:
If we know that it’s “now or never”, we may be able to prioritise that important fix, rather than throwing it into the black hole that is the bottom 80% of our backlog.

We can even use this to inform our work agreements and processes.
One process that worked pretty well for my team in the past, was to allocate a “cleanup” period immediately after each project.
The team doesn’t move on to the next thing right away when a feature is shipped. But rather, it has time to improve all those things that are important, but will otherwise never be fixed.

If we keep believing that a deferred task will one day be done, we’ll never fix anything.
If we acknowledge that we only have a small window of opportunity to act, we can make realistic, actionable plans.

(Case in point: I thought about writing this post while in the shower yesterday. When I got out, I told my wife about it. She said “Go and write this post right now. Otherwise you’ll never do it”.
And she was right: Compare this, written and published post, to the list full of “blog post ideas” that never materialized. I never “wrote it later”
I’m going to take my own advise now, and delete that list entirely.)

Business analysts hate him: how to get the business to give you the correct requirements right from the start

Business analysts hate him: how to get the business to give you the correct requirements right from the start

In a recent episode of the advice show “soft skills engineering”, a listener described some of their reasons for hating their job:

developers who need me to Google for them, business people who don’t understand how to provide requirements

I’d like to focus on “business people who don’t understand how to provide requirements”.
That’s an issue that used to frustrate me to no end earlier in my career.
I would receive a work ticket describing functionality to be implemented, complete with a UI design mockup.
I would go into my hole, and emerge a few days / weeks later with everything working as described.
Then, to no-one’s surprise, I would be assigned a new task. “move that button from the bottom of the screen to the top”. Or “limit this set of functionalities to admin users only”. Or “for premium users, the calculation should be different”.
And then, also to no-one’s surprise, I’d get annoyed.

“But you signed off on the mockup that had the button at the bottom!” or “Now I have to completely re-write the permissions mechanism for this!” “What’s changed between when you wrote this spec, and now??! Couldn’t you have thought a bit harder and given me the correct requirements the first time?”

It seems that the question asker and I are kindred spirits.

The first thing podcasts hosts Dave and Jamison said is this: If the question asker were to change jobs, they would find that this is still happening at their new job as well.

Too right, I say! Aren’t those business people stupid, hurr durr…

They also said –

No matter how clearly they document requirements, there’s always some ambiguity.

It’s unreasonable to expect business people to come to you with even somewhat fleshed-out correct requirements

Developers underestimate how extremely challenging it is to translate problems that people observe in the real-world into software requirements that can be implemented

You need to talk to them [people who provide requirements] more. If they give you requirements that you are concerned about, you just talk to them about it and ask them:
Why do they think this is the right thing? how certain they are that the requirements aren’t going to change? what if it takes longer than they expect?

These are all great observations. But I’d like to expand on them a little bit, since this is an important issue that makes life miserable for many developers.


So now, I’m ready to reveal the secret. Something that I’ve learned after many, painful experiences as a software developer. Are you ready?

How to get business people to figure out what they actually want

Before we dive into the answer, let’s try a few thought experiments. These are designed to get us into the head of the business people, and try and think like they think (don’t worry! it’ll only be temporary. You’ll be back to thinking rationally in a minute).

Experiment #1: Have you ever changed your mind about something?

Has it ever happened that you thought that one thing was the best option, but, upon seeing what it actually looks like, you realised it wasn’t? For example –

  • You thought that the big armchair would look best next to the chaise-longue, in the drawing room. But then you realised that it was the most comfortable chair in the house, so it should go in front of the TV.
  • Or, you thought that there is no way that sweet pineapples belong on savory pizza. That’s insanity! But then, when you tasted it this one time, it totally worked!
  • Or, you thought it would be a killer photo if you and your boyfriend pretended to hold the leaning tower of Pisa up between you. But when you posted it on instagram, it got no likes at all.

Do you see where I’m going with this? It’s impossible to know whether something would work for you before actually trying it.
If you’re still not convinced try this one –

Experiment #2: Can you provide the right requirements upfront?

Imagine a software development process consisting of these stages:

  1. Write requirements
  2. Create software design (e.g. classes and modules) based on those requirements
  3. Implement the design

Now imagine that you’re responsible for step 2: Given a set of requirements, you provide a set of classes and functions to satisfy those requirements.
Do you think the resulting implementation in step 3 is going to 100% reflect your design?
Or will you find edge cases, unforeseen complications, wrong assumptions, which would mean that the implementation needs to deviate from the design?

The simple truth is that, like Dave and Jamison hinted, there’s no such thing as “the correct requirements upfront”.

So what can we do?

This is a solved problem

20 years ago, a bunch of smart dudes got together, and wrote a manifesto for better software development. Here it is, in (almost) its entirety:

  • Individuals and interactions over processes and tools
  • Working software over comprehensive documentation
  • Customer collaboration over contract negotiation
  • Responding to change over following a plan

How does that help us to get the actual right requirements out of those finicky business people?
Let’s see:

  1. Customer collaboration over contract negotiation: don’t waste your time writing specifications that you know will be inaccurate. Instead, find out who has the answers, and work together with them to find the right solution.
    How?
  2. Individuals and interactions over processes and tools: don’t invest time and effort into creating a process for generating and validating requirements.
    Instead – talk to the actual subject matter experts. Get them to explain their problem to you.
    Understand their pain points, and points of view. Raise any questions immediately to them. Get their feedback early and often.
    Get their feedback on –
  3. Working software over comprehensive documentation. Remember – you can’t tell what that Hawaiian pizza tastes like just by reading the recipe. you need to actually try some.
    If you do all that, then you’ll be
  4. Responding to change over following a plan: change is inevitable. Even if you think you like that sofa on that side of the room, you might realise it looks much better on the other side.
    Change will happen. You can choose, like me, to be grumpy and bitter about it. Or, you can accept it, and make sure that it causes you the least amount of disruption and pain.

Some practical tips

Here are some ways I’ve had success putting these principles into practice –

  1. Cut out the middlemen – find out who the actual users / subject matter experts are.
    Hint: They are not your product manager / business analyst.
    Talk directly with the people who personally experience the business problem. Understand what you’re actually trying to solve.
  2. Get the business people engaged – in the first step, you’ve found the people who care most about getting this business problem solved. You’ve listened to them in earnest. Most chances are that they now trust that you care about their problems as well.
    It then hopefully shouldn’t be difficult to get them to commit to engage with you.
    Ask them to be available for reviews, questions, etc. Explain that, this way, you’d be able to build what they need faster and better.
  3. Feedback – get their feedback early and often.
    Invite them to your story refinement / estimation meetings. Have regular demos of (some) working software. Change your upcoming tasks based on what they tell you.
  4. Embrace change – changes will happen. When they do, be thankful that you caught it as early as you did.
    If you have a good understanding of the business problem at hand, you should also understand why that change happened. That should make it a bit easier to stomach.

* for cases where you don’t know your users (e.g. a public-facing product), this process will have to change a bit. You’d still want someone who’s a subject matter expert (this may very well be your product manager). Together, find ways to get users feedback without being able to talk to all the users.
For example – talking to a sample set of users, using A/B tests.

The nice thing about these steps is that anyone can take them. You don’t have to be a manager / lead: All you’re doing is talking to some of your colleagues in other departments.
Unless you work for a very broken organization, nobody would tell you off for simply talking to other people (if they do – consider changing jobs…)

Closing thought: But I’m a developer, why are you asking me to do a PM / BA’s job??

That’s a common misgiving developers have to the approach I presented above.
So, what do you think is a developer’s job? If it’s only about converting requirements into software, then our job description would be something like:

Write code to fulfill given requirements

Maybe that’s what you think your job is. And, for junior developers, it probably is.
I propose, though, that for senior developers, this job description is more accurate:

Provide business value by creating software

How is this different from the previous definition? The replacement of “requirements” with “business value” is subtle, yet profound.


Imagine that you 100% accurately implement the given requirements.
But then, no user is buying that functionality. Or, the internal users still use excel to perform this task, because your solution doesn’t cover all cases.
You have fulfilled all requirements. But have you done your job?
I’d say no. You’ve created software, but that software hasn’t provided much value.

As we progress in our careers, we’re expected to have a larger impact on our company, to justify that larger paycheck we’ve come to expect.
And it’s not only on the ‘how’: less buggy, and more maintainable, software. It’s also on the ‘what’: making sure that the software we create serves the needs of our employer.

Testing anti-pattern: The soviet police-station

Testing anti-pattern: The soviet police-station

There are many good reasons to write automated tests:

They can help drive, or provide feedback on, the implementation.
They act as living documentation / proof of what the program does.
They prevent us from putting bugs in front of users.

But, to me, the most valuable purpose of automated tests are the confidence that they inspire.
When I have a suite of good automated test, I’m confident it will tell me about any bugs I introduce before my users do.
It gives me the confidence to change my code without worrying about breaking it.
In other words – the tests help confident refactoring (defined as the changing of the code’s internals, to make it more readable / extensible etc., without changing its behaviour).

That’s why I was quite surprised when my colleague, Nitish, pointed out how the tests I was writing were hindering, not helping, refactoring.
Here’s a short,completely made up, example:

As part of our e-commerce application, we have a shopping cart, where we place items. We can then calculate the total amount to be paid for all the items in the cart:

class Cart
  def total
    cost = @items.map(&:price).sum
    shipping = Shipment.new(@items).cost
    return cost + shipping
  end
end

fairly straightforward: the total to pay is the sum of all items’ prices plus shipping costs.
The calculation of shipment method and cost is delegated to the Shipment class.

class Shipment
  def initialize(_items)
  end
  def cost 
    15 # TODO: some complex logic that takes into account number, and size of, items
  end
end

Now, as is inevitable, some evil “business” person decides to ruin our beautiful code.
They tell us that premium users will get a discount if they purchase more than 5 items.

OK, so technically, this means that the code to handle Shipment now has to know about the user that owns the cart. Let’s make that change.
Staying true to Kent Beck’s “make the change easy, then make the easy change”, and working in small, safe steps, we make a small change:

class Cart
  def total
    cost = @items.map(&:price).sum
    shipping = Shipment.new(@items, @user).cost # provide user object to shipment
    return cost + shipping
  end
end

class Shipment
  def initialize(_items, _user)
  end
  def cost 
    15
  end
end

This is a textbook refactoring: the behaviour of the code hasn’t changed. It’s not even reading the new user parameter yet. It returns the same result as it did before.

However, a test now fails:

#<Shipment (class)> received :new with unexpected arguments

Let’s have a look at that failing test:

RSpec.describe Cart do
  describe '#total' do
    before do
      # `Shipment` is a complex class; 
      # we don't want the tests for `Cart` to fail whenever `Shipment` changes
      # It's enough to just validate that the correct interaction happens
      fake_shipment = instance_double(Shipment, cost: shipment_cost)
      allow(Shipment).to receive(:new).with(items).and_return(fake_shipment)
    end

    let(:items) { [Item.new(price: 5), Item.new(price: 10)] }
    let(:shipment_cost) { 20 }

    it 'updates the total to be the sum of all prices plus shipping costs' do
      expect(Cart.new(items).total).to eq(5+10+20)
    end
  end
end

Can you spot the problem here?

The fake Shipment object that is set up in the test doesn’t yet account for the new user argument.
No problem, just add some more mocking and we’ll be off on our merry way:

allow(Shipment).to receive(:new).with(items, user).and_return(fake_shipment)

That’s what I’ve done for years. Often, with much more complex class interactions, requiring many more changes to tests. Until Nitish pointed out that this is a waste of time.

My tests were essentially a copy-paste of my production code.
I only sprinkled some doubles, allows, and expects on top.
My tests were not going to find bugs, because all interactions were faked and will always return a constant value.
And, as we just saw, they were getting in the way of, rather than helping with, refactoring.
He called it the “Soviet police-station” style of testing.
The process to work with these tests is like so:

  • Refactor production code.
  • Observe some tests go red.
  • Take those tests to the back room.
  • Beat the sh*t out of them to make them look like the production code.
  • Tests finally confess to being green.

In other words – when a test fails, the “solution” is to copy over the implementation into the test, rather than fix an actual bug.

what purpose do those tests even serve?

They may have helped us drive the implementation at one point.
But they’re documenting the implementation, rather than what the application actually does.
They will certainly never find any bugs, because they’re just faking everything.

They will either stay green forever, or, if they do go red – only make us beat them up again rather than fix anything in our application.

Test behaviour; not functions

What’s the alternative to the soviet police-station type of useless testing?
We can verify the desired behaviour of the system, rather than individual functions and methods.
For example, instead of the test

Cart 
   #total 
      is the sum of all prices plus shipping costs

we can have something like

Shopping 
   inserting an item into the cart 
      updates the total to be the sum of all prices plus shipping costs

What’s the difference?

We’re not interested in testing a specific method (total) on a specific class (Cart). This is an internal detail of our implementation. It’s not what the users of our software care about.
What they do care about is, when they put an item into the cart, then they can see an updated total amount.
So long as this holds true, then the implementation details are irrelevant.
Our test should remain green, regardless of the implementation we choose.

(There’s much more to be said about this, and Ian Cooper said it much better in this life-changing conference talk.)

How does this work in practice?

What would be a good implementation of a test that verifies the application’s behaviour?
Well, that depends on our architecture.
If we use something like ports and adapters / hexagonal architecture, then our application’s control flow would look something like:

user request (e.g. click on UI, API request) --> adapter (e.g. web controller) --> port (e.g. domain service) --> core application code

In this case, a natural place to test the logic of our application will be the port.
The port exposes an API to accomplish a certain task (such as “add item”, “remove item” or “get cart total”). This is exactly what the user is trying to do.
It’s also decoupled from any fast-changing presentation / transport concerns.
Testing at the port level allows us to test the entire flow of a user request, which is just what we want:
We want to test the behaviour of the application, as seen by the user.

What if I’m using a different architecture?

then you’re wrong, and you should re-architect your application.
Just (mostly) kidding!

For other architectures, we’ll need to figure out where our “business logic” begins.
This is the point in the code that we want to verify.
It’s quite possible that this beginning is intermingled with presentation concerns. (for example: controllers in Rails’s implementation of MVC).

In this case, one option is to bite the bullet and test everything – the business logic, intermingled with other logic.
This is my first technique when using Rails – I use the built in methods for testing Rails controllers.
And it works reasonably well. It can break down in a couple of instances:

  1. When the ‘non-business’ (e.g. presentation) logic keeps changing.
    For example – the calculation doesn’t change, but the HTML generated by the controller does.
    In this case, we’re forced back to our old role as soviet cops, beating up the tests until they pass.
  2. Performance issues.
    Operations such as serializing JSON or rendering HTML may be time-consuming. If these are included as part of the executed code in tests, then the tests may become slow.

To solve those issues, it’s possible introduce a “port” (for example a “domain service” or “action” class). This leaves behind the presentation (or whatever) logic, and encapsulates all ‘business’-y logic in a more testable class.

P.S. we can still write isolated unit tests

The fact that we prefer behavioural tests doesn’t mean that we can’t also test in isolation, where it makes sense:

  1. complex pieces of logic may be easier and clearer to test separately. (think about the logic to calculate shipment costs, above. it could depend on a huge amount of variables, such as number of items, their size, who the user is, delivery address…)
  2. If we want to use mocks and stubs to drive our design (aka the “London” school of TDD).
    That’s a useful technique for discovering the implementation, and how the class design and interactions may look.
    Personally, I’d consider deleting these tests afterwards. They’ve already served their purpose of guiding our design. From now on, as we’ve seen, they will only serve to hinder changes to that design. They’re certainly not going to find any bugs.

Closing words

Tests should be an enabler for well-maintained code.
Tests should give us confidence that, if they are passing, then the application works as intended.
Additionally, they shouldn’t get in the way of changing the code.

Tests that verify implementation hinder us from changing our code, as well as being less confidence-inspiring.
Tests that verify behaviour enable us to change and evolve our code, as the application’s functionality, and our understanding of the domain, evolve.

Mock-driven development

There are lots of different way to fake behaviour in tests -
you've got your mocks, your spies, your stubs, your doubles etc. 
They are all (subtly) different. 
So let me say, right off the bat - I can never remember which is which. 
I'm going to use these terms interchangeably, and in some cases - wrongly. 
If you think this stuff is important, please let me know in the comments.


A colleague of mine invited Tim Mackinnon to an informal drink at our office one day. He introduced Tim is “the co-discoverer of mock objects”.
I felt that it was only common courtesy that I read the man’s work before meeting him.
Reading how Tim and his colleagues used mock objects to make the tests guide the design of their application was eye opening.

How it works (as I understand it)

In very short – the process, termed “needs-driven development” (but I call it “mock-driven development” because, why not?), is this:

  1. Tests for a particular functionality are written first, before implementation. So far this is good ol’ TDD.
  2. Unlike ‘classic’ TDD, we don’t make the test pass by implementing the functionality. Instead, we discover what the code under test needs from the ‘outside world’ to make the test pass.
    Meaning – what are the dependencies that the code under test needs to have in order to do its job.
  3. We then use test fakes to simulate those dependencies, without implementing any of them.

An example from the above article

The authors set out to implement a cache class that falls back to loading objects from the database, if they are not cached.

The first test is: Given that the cache is empty, when key A is requested, then object A is returned.
However, rather than being satisfied with black-box testing such as expect(cache.get('keyA')).to eq(objA), the authors stopped to think not only about the interface (tested above), but also about the design.
They realized that the cache object would need to fetch the objects from storage (as they’re not cached). They identified “fetching from storage” as a distinct responsibility. This responsibility can (and should) be implemented by a separate object.
They named this object “object loader”. And they’ve coded this into their test: expect(object_loader).to receive(:fetch).with('keyA').and_return(objA)
That meant that they were able to get this test to pass without worrying about how to actually fetch objects from storage. All they had to do was find a way to get a fake object loader into their cache class.

The complete test case looks something like this –

it 'loads an object from storage when the cache is empty' do   
  object_loader = double('object loader')
  expect(object_loader).to receive(:fetch).with('keyA').and_return(objA)
  cache = Cache.new(object_loader)
  expect(cache.get('keyA')).to eq(objA)
end

Then, they were able to continue focusing on further functionality of the cache (like caching, eviction policies etc.), without getting sidetracked.
I recommend reading the full article – it’s not nearly as academical and scary as it looks.

A new(ish) way to look at things

I never used test fakes like that – I’ve always used them after the fact.
Either when writing tests after I’ve written implementation (insert wrist-slap here), or as a way to speed up slow tests by faking-out the slow bits.


When writing code, I’m always trying to think as ‘dumbly’ as possible. As in – when writing class A that uses class B, I pretend like I have no idea how class B works. Looking only at B’s interface helps me decouple A from B, and helps me detect any leaky abstractions that B may expose.
That’s why the ‘mock driven development’ approach suggested by Tim et al., appealed to me. The notion of “I know I need an X here, but I don’t want to actually think about X for now” is exactly how I like to think.

A simple example

I’ll describe how I used this process to implement a repository that uses AWS S3 as its backing storage.
The required behaviour was: take an object as input, and write it to an S3 bucket.
I started out by defining the steps in the process: serialize the object to string, connect to an S3 bucket, write string to bucket.
By identifying the different steps, the design became apparent.
Using fakes, I was able to write tests that verified these steps:

it 'uses a serializer to serialize the given object' do
    input = MyObject.new
    serializer = double('serializer')
    expect(serializer).to receive(:serialize).with(input)
    
    described_class.new(serializer).save(input)
  end

(Note: I’m using dependency injection to enable me to stub-out the serializer object.)
The only thing I know after writing this test is that the input is serialized using something called `serializer`, that has a `serialize` method. That’s the first step, and I can write some code to implement that.

Now to implement the next step: connect to an S3 bucket

it 'connects to an S3 bucket' do
    bucket_factory = double('bucket factory')
    expect(bucket_factory).to receive(:get_bucket)
    input = MyObject.new
    serializer = double('serializer')
    allow(serializer).to receive(:serialize)
    described_class.new(serializer, bucket_factory).save(input)
   end

Here I realized that connecting to an S3 bucket is its own responsibility, and opted for a factory design pattern. Again, I don’t care at this point how this factory object works.
This was not my original design, though – using a factory was not my first thought.
I actually started by writing a test that verified that the repository connected to S3, by stubbing some of the AWS SDK classes.
However, I found the test setup too verbose and complex. That made me realize that the implementation would be too.
A verbose and complex implementation suggests that there’s extra responsibilities to tease out.
(Also – they say “don’t mock what you don’t own”. So, in any case, it would’ve been better to wrap the AWS-specific stuff in a wrapper class and mock that wrapper class)

I realized all that without writing a single line of implementation code. All it took to change my design was changing a few lines of test code; it cost me nothing.
By abstracting-away the interaction with AWS I made my life much simpler, and I was able to stay focused on the original steps defined above.
Later, when I went to implement the factory class, my life was pretty simple, again. I could ignore everything – serialization, coordination etc., and concentrate only on connecting to S3.

The final step is to use the bucket returned from the factory to write the string returned by the serializer:

it 'saves the serialized object to an S3 bucket' do
    bucket_factory = double('bucket factory')
    bucket = double('S3 bucket')
    allow(bucket_factory).to receive(:get_bucket).and_return(bucket)
    input = MyObject.new
    serializer = double('serializer')
    allow(serializer).to receive(:serialize).and_return('serialized object')
  
    expect(bucket).to receive(:put).with('serialized object')
    described_class.new(serializer, bucket_factory).save(input)
  end


And that’s the repository tested!
Now, take a second to try and imagine what the code that satisfies these tests looks like. Don’t peek below!

Is this what you had in mind?

class Repository
  def initialize(serializer, bucket_factory)
    @serializer = serializer
    @bucket_factory = bucket_factory
  end

  def save(obj)
    serialized = @serializer.serialize(obj)
    bucket = @bucket_factory.get_bucket
    bucket.put(serialized)
  end
end

Analysis

The resulting code looks laughably simple.
Why did I go to all that trouble with faking, testing etc. just for this, frankly trivial piece of code?
The reason is – those tests aren’t the artefacts, or the result, of my coding process. They are the coding process.
If I hadn’t used them to guide my coding, my code would’ve looked completely different. here are some of the advantages I saw with this process, and its result:

  • It encouraged me to separate responsibilities to other objects.
    With black-box testing it could be tempting to implement everything inside the same class. (Note: the red-green-refactor cycle of TDD should help me get a similar result, as I would’ve refactored my implementation once it was working.
    However, using mocks removed a lot of friction out of refactoring, as I only had to change little bits of test code, and no production code.)
  • I defined the APIs of the collaborating objects before even implementing them. That means that, by definition, those APIs don’t expose any implementation details. The result is very loosely-coupled code.
    (In fact, we went through several different types of serializers, but our tests never changed)
  • The tests are super fast: Finished in 0.00524 seconds (files took 0.05876 seconds to load)
  • The code and tests are isolated; the only dependency required to run the above code and tests is `RSpec` (testing library)

There is, of course, one very glaring problem:
The tests for my repository class are all passing, but it doesn’t actually persist any objects!
I still need integration tests that verify that the class is doing what it’s meant to do.
However, with extensive unit tests, the slower, more brittle, integration tests can be limited to a narrow set of scenarios.

Conclusion

For a while now, I’ve found that writing tests first helped me define my code’s interfaces more clearly.
Having tests as the first users of the code allowed me to look at it from the outside. That meant that the needs of the code’s users determine how the code is used, not any implementation details of the code itself.

Using mocks to drive the implementation takes this process one step further – I can now define other objects’ interfaces, driven by the user of those objects (which is my code under test).
In the example above, I defined the interfaces of the `Serializer` and `BucketFactory` classes while thinking of the `Repository` class.

The next thing I’d like to think about, is the long-term value of these unit tests:
Now that I have my nice, loosely-coupled design, have those tests served their purpose? Do they provide value anymore?

How to not get a job that sucks

How to not get a job that sucks

As I’m writing this, we’re in lockdown due to COVID-19. Nobody knows for sure what the world of work would look like once we’re out of this.
But we already know that the economy is going to suffer, in a major way.
This, unfortunately, means that some of us will lose their jobs, and will be looking for a new one.
A friend of mine recently wrote a post about preparing for a coding interview. She recommends “Cracking the coding interview” to prepare for algorithmic questions.
My immediate thought was: “If I’m evaluated on theoretical material that’s completely irrelevant to the day-to-day of the role I’m interviewing for, I’d rethink my application”.
I won’t go here into trying to convince you why a whiteboarding interview is a red flag (search the web for ‘whiteboard interview’ for some interesting opinions).
But it did give me the idea to write about another, often neglected, aspect of job-hunting: Evaluating a potential employer.
In this post I’ll share my process of gathering and analyzing data during the interview process. This helps me evaluate whether a particular company is a good fit for me. I’ve developed it over the course of many years, and a few very avoidable employment disasters.
I hope you find it useful.

How can you assess if a company is a right fit for you?

At its core, it’s a simple system: Like a scientist, try to gather as much evidence as possible. then try to draw conclusions based on them.

On their own, many pieces of evidence may seem trivial, unimportant, or can be excused away. But, putting them all together, they tell a larger story.
For example –
An interviewer being late to the interview isn’t too bad – we all run a little late sometimes.
Not offering you a drink of water during your 3-hour session is annoying, but not a big deal.
Being unprepared, not having read your resume beforehand, is irritating, but forgivable.
But all together paint a picture. If you aggregate all the above data points, you could conclude that, most likely, this company is not respectful of candidates.
In that case, you can deduce that the company isn’t very respectful of its employees as well.

Evidence-gathering methods

1. Questions
Those are the most obvious route: Ask your prospective employer questions that aim to gain insight into the company. Asking the right question in the right way can reveal a lot about a company.


But asking too many questions will make me seem condescending / snobbish / difficult, and they won’t want to hire me!
Think of it this way – a good company would love the opportunity to show how good they are. They won’t shy away from questions, because they have good answers that help their sales pitch.
On the other hand, being afraid of questions is an indication of the opposite.
Avoiding a hire because of a candidate’s legitimate concerns is a huge red flag. If that’s the case – be glad that you won’t work for an organization that punishes people for asking questions.

2. Observations
Those are the things that you see and hear during your interactions with the employer. The more attention you pay, the more observations you make, the more evidence you’ll have to draw conclusions from. Observations can be about anything – the speed with which your email is answered, the phrasing of their reply, the chat you had with the receptionist while waiting for your interview, the size and shape of the office.. Any of the above can be a small piece of a puzzle that would give you a clearer picture about the company.
Making observations can be even more valuable than asking questions. Unlike answering questions, the company can’t control the narrative of your observations. Company representatives can give favorable answers, but they can’t control what you observe. Your observations are more likely to be unbiased, and more revealing.

Undoubtedly, you’ll discard some (or many) observations as insignificant or irrelevant. However, remember – you can piece together many small pieces to create a larger whole.

Not all signals will be as obvious as this

Deal breakers

Before we go into subtleties and clues, it’s worth thinking about what your line in the sand is. What you just won’t accept, no matter how good everything else seems.
For me, it’s honesty. If an employer is not being truthful, or not acting in good faith, there’s no chance I’d want to work there.
(Note: recruiters lying is par for the course, unfortunately. So this rule can’t be applied to them.. I’m talking about actual company employees)

story time:
I once interviewed for a small startup, who offered 25 PTO days a year – surprisingly above the normal 22 days.
When they sent over the contract for me to sign, however, I noticed that it stated 18 days off. When I questioned this, they said that the 25 days figure included the 7 national public holidays.
I’ve never heard of anyone calculating PTO that way. They were obviously trying to inflate their numbers to look more attractive. Even the recruiter was surprised.
And what do you know – I lived to regret not walking away from that job right there and then.

What are you trying to assess?

There are generally a few criteria that make for a good, or horrible, work experience.

  1. The company culture – what do they value and reward?
  2. How they treat employees – do they make an effort to keep employees happy?
  3. Day-to-day work – what do you actually do 9 (or 12) hours a day? and who with?
  4. Long-term future at the company – what can you expect after having served for a few years?

Different people will be looking for different things in each of these categories. (e.g company culture – relaxed? friendly? high-intensity?, day-to-day work – latest tech? fast paced? slow and steady?)
The first thing to do is to consider what you’re looking for in each of these areas. It would be good to also rank them, so you know which are more important.
For example, I prefer to work with statically-typed, compiled languages, especially C#. But, a friendly, supportive and relaxed culture is even more important to me than that.
That’s why I’m now with a company that is incredibly supportive and friendly, which I love, working in ruby on rails, which I really don’t.

Once you have a prioritized list, you can set about trying to assess potential employers based on it. For each category, let’s see how:

Culture

There are many different opinions of what culture is (personally, I think of it as the house rules, or ‘how things are done around here’). But there’s widespread agreement on what culture is not – pithy creeds printed on posters.
Try to disregard company “mission statements”, mottoes and the like. They’re what companies want you to think they’re all about. They don’t necessarily reflect reality.
(remember – “don’t be evil” is the motto of the world’s richest spying agency)

Questions / observations to gauge the company’s culture –

  1. “What does it take to succeed here?”
    I like this one because, usually, the answer to “what’s the company culture” is a regurgitation of company PR.
    But, phrased like this, an honest answer would reveal the behaviour that the company actually rewards. It may or may not be what management says they value.
  2. “What do you do for fun?”
    The details aren’t important here. Going for drinks vs playing football in the park – that doesn’t matter.
    What you’re trying to understand here is: Does the company care about employees as human beings, or as cogs in a wheel?
    Do employees want to socialize with their colleagues? Meaning – do they have good relationships?
  3. “Do you like working here, and if so – why?”
    Very open-ended – it gives employees of good companies a chance to show off all of the good stuff.
    In other cases, it gives you a chance to watch your interviewer squirm as they try to come up with a diplomatic answer.
  4. “What would you improve at the company?”
    A complementary question to the last one. Don’t expect to get all the dirt on the company. Instead – observe the answer. Is it honest and truthful? Or some obvious platitude?
    That will tell you a lot about whether employees feel free to speak openly at this company.
  5. “How’s the work-life balance?”
    In case, like me, you care about having a life outside the office. “we work hard and we play hard” == “you work hard while we play hard”. Run away.
  6. “How are differences of opinions settled?”
    This can tell you whether the company is very hierarchical. How openly employees are able to communicate. And generally – how communication works in the organization.

How they treat employees

  1. Look at the facilities
    I read an account by a woman who, at every interview, would go to the ladies’ bathroom and look at the tampons they had. If they were the horrible cheap ones – she’d know that the company doesn’t care about employees’ welfare.
    You can tell a lot just by looking around – are the office and bathroom clean and well maintained? are employees provided with an adequate working environment? are everyone sat squished together in an awful open-space? do they have access to a decent coffee machine, or are they only provided with some plastic-y coffee vending machine? All these are an indication of how much the company values its employees.
    Note: Especially in tech, try to look beyond the gimmicks. A ping-pong table and a fridge full of coke may be nice, but ultimately don’t make or break your working experience. Dig deeper.
  2. “How do leaders support employees?”
    What you’re actually asking is: “Are employees considered drones, here only to serve management? or do you actually care about their needs and wants?”
  3. “What’s the employee turnover?”
    Mostly you won’t get a straight answer here, but observing the subtext of the answer can be useful.
  4. What’s the interview / onboarding process like
    Do they demonstrate a lack of respect for your time? (hours and hours of interviews, hours of unpaid work aka “take home tests”)? Are they testing for skills that aren’t relevant to the job? (remember “cracking the coding interview”?)
    An interview is a two-way street, and the company knows that they have to make a good impression too. If they screw up when they’re supposed to show their best side, do you think things will be better when you’re actually employed there?

story time:
I was once interviewing with a startup that seemed very nice. I met with the engineering manager and several engineers, who all made a very good impression.
I accepted an offer from them, and was requested to start about 4 weeks later. I wasn’t working at the time, so I requested to start sooner.
The engineering manager told me to contact HR; HR told me it was up to the engineering manager. I went back and forth like this for a while. In the end, I couldn’t get an earlier start.
On my first day, they didn’t have my laptop ready – I had to spend a couple of days shadowing another engineer instead.
They also had trouble finding a place for me to sit – I ended up with a weird desk-sharing situation.
All the above are certainly annoying, but, I kept thinking “This isn’t really a deal-breaker.” Especially considering the great impression I got from the engineering manager.
I later learned the reasons for these differences: The engineering manager was desperately trying (and ultimately, failed) to maintain his department as an island of sanity. If only I had put all these signals together, I would’ve saved myself the misery of working at a certifiably pathological organization.

Day-to-day work

The culture and behaviour of the organization at-large are important. However, you’ll be spending most of your time with a small number of people, doing a certain type of work. You want to make sure that both fit you.
Remember the saying: ‘People join a company, but they quit a boss’. A company can have the most amazing culture, benefits and facilities, but they all amount to nothing if you don’t fit in with your team and the type of work that it does.

  1. Try and actually meet with the team
    see if you connect with these people. Can you imagine yourself working alongside them every single day?
    For me, it’s a (slight) red flag if your interviewers aren’t actually the people you’ll be working with. this type of system means that, if you were to join the company, you’d have no say in who you work with.
    On the other hand, inviting candidates to lunch / socialize with the team is a big green flag.
  2. What does a typical day look like?”
    How many meetings / admin a day / week? are there any other duties that you’re expected to carry out?
    Story time:
    On my first day at this one job, my manager casually asked “By the way, do you know VB?”. Yeah, I forgot to ask about other duties, and they “forgot” to mention that legacy system that needs maintaining. Oops!
  3. “How does a typical task go from an idea to being in front of a customer?”
    Who decides what to do, who does it, how it gets approved, how it gets to the customer (CI / CD) etc.
    This will give you an insight into the work process. For example – how much freedom and autonomy (and responsibility) you can expect to have. Or how up-to-date their practices are.
  4. “What technologies do you use?”
    Personally, I don’t care. I worked professionally in C#, javascript and ruby. How much I liked the work was barely influenced by the tech (except for rails, of course). But, you may feel differently.
    Make sure that you’d be happy to work with the stack they have. Another very important thing is to make sure that their tech isn’t outdated.
    Perhaps you’re planning a career as a specialist Fortran programmer. In that case, you probably don’t have to worry about updating your skills. Otherwise, you’d want to leave that company having acquired some marketable skills.
  5. Look at the code
    When I’m in advanced stages, I always ask to look at some production code at the company. I’d estimate that I have a better than 50% success rate.
    This can give you a clue as to the standards of work at the company. (also, being willing to show the code to a candidate indicates that the people there are proud of their work! a big green flag)
  6. “What is something you’d improve about the way you work / the work itself?”
    That’s just a sneaky way to ask ‘what parts of the job suck?’

Long-term future

You may (or may not) be looking to stay at a company for a number of years. If that’s the case, you have to make sure that you have place to grow.
You want to leave a company having gained 10 years of experience, not one year of experience repeated 10 times!

  1. “What training opportunities does the company provide?”
  2. “How do promotions / performance reviews / pay rises work?”
    For me, anything other than a clear, well-defined process means “we’ll give you a raise if and when we feel like it”.
    Spoiler alert: much like your husband, they never feel like it.
  3. “What’s the ‘career ladder’ at the company?”
    For example – do you have to go into management in order to advance?
  4. “How often does the company adopt new technologies?”
    Their technology may be current today. But, unless the company makes a conscious effort to keep up, it (and you) will stay behind.

Conclusion

We all have different preferences. Different types of circumstances and environments that would make us happy.
The first step is to explicitly recognize what works for you (you will not get it right the first time – it took me quite a few different jobs to find out!).
Then, actively gather evidence about how the company ranks in these different areas. Put all of these pieces together, to reach conclusions you can be fairly confident of.
Then see how a prospective employer ranks in the metrics that matter to you.

Good luck!

Bonus question – COVID-19 response

Something I’ve seen online – a great question to ask employers in the future is around their response to the COVID-19 emergency.
Did the company take care of its employees? Was it understanding, proactive and fair to its customers? Did they take taxpayers’ money unnecessarily?
You can tell a lot by the way a person, or a company, behaves in a crisis.

Code review as a tool to improve quality. Part 3 – why should you care?

In the previous two posts we saw what feedback can be given in code reviews, and how best to provide it.
However, reviewing every (or most) proposed changes in your team is time consuming.
In this final post I will try to convince you that the effort of rigorously reviewing code is worthwhile.
I believe that code reviews are worth the effort because they are a cost-effective way to instill a culture of high quality of work in a team.

A communication method for technical leadership

An important effect of strict code reviews is: It changes the culture of the team.

Every team has a culture – the set of (usually) unwritten, unspoken rules, that the team abides by. If the reason for a team’s processes or work method is “that’s just how we do it”, then it’s part of that culture. even if that “rule” was never explicitly stated, or even discussed.

Examples of this implicit culture can be –
“only QA people do testing”, “a developer never works on more than one task at a time”, “every change must be covered by unit tests”, “we never deploy on Fridays”, “it’s OK to release code that wasn’t tested if the product manager deems it urgent enough”.

These rules evolve organically as team members observe what team management values – Which behaviour gets rewarded, and which gets punished (or is not allowed).
And these implicit rules are much stronger than anything that is said explicitly:
How many times did you hear from management that “quality is our top priority”? How many times did you then go back to your desk and push out a hacky change because it was urgent?
Actions (in this case – allowing quality to suffer due to ‘urgency’) speak louder than words.

That’s why the review process is a powerful way to set a culture of high quality work. The action of rejecting low-quality changes speaks louder than any talk about quality.
Rather than talk about wanting to improve quality, we actively reject sub-standard work.

In my experience, developers pick up on that culture very quickly.
A developer may get irked when they are asked to break up a long method, standardaize a component’s API, or justify why a certain method belongs in a certain logical layer. Especially if they’re not used to these things being an issue.
However, after a few times, they will adapt. They’ll simply save themselves the time of submitting changes that are bound to be rejected.

Developers will follow those best practices not only because that’s the only way to get their work done.
But also because they’re interested in improving and leveling-up. And this review process, done with teaching and mentoring in mind, helps them do just that.

“Academic” Training can help team members recognize what constitutes high or low quality work. But how many times have you come back from training, full of wonderful new ideas, only to return to your familiar way of working in your old project?
The review process is an effective and efficient tool for making a high-quality way of work stick. This is because, unlike training, it’s baked right into the team’s processes, and it’s provided in the context of the actual work being done.
(pair programming is even better, as it allows teaching and discussion in real-time. However, it isn’t as time-efficient as reviewing code.)

Why you don’t do it – awkwardness

I sometimes ask colleagues why they don’t employ a review process as suggested in this series. By far the #1 reason I hear is awkwardness.
It isn’t that they disagree with the importance of it, or with the different guidelines; It’s the human aspect that stops them:
Conducting such a thorough review ends up with dozens of questions or requests for change. People feel uncomfortable doing that.
They worry that the code author would be insulted or made to feel inadequate by a torrent of comments.

These concerns are justified. That’s why the first post of this series is dedicated to ways of avoiding that. It’s important to ensure that feedback is provided well, and taken in context, and not as a confrontation.

I can relate my personal experience of employing this method:
At my job, I submit by far the most comments on any given PR (about 5 comments from me to 1 comment from anyone else).
However, I try to do so with empathy and sensitivity.

The feedback I receive is overwhelmingly positive.
My colleagues (and me) have a positive learning experience as a result of my questions / suggestions. They appreciate the thoughtfulness and effort that went into reading their work.
Remember that your colleagues, like you, value opportunities to learn and improve. Don’t be afraid to help them.
(In fact, if you’re in a leadership position, that’s probably part of your job!)

The role of senior leadership

Improving the team’s quality of work is one of the technical leader’s responsibilities.
If employing rigorous review is a way to achieve that, shouldn’t it be a part of every technical leader’s job? Just like coordinating the technical work on a project, or attending pointless meetings?
So why isn’t it?

Many, senior management included, regard low-quality work as an “act of god”. Something that we are powerless against. Or, at least, a problem that has so many different causes that nobody can be personally accountable for it.
The manager didn’t actually write that bad code. There’s nothing they could have done, right?
Well, they could’ve at least read that code..

Imagine explaining to upper management why certain features are delayed / buggy / unmaintainable. Imagine management asking “who authorised this low-quality of work?”
That would make not rigorously reviewing code an even more awkward situation for the technical lead. Thus helping her solve the problem of awkwardness around applying rigorous reviews :).

Series Summary

Some of the largest hurdles that teams face in trying to improve the quality of the code they produce are:
Lack of awareness (we don’t know that there’s a better way to do this),
Lack of skill (we know there must be a better way to do this, but we don’t know what it is),
or Apathy (we know there must be a better way to do this, but it isn’t important enough).
(The last one, often expressed as “time constraints”, is something that I don’t know how to solve )

As a leader (or team member), you can use the review process as a time-efficient way to address the first two issues.
Reviews are a tool to raise awareness of problems (by commenting on them). They also to help improve skill (by prompting for, or working together on, a better solution).
Most importantly – they signal that code quality is important enough, by not allowing low-quality changes to go through.

Reviews are more specific and relevant than training, as they deal with the team’s actual code. They are also less time consuming than pairing.
This allows a single person to influence greater quantities of written code.
Done right, code reviews are a tool for teaching, learning, and setting culture and standards. Give them a go!

Code review as a tool to improve quality. Part 2 – What feedback to provide

Other posts in this series

Part 1 – how to provide feedback
Part 3 – why should you care?

In the previous post we discussed what a code review is, and some guidelines on how to deliver feedback. Also, importantly – what goals we should try to achieve when reviewing code .
We defined the first, most obvious (and difficult) goal as:

1. Quality control
The reviewer tries to ensure that the proposed changes meet certain bars of quality. If it doesn’t, they may suggest ways to improve the code’s quality.
(“Code quality” can be subjective, and I’ll try to define what (I think) it means in the next posts.)

Therefore, in this post, we’ll explore how to spot “low quality” code that can be improved on.

What code quality is not (AKA you’re not an inefficient, overpriced linter)

Don’t waste time checking for things that can be automated (style and formatting violations). Instead:
1. Plug a linter into your test suite / CI pipeline
2. Have a team-wide discussion about tabs vs. spaces, curly braces on their own line or not, and all that fun stuff
3. never think of it again.

Don’t distract yourself from the purpose of the current code at hand with linting.

What I would suggest you do focus on, are the fundamentals of maintainable code and design. Those include – SOLID principles, consistent architecture and design, and code smells.
I’ll briefly discuss those in this post. I’ll provide links for further reading on each topic, as each one of them is worthy of a blog post (or book) of its own. I’ll merely provide a taster for each one.

Well-known code smells

These are relatively easy to identify, so are a great starting point.
If you take just one thing from this post, take this: Familiarize yourself with some common code smells (use the excellent refactoring guru), and look out for those.
(Note: these are highly dependent on context and situation. Some smells result from fixing other smells. You’ll have to use your judgement as to what applies to the code at hand.)

Example:

public updateUserDetails(UID userId, string newFirstName, string newLastName, string newStreetAddress, string newCity, string newState) 

This method suffers from the “long parameters list” smell.
A reviewer can point out that smell, and help guide the author to its solution (possibly using a User object rather than a list of fields, or a parameters object).

Well-known code anti-patterns

These are both relatively easy to identify, and removing them provides high returns.
Familiarize yourself with some common anti-patterns, and look out for those. (Some examples – pokemon exception handling, using inheritance where composition is preferable)
The fact that they are generally agreed to be “bad” will also help you convince the code author to change them.

Example:

public isValidDate(string str) {
  try {
     var dummy = parseDate(str);
     return true;
  }
  catch (FormatException e) {
    return false;
  }
}  

The above method employs the known anti-pattern of using exceptions for flow control.
A review comment here might point out the anti-pattern, and ask the author how it could be changed.
(remember from our previous discussion – prompt the author, do not spoon-feed them)
A solution to the above example could be:
Move the logic that causes a FormatException to be thrown from parseDate to isValidDate. change parseDate to call isValidDate.

Well-known architectural anti-patterns

Similar to the above, these are cases where the proposed code changes go against the system’s architecture.
For example – tightly coupling modules that should be independent of each other. breaking well-known architectural principles (such as placing UI-related code in the domain model).
Example:

private submitButtonClicked() {
  var user = UserRepository.get(usernameField.text);
  updateResult = UserController.updateUserDetails(usernameField.text, firstNameField.text, lastNameField.text);
}

The above method breaks the system’s layer model. It invokes a repository method directly from the view.
A reviewer may point that violation out.
If necessary, she can suggest moving the call to the repository to a more appropriate layer (controller layer, for example).

Loose coupling

A component has, or makes use of, little or no knowledge of the definitions of other components.
Components that do not adhere to this principle make the system harder to change. That’s because changes to component A will need to ripple out to components B and C that are coupled to it.
Example:

var streetAddress = user.Address.Lines.split('/n')[0];

The above code is tightly-coupled to the implementation of the User’s address field. Any future changes to the implementation of Address will need to take this code into account.
A reviewer may point this out (with the goal of moving this logic into the Address object itself).

Single responsibility

A component should have responsibility over a single part of functionality
(should have one, and only one, reason to change).
Useful question to ask yourself as you review a component : “What’s the role / responsibility of this?”. Look for the word “and” in the answer.
Example:

class UsersManager {
  public search(string name);
  public update (string userId, string newName, string newEmail);
  public resetPassword(string emailAddress);
}

The above class is an example of a “god-object” ; it has many reasons to change.
update will change if the fields of the User object change.
search will change as we add more searchable fields.
resetPassword will change with any changes to our authentication mechanism.
A reviewer may suggest breaking up the different responsibilities into different objects. A common pattern to do that is employing domain service objects.

Leaky abstractions

An abstraction exposes implementation details that are not relevant to its interface.
Or: a user needs to be aware of the implementation details when using the abstraction.

Useful question to ask as you read the code: “As the user of this class / method, what do I have to know?” In the answer, look for anything that is related to “how”, rather than “what”, the component does.
Remember – to use a component, you shouldn’t have to know how it does its job.
Example:

class Payment {
  public charge(double amount, string stripeMerchantID);
}

Users of the above class have to know that charging a payment is done by stripe.
A reviewer may request that references to ‘stripe’ be removed from the class’s public API. A common way to do this is to –
1. Separate the creation and usage of instances of the Payment class (for example – using inversion of control)
2. configure the class at creation time with the necessary stripe credentials.

In that way, the actual usage of this class be free of references to the underlying implementation.

Sane and consistent APIs

Good and consistent API is such that:

  • The inputs and outputs are consistently the same structure / types.
  • No unforeseen side-effects (such as modifying an input parameter)
  • the API client doesn’t need to do special parsing to pass values in and out of the API

Examples:

newUserID = createUser(params)[0]
the createUser method’s result requires the client to do some weird parsing.
public makeCharge(Money amount, double discountAmount)
this method represent an amount inconsistently – once as a Money object, and once a plain double.

Naming

This isn’t necessarily a category in itself.
However, paying attention to how things are named can help us spot a smell of one of the other categories.
For example –
public makeChargeAndMarkAsPurchased() indicates that the method is not singly-responsible.
The name mongoCacheProvider.store(value) leaks the fact that our cache is implemented using mongodb.
The name SendRequestToStripe(amount, merchantID) indicates how the method does its job. A better name would describe what the method achieves. For example – chargeCustomer.

Test-code smells

Test code is code in your system that needs to be maintained and evolved over time. It should not be overlooked when reviewing changes.
All the above criteria also apply to test code, especially –

  • Single responsibility – a single test should verify one thing.
    a good metric is to make sure that each test contains exactly one assertion.
    Another rule of thumb is to look at how much faking / stubbing is happening in the test. If a lot of dependencies have to be faked, this may be an indication that the subject under test is doing too much.
  • Leaky abstractions – the test checks against the public API of the subject under test. i.e inputs and outputs, or side-effects, such as writing to DB.
    look for tests that test the SUT’s implementation (i.e the SUT calls a certain method on another object)
  • Naming – the test description talks about the object’s public API, rather than the implementation. i.e assertReturnsANewUserRecord rather than assertCallsUserRepository

Commit hygiene

separate commits can (and should) be used to make proposed changes clearer.

Atomicity (or single responsibility)

A commit should be a small, self-contained change. Applying that commit should achieve a single, well defined, goal.
A single “implemented the whole world” commit defeats the very purpose of having commits at all.
Ideally, a commit contains a small set of functionality, along with its verifying tests.

Commit message explains “what” and “why”, rather than “how”

A commit message should convey what functionality / business goal this commit implements. When necessary, also – why the proposed changes were made in that way.

A useful trick is to write the commit message as the completion of the sentence “When applied, this commit will..”.
For example – when applied, this commit will “add support for logging in with google”.
You may also add clarifications regarding the chosen implementation. i.e “made the login functionality async, since google authentication works asynchronously”. This will clarify why the change was made in the way that it was.

Some examples that a reviewer would want to comment on:
WIP , it works, trying something – these don’t explain anything.
Deleted unnecessary code from User class – doesn’t add any information. A reader can understand that code was deleted by just looking at the code changes.
A better commit message would explain why this code was deleted. For example: Removed support for MFA explains the business functionality that’s achieved with that change.

Some VCSs support re-writing the commit history.
you can (and should) encourage your team members to do that to create a coherent story that’s easy to follow.

Summary

These are the main things I look out for when reviewing code. It may seem like a lot, but, with time and practice, they become easier to spot.
However, a rigorous review still takes up more time than you may be used to, even for an experienced practitioner.
I’ll try and convince you that this time is well spent, in the next (and final) post of the series.

Read next

Part 3 – why should you care?
Part 1 – how to provide feedback

Code review as a tool to improve quality. Part 1 – how to provide feedback.

Other posts in this series

Part 2 – what to look out for in code reviews
Part 3 – why should you care?

Reviewing code is a fairly standard practice in most software teams.
However, like with any deeply ingrained practice that we take for granted, the rationale and purpose behind the practice can get lost, and the process is only followed mechanically, but not effectively (did someone say agile ceremonies?).
In this series of posts, I’ll take a look at what makes for an effective code review process. And furthermore – how I believe it can be leveraged to increase the quality of code for the team.


But first we must begin with a –

Definition

A code review (or peer review) is the process whereby proposed changes to a codebase by an author are inspected by a reviewer.
The mechanics of how this is done can vary – Github has popularized the “pull request” concept, but reviews can also occur over email, or even by just grabbing a colleague to come view your code.

Goals

To make a process effective, we should first define what it is we’re trying to accomplish. Here are the goals of a code review as I see them:

  1. Quality control
    This is about ensuring a standard of code quality – not about finding bugs or mistakes.
    (Although, if we review test code as part of the proposed change, then this might serve for that purpose as well.)
    The reviewer tries to ensure that the proposed changes meet certain bars of quality. If it doesn’t, they may suggest ways to improve the code’s quality.
    (“Code quality” can be subjective, and I’ll try to define what (I think) it means in the next posts.)
  2. Knowledge sharing: Reviewer –> Author
    The reviewer can suggest simpler, more efficient or more maintainable options for implementing the proposed changes.
    This may be because they’re familiar with a pattern or a language feature that the author is not aware of. Or they may know that someone has already implemented similar code that the author could re-use.
  3. Knowledge sharing: Author –> Reviewer
    Firstly, like the above – it’s possible that the author has used a pattern / language feature that the reviewer is not familiar with. The reviewer has now learned something new.
    Secondly, this is an opportunity for the reviewer to familiarize themselves with a different area of the codebase. This means that, in the future, the reviewer will be able to more easily maintain this code.
  4. Knowledge sharing: Documentation
    In some cases, the review process is done in writing (like a pull request). In those cases, the review itself becomes an artefact that can later be referred to.
    For example – when a maintainer wants to understand the purpose of a certain piece of code, or why it was written in a certain way.

Some important corollaries of the above goals are –

  1. A review is a communication method for technical leadership. By accepting or rejecting code, they signal to the team what kind of code is and is not acceptable.
  2. A review is a mentoring opportunity. It’s a chance for the reviewer to teach the author something that they did not already know (or learn from them).

Review as a mentoring tool

This is the first area I’d like to touch – how to make a review an effective tool for teaching and learning.

1. Be kind

Developers are only humans, and are subject to human reactions. We are much less likely to take advice or feedback on board if we feel attacked or insulted.
Phrase your feedback in an encouraging and constructive way.
For example, comments such as “I can’t merge this – this change will break the entire app!” or “This code is a bit messy – Please tidy this up before merging” are both antagonizing and unhelpful.
Even if those statements are factually correct, they don’t contribute anything to solving the issue.
Comments like “Looks like these changes break the signup functionality in X scenario; could you make sure it’s fixed before merging?” or “I find it hard to wrap my head around the implementation of the email reminder functionality; could it be simplified?” are much more helpful as a first step to resolve the issue (and actually point the author to what the issue is).

2. Teach a woman to fish

As we discussed, one of the goals of a code review is knowledge sharing. Meaning – teaching the author something they don’t yet know.
Take these opportunities to teach. Don’t focus only on “fixing” the immediate problem at hand.

Firstly, explain why you’ve made your comment, and what general principle or pattern you have in mind.
For example, rather than “This class should be smaller”, explain that “This class has too many responsibilities”.

Secondly, prefer to not prescribe a solution to an issue you’ve found.
This is because we learn better when we figure out something for ourselves, rather than being instructed.
Also, force-feeding solutions might serve to demotivate your colleagues. They may feel as though you’re treating them like code monkeys, not allowing them to think for themselves.

My preferred approach is to identify the issue clearly, and prompt the author to find a solution.
Examples include “This method seems a bit long; would it be possible to shorten it?”, or “Looks like this code is being repeated in both the DBConfiguration and XmlConfiguration classes; could we DRY it up?”

There may be cases where you do choose to suggest a solution. Make sure to explain why you’re suggesting a certain change, and what general principle or pattern you have in mind.
For example, rather than simply instructing the author to “Remove the duplication in the DBConfiguration and XmlConfiguration classes by extracting everything except the actual data fetching into a base class, which defines a virtual getConfig method that will be implemented differently by both subclasses” , explain what the “template method” design pattern is, and when it’s appropriate to apply it.

3. Seek to learn

Ask questions to make sure that you have a good understanding of the proposed changes. Ask the author why they chose a certain implementation over another. Ask about anything that seem non-standard, or that you haven’t seen before. Ask if there’s anything you don’t understand. Asking for clarifications achieves multiple goals –

  • It verifies your understanding of the code (meaning – you’ll be able to more easily maintain it in the future)
  • It challenges the author to re-think and justify their choices – They may find a better alternative in the process.
  • It helps you learn from the code.

Conclusion

Remember that a code review is not just about this particular change.
A review is one of the easiest, lowest-cost teaching opportunities you can get.
Approach your review as starting a conversation rather than a binary approve / reject scenario.
Many times I’ve found that I, as the reviewer, have learned something new as a result of that conversation.


Read next

Part 2 – what to look out for in code reviews
Part 3 – why should you care?