Optimizing for Teamwork in software projects

Context ↩

Twice in recent memory for me the idea around "optimizing developer experience" has come up. The first one, which doesn't directly impact me and which I just heard through the grape vine was some notes about how the rust formatter was causing unneccesary diffs in imports. The second, was a recent project at work that I'll anonymize a bit and share.

I think the linux one is fairly easy to summarize. As I understand it, the rustfmt tool can't be configured in non-nightly builds in a way that gives it sensible control over when it decides to adjust imports. So, for example, if you had code like:

use std::time::Duration;
use std::time::Instant;

Depending on the rules, when you ran the formatter, you might end up with no change, or this:

use std::time::{Duration, Instant};

This might not seem like a problem, but think about having 100 or more imports from the same module and reviewing code that constantly shows you that the same line changes, over and over again. Or worse, that you have a merge conflict because branch A and B both changes the imports on the same line, removing some, adding some, or re-ordering them.

How annoying! You might say, and then channel your inner Jeremy Clark:

What's the big deal? Friction is the big deal. Any programmer who's ever been the "release engineer", by name or happenstance, understands that if you have a number of different features going out into a release for something, if they all changed the same area, you get a longer period of resolving merging the various changes. Even if it's something as straightforward as compiling and making sure that no imports fail, or something more intense such as ensuring none of those imports were malicious; it still takes time.

The issue with the rust formatter is that shoving everything onto one line makes conflicts happen more often. While having them spread out, preferably in alphabetical or logically grouped ordering, makes it much much easier to tell what's changing at a glance. Tell me, what changed here?

use myfake::library::{A,C,D,F,G,I,T,H,P,Q}

use myfake::library::{D,Q,G,F,L,A,C,T,H,P}

How long did that take you? How about:

use myfake::library::A
use myfake::library::C
use myfake::library::D
use myfake::library::F
use myfake::library::G
use myfake::library::H
use myfake::library::L
use myfake::library::P
use myfake::library::Q
use myfake::library::T

use myfake::library::A 1
use myfake::library::C
use myfake::library::D
use myfake::library::F
use myfake::library::G
use myfake::library::H
use myfake::library::L
use myfake::library::P
use myfake::library::Q
use myfake::library::Z

I would bet that you found the change in the second set a lot faster than the first. ²

So, obviously one style promotes better readabiliy by the reviewer. And so, in Linus's case, he prefers this because he reviews hundreds of change requests as part of his role in the linux maintainers world. While an individual contributor might not think too much about it, it makes a big different to him. This same thing I think can be pulled out into regular teams. Depending on what you're doing at work, you'll be optimizing for different things. I think it's nice to always try to optimize for readable reviews, but sometimes you can't.

There's probably more things to be said here, and this is also opinion so you may not agree with it all. But I'd like to dive into the third row here, as this relates to the work related experience I mentioend at the start of this section: High Churn code.

An Example ↩

Within the last month at work, one of my teams had to deliver a lot of work, really fast. That probably sounds like par for the course for some of you. But, when I say fast, I mean two weeks. And by a lot work, I mean that at the end of the day we added around 10k lines of new code, encapsulated close to 100 new business rules into our system, and pushed it all through without any bugs and without any merge conflicts.

Now, it's not hard to write 10,000 lines of code. My last blog post was 5000 lines of text, and the project it described was ~2400 lines of code for something relatively simple. Lines of code is not a measure of productivity, but it does help to give you an idea about the volume of work done across the system, and most importantly, when a lot of those changes are adds and deletes in the same area, you get an idea of how close to the sun folks are flying when it comes to merge conflicts and the like. ⁴

High churn areas of code are often the critical path of your system. If you're writing software that handles managing and combining lots of deals and carts and whatnot, then I would suspect you'd spend a lot of time ensuring that code is maintainable. I'd also suspect that many teams do not have the time for such a luxory and end up in messes of neccesity. So I think that it's important, when you're on a team and you know that changes to a particular location are going to come in often, that you optimize that code for that use case. CPUs are generally pretty fast, and so in your typical application, if you're not writing code for highly constrained systems, it's probably worth the CPU time to optimize for the developer experience.

Let's come up with a fictional example of transforming data. Your cool system at work takes data from super duper API named Galactus and has to send it off to some client company of yours, let's call it, WheelInventedHere, who have defined their own obnoxious JSON schema format for documents. In the initial stages of the project, you're prototyping and figuring things out. Perhaps this isn't the work of the team, but one person tasked with a project and a spike to poke around and figure things out. Thus,you write everything in one shot to make things easy for you. You've optimized for speed of a single dev:

class Galactus2WIHTransformer {
    getSystemVersion() {
        return 1; // TODO: tweak later to pull from git or something. 
    }

    transform(galactusData) {
        const components = [];

        const title = galactusData?.title || 'No Title';
        const metadata = {};

        const toISO8601 = (epochSeconds) => {...}

        metadata.publishDate  = toISO8601(galactusData?.dates?.publishDate);
        metadata.lastModified = toISO8601(galactusData?.dates?.modified);
        metadata.revisionDate = toISO8601(new Date());
        metadata.version = this.getSystemVersion()

        const intro = galactusData?.intro?.parts || [];
        for (let i = 0; i < intro.length; i++) {
            let component;
            switch (intro[i].type) {
                ... a bunch of checks for 100s of lines 
                ... and some code to tweak and change each 
                ... into the WIH format... 
            }
            if (component) {
                components.push(component);
            }
        }

        const body = galactusData?.article?.parts || [];
        ... similar for loops and etc
        ... more lines of code adding things to the list
        ... with business logic and etc to choose how to 
        ... construct each one

        const wihData = {
            title,
            metadata,
            document: components
        };
        return wihData;
    }
}

Now the fun begins. We've got a few hundred lines, maybe a thousand or more. All in one method becuase the person writing this was tasked with getting something working. Not with getting something working that a team could immediatel contribute to. Heck, given that in our scenario here this is prototypical spike code, there's probably nothing for a "test" more than running the application itself. If we're lucky, some foresight maybe added in something to test at the transform seam, but given the use of new Date(), it's only going to be a partial test of some pieces.

The happy path, as it were.

The happy path quickly becomes the regretted path because just as the prototype is ready, a command from above comes in. That expected deadline of two months from now? It's now two weeks. So the team decides to take the working prototype and throw it into production. Over time, requests come in to change things:

Hey, for the buttons in the intro, can you make them… Oh! Did you see? The websites are now linking headings to site X, can we do that too? Hm? What are they linking to? Oh to the same links as the buttons after the heading… Oh could we make a table of contents for all the headings in the doc…

Each of these changes come in, and so dilligently, the team tweaks the ever growing mountain of code in the transform method. When multiple requests come in, two developers might work on it in parallel, but the release engineer always spends extra time to resolve the conflicts in the mega function, there's often defects that surface after release even when there isn't a conflict because of some interaction between the two changed halves that wasn't exercised.

The code that was created by one developer, for one developer, resists changes from more than one developer at a time. It's a problem for the team. And so, the team has to either bite the bullet and refactor the whole block of code or they need to do something better at the start. The better thing to do, if you're optimizing for an entire team of developers, is to create a pipelining pattern that seperates out the various transformations into their own classes. For example:

class Galactus2WIHTransformer {

    constructor (versionInfo, transformers) {
        this.versionInfo = versionInfo;
        this.transformers = transformers;
    }

    transform(galactusData) {
        const metaDataTransformer = new MetaDataTransformer();
        const metadata = metaDataTransformer.transform(galactusData, this.versionInfo);

        ... business requirements changed and now we change title based on x y and z ...
        const titleTransformer = new TitleTransformer();
        const title = titleTransformer.transform(galactusData, metadata);
        
        const components = [];
        const introTransformer = new IntroTransformer();
        components.push(...introTransformer.transform(galactusData));
        ... similar for other parts of the document being transformed

        const wihData = {
            title,
            metadata,
            document: components
        };
        return wihData;
    }
}

class IntroTransformer {
    transform(galactusData) {
        const components = [];

        const intro = galactusData?.intro?.parts || [];
        for (let i = 0; i < intro.length; i++) {
            const componentType = intro[i].type
            if (this.transformers.supportsComponent(componentType)) {
                const transformer = this.transformers.get(componentType);
                const result = transformer.transform({
                    component: intro[i],
                    docContext: galactusData?.identifier,
                    ... other contextual data ...
                });
                if (result) {
                    components.push(result)
                }
            }
        }

        return components;
    }
}

... similar classes for the other body parts, pre or post processor, synthesizing new components
... based on others, etc etc

The main point of the change to this code isn't just that we've taken the guts of that switch statement and pulled them out into seperate classes. But that we've created a framework which developers can hang their code out to dry in without having to wash someone elses laundry at the same time. If we get a task to update the intro items to do X, and also to add in a new component for Y scenario, but only within Z context. We can do that within the different transformers. No more merge conflicts.

What about the incoming change like: Oh, you can get the price information for the buttons in the X type documents from the Springus service! Before the change, we'd be tweaking the transform method of the Galactus2WIHTransformer for every single change. It'd balloon with each dependency and potentially get harder and harder to construct for testing purposes until people just give up doing so and rely on manual QA for every little thing.

With seperated transformers and a proper inversion of control to inject those dependencies to each?

class SpringusButtonTransformer {
    static get inject() { 5
        return [
            'SpringusService'
        ];
    }

    constructor(springusService) {
        this.springusService = springusService;
    }

    transform({ component, docContext }) {
        if (!this.transformable(component)) {
            return undefined;
        }
        const priceInfo = this.springusService.getPriceInfo(component.buttonId, docContext.identifier);
        ... bla bla bla ...
        return coolButton;
    }
}

The concerns that a given transformer needs access to some resource becomes its concern. Not the concern of the overall process. Now, obviously, as a developer working on the system we should know about if we're making random network calls within a loop and cache and optimize as needed, or better yet, fetch the data upfront and then pass it along within that context or something so we can work in pure functions that don't even need to stop and think about an await hanging around somewhere.

But by creating this structure and framework, we've enabled ourselves to do this without having to keep 1000s of lines of context in our heads at once. When I think about springus buttons, I only think about springus buttons. If there's some sort of mutable state shared between the rendering pipeline, then I have the tools to potentially minimize where we think about it, or can put in logging and tracing in a centralized place to watch for pollution.

You get a lot of good stuff by optimizing the code so that it can be worked on by a team, don't you agree? Do you lose things? Sure. Potentially you've used more system memory, or spent more CPU cycles on setup and extra checks that a transformer exists for X or Y, or handling potential fallbacks when it doesn't. Heck, you might even waste time with checking decorators or a chain of responsibility of potentially multiple transformers that all want a piece of one thing in different contexts.

The trade off is worth it though I think. Flexibility is a powerful thing for a system to offer to the powers that be within a business. Being able to pivot quickly can be dependent on how easily it is to task multiple changes to a system indepedently of each other. And that's exactly how my team and I did what I described before in so little time. Because each of us could take a task from someone dictating new and changing business rules to us, we could easily modify and update the system without fear of conflicts. Because each change was localized to a single responsible party, we could easily construct test cases and minimal data examples to get unit tests in place. Minimizing how much wasted time would go into potential QA time by another person.

Merging releases and worrying about spooky action at a distance? Not an issue when the code isn't sharing the same function space. 0 chance of variables accidentally being re-used, shadowing each other, or other issues one might run into within a mega function. There are other patterns that I've used in code similar in spirit to the above to make things simpler, but I won't go into details here. I think this example is hopefully illustrative enough for why it's a good idea to avoid the one-shot function if you're planning on ever having a team share the code.

Closing thoughts ↩

So. How do you do this? If you can recognize what's happening early on in a spike, you can setup the code to be refactored easily by using helper methods. Constructing outlines in your code on what the program is doing. If you don't want a full on object oriented approach like I noted above, you could just as easily declare each transformer as functions. Though, the springus example becomes a bit more tricky to do ⁶. But that aside, just having something like:

transform(...) {
    this.step1(...)
    this.step2(...)
    this.step3(...)
}

Will make the process of pulling things out easier. I think it doesn't quite optimize for developer throughput though. If folks are still modifying the same file, you still run the risk of merge conflicts from all the vertical shifts between helpers and whatnot. It's better to pull things out into seperate places I think, but baby steps can be helpful if you have this problem on your hands right now.

And if you do have this problem of mega methods in your life. With no tests. Then leverage static helper methods if your language supports it. If you lift up some of that logic into a place where you can test it and get a bit of sanity checking on your system, all the better. If you can't test because it requires some big object, refactor the helper to use primitives or a simpler interface. There's a lot of techniques you can do, even without tests, to get things under control. The main goal is that if you want multiple people to work in the same project, you need to make space for them to work without interfering with other in-progress work.

Do that, and you get real proper human productivity. It does require discipline though, and people actually reviewing code with a big picture in mind to catch these things early. In my opinion, the ability to have one of these "soft skills", where you empathize with the rest of the team as you code and optimize your output so that it's easy to ingest by those people is an indicator of what seperates a junior and senior developer.

I often see people in the "tech space" of the internet crusading for whatever the current trend is. Senior developers who formerly worked at BIG WELL KNOWN COMPANY, saying you should do X or Y, or that they HATE boilerplate and "over complicated" code. A lot of the time these people are coming from a very specific context. Like system programmers rejecting all of OOP because they misinterpretted a talk from a conference. There's also plenty of folks who play a role of jumping in to "save" other teams, the type of dev who sits down, collects context into their head, and then shits out a bunch of code to make something critical work, then is ushered over to the next project to save.

The people left behind by those fast moving frontier exploring folks are the ones who have to maintain the system. Maintaining a system requires a team. Not an individual ⁷. And so, folks who are optimizing for a system that will be around for a while should keep their eyes focused on making code that does one thing, and does it well. So it can be tested easily, and expanded on without having to blow up other parts of the code as much as possible.

I think I'd just be berating the point if I continued on here. But I hope that I've at least made my case. Feel free to argue against it, you might be one of those people who can read 300 lines of code, understand it, and say "yeah that's fine, that's neccesary complexity". I am sometimes. But in a team, it's not the ceiling that dictates the speed. It's the floor. So if not every person on your team is a rockstar, you need to make a framework that paves a path for them to be productive in a way that works for everyone, not just for a "savior" programmer archetype.

If you are a solo dev. Then keep programming however works best for you! Just remember, the you of two months from now might have a totally different idea of how things should be done, almost like they're another person. If you want to optimize for the team of me, myself, and future I. Perhaps consider setting yourself up with a framework for later.