AFP548 Site News April 13, 2020 at 8:31 am

Code Wranglin’, A Series

Welcome to a series about wranglin’ cats code. Some of us are inclined to care about titles, and while I’m fine being a MacAdmin, Client Platform ‘Engineer‘ is a more recent term recruiters want to plug into LinkedIn. That engineer part is meant to differentiate us from the apes sysadmins, which should already have been on a path to automation domination and therefore probably sling code like we might aspire to… Anyways, being even tangentially related to Software Engineering as a discipline has the connotation that we’ll show our code to someone eventually and want people to collaborate on it, or give it a sniff before it blows things up in ‘prod’. When we do that with others often enough, seeing various code suggestions and workflows, it starts to feel like it would be good for everyone to collaborate and standardize on a minimally-viable-process so us code-butchers can ship better scripts & stuff.

It’s not helpful to be opinionated AND inconsistent, so I wanted to… codify some guidelines, and an approach that the folks in my team (and hopefully the community at large) can use as a starting point to collaborate on/argue about in the service of getting us all on the same page regarding style/design ‘quality’/etc. Even open source projects/loosely affiliated groupings of individuals shipping code may benefit from writing this down, but I didn’t have a good example of how to approach the task for us sysadmin-y types in particular. (If you have references you’ve found/like for any of the topics in this series, please share on Twitter or in the comments!) It turns out this is a big enough thing for several posts, so I’m breaking it up and will cover the scripting languages MacAdmins commonly use later on. ‘Code reviews’ as a concept may be new to some, so that’s where I’ll start in this post.

Why We’re Here

The point of code review is not to ‘rubber stamp’ as a mindless formality, nor is it to just confirm it kindof looks like a familiar file extension the author wants to spread blame for prior to shipping into prod. In more rigorous environments, it is also not a given that the reviewer will fully grasp the problem the code is attempting to solve, or be able to exercise all parts/functions in the code, or safely run it against ‘production’ data/live systems to 100% validate it,[1]

the point of code review is to have someone else spot-check the code, to… y’know, read it, every line, and to make sure it passes some degree of muster for maintainability/whole-team support/adoption that could achieve something almost like endorsement when it ships. The reviewer should display empathy and hopefully have TIL (today I learned) moments, they should reason about the design used and offer alternatives, constructively argue when individual nitpicks clash and help the team collaborate on and grow the style guide with things that are important enough to at least bury hatchets about,[2]

and the changes proposed in the review can be followed through/integrated into the code before/after shipping if/when we have the time/see the value. Because rhetoric is all well and good, but as they say, real artists ship.

  1. BTW, ‘correctness’ by the original author is not taken for granted, there’s ways to help prove it to the reviewer we’ll get into later, but it’s the author’s name in git blame who is considered primarily responsible.
  2. Sometimes a style guide contains headstones for your litany of hangups that got sacrificed for team unity/sanity/productivity.

The statement right before these sort-of footnotes is important to expand, because developers/traditional ‘engineers’ have different incentives and expectations set on them than sysadmins who ship code. Many startups can get years down the road before the YOLO/eff-it-ship-it attitude starts losing its luster and editing code on the live production instance actually causes enough of an outage that business processes are interrupted. Or maybe you get interested in verification processes when you’re getting audited/acquired, or the lack of rigor just becomes unseemly. For organizations that don’t value computers, or have low-enough standards, or a high-enough tolerance for pain, maybe that time never comes. For MacAdmins, we know that on the technology treadmill there’s constantly demands to ship product and anticipate issues. Most (if not all) of our tasks are not purely intellectual exercises, and our managers are incentivized to help us ship better than ‘working prototypes’ more often than not. At least, after a company’s culture evolves to a point that it reaches a certain scale (or plateau) and decides to emphasize stability over velocity.

Ideally, ultimately, we hope things ship with increasing reliability. And in the process of getting muscle memory going through a process, we hope reviewers understand both what was written and why, agree that the implementation should ship, potentially help introduce efficiencies, and, where the REAL value comes from, (which is the reason management encourages us to do this in the first place,) reduce/prevent bugs/unwanted side effects.

Opposition to taking the time to check the code into version control in the first place, let alone do a review, usually starts with ‘well if there’s an emergency…’ Define what an emergency is to your team, otherwise they get the Princess Bride quote, or the ‘lack of planning on your part doesn’t constitute an emergency on mine’ tough love. Getting everyone to stop being independent operators is not easy, so management needs to be on board and lower the (usually false) sense of urgency. We’ll talk about asynchronous tips below, but anecdotally, having a distributed team increasing turnaround expectations actually helps with this. (When folks aren’t chained to Slack or have Zoom sessions open all day also helps, because often code design benefits from your brain operating in system two and the stimulation of a busy chat with instant gratification tends to prevent you from being idle at regular intervals.)

It’s dangerous in the code base, take this

I’ve already droned on waving my arms for a bit, but needless to say Google shipped their code review and style documentation years back as well-edited references. An important point they discuss for reviews is calling out the good things you see as a reviewer, since this can often look like a critique that the humans submitting the code can feel bruised by, especially in public/on the official record. Breaking the computers, less people are worried about (eventually we all have to Escape from LA). They get understandably annoyed if you break the malleable humans.

I also benefitted from this series about being on a distributed, asynchronous/non-time-zone-overlapping team landing code together. It does a great job illustrating that this is all about context transfer/communication by taking action and thinking about the other side of the coin and our shared goals. AFP548 lets me publish text in english on the internet, but only you as the reader can judge if I made my point/got across what I set out to say. (Communication is judged on the receiving end, exhibit A.) Positive code review experiences are the result of good back-and-forth communication.

Out in the real world for us sysadmins where reviews may be less codified, if you’re lucky enough that the project you’re sending code to already has contributing guidelines publicly stated (e.g. in a CONTRIBUTING.md), then that’s your defined jumping-off point. We often can’t choose who reviews our code nor express tone clearly in text from either side of the interaction, so each has to provide context and be as welcoming as possible. As the author, be equal parts ego-less and greedy – showing you really care that others help you is a great look. If a review process is still in flux, hopefully you find these concepts helpful to mull over, and we can all have fun painting bike sheds later when we get into code style details.

Up next, it’s unavoidable to talk about the version control tooling itself. TTFN!

Allister Banks

Allister lives in Japan, has not read the Slack scroll back, and therefore has no idea what is going on.

More Posts - Website

Follow Me:
Twitter

Tags:

Leave a reply

You must be logged in to post a comment.