The risks of code review

Code (or peer) review is supposed to be a process, during development, that helps producing better software. It sounds simple enough, except it’s not.

Coding is not an exact science. Not even close! It requires a lot of subjective things of the person producing it. Things like: empathy, ability to communicate, imagination, past experiences, capacity for understanding different layers of a problem etc. These things are also the ones being employed during code review. They constitute the differences between us and let’s face it, as much as we would like to think that our differences create a better world, in reality we are hardwired mostly for war and these differences are triggers.

Of course, we do have mechanisms to regulate fighting impulses, but most of them are activated as responses to direct stimuli: voice tone, hand movement, body language in general. During code review, all these things are missing, so guess which way the “conversation” will go. Without a real conversation with the other person, all the important cues and hints are missed, making for an extremely poor conversation. This is one of the reasons internet discussions are almost always going from “Here’s my nice kitten” to “I hate your race!” in just a few replies.

This is one major reason why pair programming makes more sense.

The subjective things matter a lot for: naming (functions, variables, constants, classes…), choices for code composition, choices between imperative vs. declarative styles, choices for frameworks. This is the space where most code reviewing is taking place, unfortunately. There is a high risk of overseeing a real problem (e.g. we read way too much data from DB into memory) while getting lost in debates over the name for a local variable (e.g. “k” vs. “counter”).

We then tend to create the so called “code conventions” within teams (which btw, will always piss off a subset of the team members), that will no doubt focus on those small things. And so will the developers from then on, trying to stick to the conventions and losing the overview (can’t see the forest from the trees). These “code conventions” (I’ve seen wiki pages with dozens of conventions for a single team) are usually not a positive, common understanding of “how we should do things in this team”, but rather a list compiled by some people that “won” the fight. This can clearly be seen as subjective, as one encounters many such lists, different from one another, when switching teams.

It’s fun to see how the author’s review responses are also extremely selfish. More often then not, they try to justify mistakes that they would have clearly accepted and corrected would they have been in direct contact with the reviewer. In a best case scenario, the discussion would be taken “offline” and resolved through direct communication. In a worst case scenario, the comments thread would span for miles. Now multiply this with just the lack of context for the reviewer and the number of reviewers. So delayed delivery is another, very real risk. What I’ve seen happening a lot is a third party (usually some form of management) stepping in and forcing a compromise that will, most of the time, send local variable “k” to production and the overlooked memory leak along with it.

I don’t want to make a case against code review here, but in my experience people engaged in it need to be really good managers of other aspects of their lives too, in order to leverage its full potential. Otherwise it’s like giving the car keys to a bunch of 12-year-olds. Also, when I say pair programming, I don’t mean focusing specifically on the “driver/navigator” paradigm, but simply 2 people sitting and working together on the same code.

NoSkills

This is new terminology for a concept that has existed for a long time in our industry. People are slowly rediscovering old paradigms. NoSkills is actually referring to an old working technique that maximizes efficiency of planning, processes and meta work by ensuring no actual work gets done. Here’s what one should do to get highly skilled in NoSkills:

  • learn industry terminology like: scrum, lean, kanban, agile, CI/CD
  • read first links that pop-up on google about the aforementioned terms
  • learn that waterfall is bad
  • learn that documentation is bad and people interaction is good
  • complain about people not interacting
  • apply 2 week sprints, so that “we can track velocity”
  • never finish the work defined in a sprint
  • have constant meetings
  • religiously impose all process ceremonies
  • NEVER write tests if there is no time
  • switch from scrum to kanban, because “scrum doesn’t fit our work flow” (= we never finish the work in any sprint)
  • use GitFlow, with independent feature branches, because our 5 dev team needs faster progress
  • use CI/CD (= install a jenkins server)
  • have refactoring sprints (“oops, but we’re kanban now…”)

This is by no means an exhaustive list, because there are so many things to be studied and so many wonderful certifications to be purchased in the NoSkills world, that it would take more than a sprint to list them here.

Folks, work is more important than meta work (and don’t let anyone fool you). If software doesn’t get done, it’s because software doesn’t get done, not because the process around it is obsolete and needs to be changed. Eating the same food with different tools won’t make it better. Something we need to re-learn is that complexity-first is a bad approach. It makes us focus on the wrong things.

One great skill to acquire is recognizing the smell of NoSkills. Don’t fight it, just walk away.

Refactoring

Over the years, I’ve come to realise that code is more of an inter human communication device than a human-machine one. Machines only need electric current to perform their tasks. Logic gates, binary digits, you know what I’m talking about…

We, on the other hand, have evolved sophisticated mechanisms to relate with the surrounding environment. Even if the commands are still electric current based, we do not know how, nor do we want to communicate with each other at that level. One of the most advanced such mechanisms is the human language. This is a very complex construction, based not only on words, but also sounds, visual queues and, most importantly, abstract concepts.

It’s difficult enough to be in perfect understanding with each other, through human language (not to mention the differences between human languages and how they might map to different abstract concepts and all that…), let alone having to translate what we mean through an enormously oversimplified mechanism: that of a computer programming language.

So, thinking this way (that writing a computer program, in a computer programming language, is something that we do mostly for our own and our peers’ understanding) it makes sense to have that program as clear as possible. And yes, I know “clear” is a subjective concept, but that doesn’t mean we mustn’t try to achieve clarity. What I mean by this, is that it’s really much better for program readers to be able to grasp the intent of the program as easily as possible, if they are to do something based on it (modify it, learn from it, fix it, etc.). As always, don’t forget that you are also one of those program readers, even if it’s your own program.

Compare the following snippets of code and see which one you prefer:

  def exec(list) do
    res1 = []
    res1 = for x <- list, x.age > 18 do
      x
    end
    res2 = []
    res2 = for x <- res1 do
      x.weight
    end
    Enum.sum(res2) / length(res2)
  end

vs.

  def avg_adult_weight(people) do
    adult_weights = people 
      |> Enum.filter(&(&1.age > 18))
      |> Enum.map(&(&1.weight))
    Enum.sum(adult_weights) / length(adult_weights)
  end

The first snippet means nothing to us until we stumble upon x.age > 18, which gives away some hint about x probably being a person. A person over 18 years of age. This looks like some kind of filtering, ok. Next there’s some kind of transformation and in the end, some math. Sum / length looks like an average. Ok, I think I understand what they meant.

But wait! Why do they iterate twice over the list, isn’t that wasteful? Sure it is.

I left the multiple iteration pattern in there on purpose (something that is unfortunately very common in production code), to emphasise the fact that the authors were just drafting their intention. Once they tried it and saw that it “worked”, they moved on.

The second snippet should be clear for every programmer and, even for non-programmers. I specifically didn’t mention the programming language, because I really shouldn’t have to, for the reader to understand it. Even if you don’t get the weird syntax with those ‘&’ signs, you should easily read over them and see what the author meant. This is the essence of code clarity.

For even more clarity, I would also add details about the function signature:

@type person :: %{name: String.t(), age: integer, weight: integer}
@spec avg_adult_weight([person]) :: float

I think that code produced in the style of the first snippet, comes from the fact that authors think in an artificial way first, trying to explain human concepts in computer science terms (data structures, loops…). Refactoring then, is the process of translating the expression of those terms to a more human friendly form. This, to me, is a very low level process that we could avoid, by approaching coding from a human language perspective. For example, this is how I would approach the average adult weight function (ok, I would directly write the elixir code in this case, but the process is valuable when you don’t know exactly how you would implement it):

I know I want a function to tell me the average adult weight for a bunch of people:

  def avg_adult_weight(people) do
   
  end

Ok. Now I get those bunch of people and I want just the adults and then do some math with their weights:

  def avg_adult_weight(people) do
   # get just the adults… some filtering
   # do some math with the weights… sum / count probably
  end

You see, by taking notes (in comments) of what I want to achieve, I also get implementation ideas (after the ‘…’). This is quite the reverse of the above process. It’s a transition from human to programming language. Now, actually substituting the comments with code, is straightforward.

NOTE: Do not assume this function is written without having some verification mechanisms in place (tests, REPL sessions…).

Too many (maybe most) refactoring debates are taking place at the low level we discussed about a few paragraphs above. In my experience, this is really counterproductive. I see refactoring, as a process, starting from the step where you already have humanly readable code (like the second snippet) and you want to reshape it, to accommodate and play nicely with similar or new concepts. I’m talking about things like creating a higher order function, to abstract an algorithm, or similarly, in OOP, creating a template method in a superclass.

Senior developers

I guess this can be about senior anything, but I’ll stick to developers here.

There are loads of senior developers out there (and I don’t mean señor developer 🙂 this time). At least that’s what they call themselves and most of the time, it’s because the industry recognizes them as such.

I argue that, at least in most of the cases that I’ve encountered, this title is meaningless. Why? It should imply a lot of things, but it usually implies years spent in the industry, which is by far not enough to drive a team, to come up with adequate designs, to talk the client’s language etc.

I have seen far too many seniors that slow businesses down, or even drive them towards critical situations, because they know better. Classical example: senior developer imposes major code refactoring, in order to “stabilize the system”. Another example: senior developer destroys junior’s confidence by completely dragging their work through mud, trying to prove all the ways in which the junior is unworthy.

I’ve met a few real seniors. The experiences I’ve had, while working with them, can be represented by just a few words: safety, familiarity, clarity, simplicity, confidence. This is not at all related to how many technologies, frameworks or languages they master (although they do master several), but how well they can get their ideas across and how they can shed light on problems.

Oh yeah, by senior developer I mean also technical leads, team leads, architects and all the titles we like to invent to make ourselves feel better. How many times have you heard an architect saying something very clear and simple, with real business benefits? Now, how many times have you listened to an architect, having no clue what’s going on? Fun times, right?

Senior C# Developer (of course the caps make a difference…). Did you stop to think what that even means? Sure. It means a person working with C# for more than X years (5, 10?). Again with the time quantification… I’ve interviewed senior java developers, with 15 years of experience (almost exclusively java), that knew the language very well, but had no clue about JVM details. Good luck dealing with performance issues in production…

A senior developer is someone who makes things clear both in business and technical talk, who is really fun to work with, laughs and teaches you things. One key trait, as far as I can tell, is the diversity of things they’ve experienced: programming languages, industries, companies and life in general.

Harmful employees

What is a harmful employee? You’ve surely met your share in your career. Maybe you’ve been (or still are) one yourself. How can you know?

Usually, instincts will tell you when you are dealing with one and if all of them seem harmful, chances are that you are in fact the harmful one.

Let’s see what being harmful, as an employee, means. Businesses are run by people. The success of a particular business is directly translated into how well those people can contribute to it. Contributing poorly, means the the overall business health is less than it could be.

Considering business goals are rock solid and well communicated to everyone (meaning everyone understands what they need to do), here’s what a harmful employee looks like:

Poor technical skills that don’t improve

What this usually means is that the person does not actually want to do that particular job. Assuming they’ve had proper training and coaching, there is no reason to push them further into doing what they dislike. I call these people passively harmful.

A concrete example: Joe has to learn Python, to cope with the new tool-chain the team has adopted. After a month of study, Joe doesn’t seem to be able to do anything productive with Python.

Ever-present and putting a negative spin on everything

Some other helpful criteria is criticism, know-it-all (yet not helping and coaching), constantly using  the straw-man’s argument by focusing on meaningless problems and amplifying them. I call them actively harmful.

These people are very destructive, because they tend to shut team members off, blocking a serious part of the business resources. No matter if they do have some technical skills, overall they are more harmful for the business if they stay. They should definitely go!

A concrete example: Sarah has a senior role (tech lead, architect etc.) within team X. One of her duties is to do daily code reviews for the team. She always adds patronising comments, letting team members know they are not doing their jobs right.

I would always choose the friendly-and-inquisitive over the rigid-know-it-all-senior type.

We’ll talk in a future post about what the “senior” qualifier is used for and what it should reflect, in the context of software development.

The purpose of software

To understand the purpose of software, we first need to understand what software actually is. So what is it? It’s a tool. Nothing more, nothing less.

We developed tools, over the course of our evolution, to help us gain significant advantage over other species and to make our lives better. Our brain’s capacity to create sophisticated technology is what brought us to where we are today. We developed an unprecedented way of living, here on this planet and, in the last few decades, we changed it so drastically and so fast, that we can barely understand the impact it has on our environment and on ourselves.

The advanced technology we use today is mostly created and run by software. Farming tools, communication devices, transportation, scientific research, weapons, etc., they all have software controlling most stages in their processes. So the purpose of software, as the tool that it is, is to operate other tools, which in turn will serve us in solving specific domain problems.
Software development became such a big deal in our society, that ever more people are doing it and want to do it. Unfortunately, a lot of software developers (and dare I say the majority) make software development a purpose in itself, not giving the actual domain problem too much thought (or not at all in many cases). This doesn’t make any sense. How can you know your tool is actually helping?
Some might argue that software can be broken down into components, which can be independently built and then integrated at the end. Somewhat similar to a car factory. I understand the need for this mindset: it makes reasoning about things, easier. However, because of the “soft” nature of software, it is orders of magnitude easier to customize and change it than it is for car parts. This is actually the reality of software development: constant change. Still, it seems like most of the time, we build software systems that are difficult to change and maintain. Attempts have been made, to change this (e.g. XP, agile, lean, etc.), but somehow we took those ideas and turned them on their head until we ended up where we started, software difficult to change and maintain.

It’s difficult to see software as merely a tool, when there are countless hours being spent just to learn a particular programming language or operating system. There are egos at stake, personal targets, preferences. Social aspects basically. I don’t doubt similar things happen in other industries as well, but the nature of software integrates better with our capabilities. This has been observed quite some time ago already (Conway’s law).

We need to find better ways of creating and using this tool, because, let’s not forget it’s just a tool and it’s meant to make our lives better.