Sign in
Log inSign up
What makes code unreadable

What makes code unreadable

Tao Wen's photo
Tao Wen
·Oct 1, 2018

"Any fool can write code that a computer can understand. Good programmers write code that humans can understand." - Martin Fowler

Readable code is all about how our brain interprets the source code, what affects its performance. There are 4 major reasons that make the code unreadable:

  • Too many or too long: when your brain needs to track # of variables, # of lines of logic
  • Non-local logic: we prefer continuous, linear and isolated logic. There are three reasons to make the logic non-local
    • coding style: global variable, SIMD intrinsics v.s. SPMD style GPU computing, callback v.s. coroutine
    • generalization: to reuse the code, we need to entangle multiple execution paths into one
    • non-functional requirements: it is temporal and spatial co-located (both in source code and runtime) with functional logic
  • What you see is not what you get: we are forced to imagine how the code works in the runtime when it is vastly different from the source code form. Such as metaprogramming, multithreading with shared memory.
  • Unfamiliar concept: we use name and reference to link one concept to another. When the linking is not strong, we will have trouble and say it does not "make sense"

Let's talk about them one by one. Examples are taken from

Too many or too long

We have limited power to hold variables in our working memory. Keeping track of the change in head consumes exponentially more power every step. 

// more variable
sum1 = v.x
sum2 := sum1 + v.y
sum3 := sum2 + v.z
sum4 := sum3 + v.w

// less variable
sum = sum + v.x
sum = sum + v.y
sum = sum + v.z
sum = sum + v.w

Every variable has the potential to be changed. If the sum1, sum2, sum3, sum4 is const, it is less stressful. 

// longer execution path to track
public void SomeFunction(int age)
{
    if (age >= 0) {
        // Do Something
    } else {
        System.out.println("invalid age");
    }
}

// shorter execution path to track
public void SomeFunction(int age)
{
    if (age < 0){
        System.out.println("invalid age");
        return;
    }

    // Do Something
}

Return early cuts down the execution paths we need to track in the head. The shorter path to reach a conclusion, the better.

Non-local logic

We prefer continuous, linear and isolated logic. Let me explain what I mean

  • continuous: the line 2 should be related to line 1, they are put together to express close causality
  • linear: you read code from top to bottom, the code executes from top to bottom
  • isolated: all you need to care about is in one place
// continuous, linear, isolated
private static boolean search(int[] x, int srchint) {
  for (int i = 0; i < x.length; i++)
     if (srchint == x[i])
        return true;
  return false;
}

There are three reasons to make the logic non-local.

  • coding style: global variable, simd intrinsics v.s. spmd style gpu computing, callback v.s. coroutine
  • generalization: to reuse the code, we need to entangle multiple execution paths into one
  • non-functional requirements: it is temporal and spatial co-located (both in source code and runtime)

Non-local logic: coding style

Communicate through global variable will make the causality implicit, making the required logic pieces harder to be put back together.

// declare global variable
int g_mode;

void doSomething()
{
    g_mode = 2; // set the global g_mode variable to 2
}

int main()
{
    g_mode = 1; // note: this sets the global g_mode variable to 1.  It does not declare a local g_mode variable!

    doSomething();

    // Programmer still expects g_mode to be 1
    // But doSomething changed it to 2!

    if (g_mode == 1)
        std::cout << "No threat detected.\n";
    else
        std::cout << "Launching nuclear missiles...\n";

    return 0;
}

It is trivial to translate same logic from using global variable to passing context explicitly via arguments. It is the coding style we choose to make code unreadable.

Second example is about SIMD programming. Write code to drive SIMD executor, we need to take care of multiple "data lane" at the same time. Notice the %ymm0 is 256bit register, 8 data lane for 32 bit:

LBB0_3:
    vpaddd    %ymm5, %ymm1, %ymm8
    vblendvps %ymm7, %ymm8, %ymm1, %ymm1
    vmulps    %ymm0, %ymm3, %ymm7
    vblendvps %ymm6, %ymm7, %ymm3, %ymm3
    vpcmpeqd  %ymm4, %ymm1, %ymm8
    vmovaps   %ymm6, %ymm7
    vpandn    %ymm6, %ymm8, %ymm6
    vpand     %ymm2, %ymm6, %ymm8
    vmovmskps %ymm8, %eax
    testl     %eax, %eax
    jne       LBB0_3

Instead of specifying how to apply same operation on multiple data lane, writing code to deal with one "data lane" is much simpler:

float powi(float a, int b) {
    float r = 1;
    while (b--)
        r *= a;
    return r;
}

It takes ispc.github.io/ispc.html to compile the code from SPMD style to SIMD style, they are equivalent.

The third example is about callback v.s. co-routine

const verifyUser = function(username, password, callback){
   dataBase.verifyUser(username, password, (error, userInfo) => {
       if (error) {
           callback(error)
       }else{
           dataBase.getRoles(username, (error, roles) => {
               if (error){
                   callback(error)
               }else {
                   dataBase.logAccess(username, (error) => {
                       if (error){
                           callback(error);
                       }else{
                           callback(null, userInfo, roles);
                       }
                   })
               }
           })
       }
   })
};

compared to

const verifyUser = async function(username, password){
   try {
       const userInfo = await dataBase.verifyUser(username, password);
       const rolesInfo = await dataBase.getRoles(userInfo);
       const logStatus = await dataBase.logAccess(userInfo);
       return userInfo;
   }catch (e){
       //handle errors as needed
   }
};

Co-routine makes the logic linear, going from top to bottom. The code written in callback style, going from left to right. But they are equivalent. We can choose the coding style to make code more readable, given the programming language allow us to do so.

Non-local logic: Generalization

To generalize, you have to specialize. If you need to support specialized code for 10 products sharing majority of common code. How often do you need to reason 10 products logic together? Very often, you are thinking about how 1 specific product type works. But reading the generalized code, you are forced to skip over the code for other 9 kinds. The skipping causes real cognitive load.

Here is a simple example

public double PrintBill()
{
    double sum = 0;
    foreach (Drink i in drinks)
    {
        sum += i.price;
    }
    drinks.Clear();
    return sum;
}

We think PrintBill is too useful, we have to reuse it. But for happy hour, we need to take discount on some drink. The code become

interface BillingStrategy
{
    double GetActPrice(double rawPrice);
}

// Normal billing strategy (unchanged price)
class NormalStrategy : BillingStrategy
{
    public double GetActPrice(Drink drink)
    {
        return drink.price;
    }
}

// Strategy for Happy hour (50% discount)
class HappyHourStrategy : BillingStrategy
{
    public double GetActPrice(Drink drink)
    {
        return drink.price * 0.5;
    }
}

public double PrintBill(BillingStrategy billingStrategy)
{
    double sum = 0;
    foreach (Drink i in drinks)
    {
        sum += billingStrategy.GetActPrice(i);
    }
    drinks.Clear();
    return sum;
}

To generalize PrintBill to both happy hour and normal hour, we have to specialize on billing strategy. To read the generalized code, it is certainly less readable than original version.

Also to support all kinds of cases, the code can not be specific for just one scenario. This will create a lot of "variation point". It is very possible, for certain scenario the variation is simply a empty impl to fill the hole. For example, if we need extra service charge for normal hours. The code looks like

interface BillingStrategy
{
    double GetActPrice(double rawPrice);
}

// Normal billing strategy (unchanged price)
class NormalStrategy : BillingStrategy
{
    public double GetActPrice(Drink drink)
    {
        return drink.price;
    }
    public double GetExtraCharge()
    {
        return 1;
    }
}

// Strategy for Happy hour (50% discount)
class HappyHourStrategy : BillingStrategy
{
    public double GetActPrice(Drink drink)
    {
        return drink.price * 0.5;
    }
    public double GetExtraCharge()
    {
        return 0;
    }
}

public double PrintBill(BillingStrategy billingStrategy)
{
    double sum = 0;
    foreach (Drink i in drinks)
    {
        sum += billingStrategy.GetActPrice(i);
    }
    sum += billingStrategy.GetExtraCharge();
    drinks.Clear();
    return sum;
}

If you are maintaining the logic for happy hour, the line sum += billingStrategy.GetExtraCharge(); is completely irrelevant to you. But you are forced to read it anyway.

Also there are many ways to write "branching" code. function overload, class template, object polymorphism, function object, jump table, and plain if/else. Why do we need so many ways to express a simple "if"? It is absurd.

Non-local logic: Non-functional requirements

Functional and non-functional code are tangled together making the code hard to interpret by human. The primary goal of the source code should be describing the causality chain of everything. When some x happen, but y did not follow, we perceive it as a bug, which is what I mean as causality chain. To make this causality chain executed on physical hardware, there are a lot of details to be specified. For example:

  • the value is allocated on stack or heap
  • the argument is passed as copy or pointer
  • how to log the execution so that we can trackback when debugging
  • how to execute concurrent "business" processes, on single thread, multi threads, multi machines, over multi days

Here is an example on error handling

err := json.Unmarshal(input, &gameScores)
if ShouldLog(LevelTrace) {
  fmt.Println("json.Unmarshal", string(input))
}
metrics.Send("unmarshal.stats", err)
if err != nil {
   fmt.Println("json.Unmarshal failed", err, string(input))
   return fmt.Errorf("failed to read game scores: %v", err.Error())
}

The functional code is just json.Unmarshal(input, &gameScores), and if err != nil return. We added a lot non-functional code to handle the error. Skipping over those code is non-trivial.

What you see is not what you get

We are forced to imagine how the code works in the runtime when it is vastly different from the source code form. For example:

public class DataRace extends Thread {
  private static volatile int count = 0; // shared memory
  public void run() {
    int x = count;
    count = x + 1;
  }

  public static void main(String args[]) {
    Thread t1 = new DataRace();
    Thread t2 = new DataRace();
    t1.start();
    t2.start();
  }
}

Count is not always x+1, given there are other thread doing the same thing in parallel, they might override your x+1 with their x+1.

Meta-programming also requires a lot of imagination power. Unlike function call, you can not jump into the function you called to checkout what you are building upon. For example

int main() {
    for(int i = 0; i < 10; i++) {
        char *echo = (char*)malloc(6 * sizeof(char));
        sprintf(echo, "echo %d", i);
        system(echo);
    }
    return 0;
}

Code is generated on the fly. There is no easy way to checkout the generated source code to reason about its correctness.

Unfamiliar concept

We use name and reference to link one concept to another. When the linking is not strong, we will have trouble and say it does not "make sense"

image

We think the game on the left is a lot easier than the game on the right. Why? Because ladder and fire are familiar concept mapped to your real life. We build new experience on top of existing experience. If the code is detached from the requirement, and detached from real life concept, it will be hard to understand.

func getThem(theList [][]int) [][]int {
    var list1 [][]int
    for _, x := range theList {
        if x[0] == 4 {
            list1 = append(list1, x)
        }
    }
    return list1
}

is less readable than

func getFlaggedCells(gameBoard []Cell) []Cell {
    var flaggedCells []Cell
    for _, cell := range gameBoard {
        if cell.isFlagged() {
            flaggedCells = append(flaggedCells, cell)
        }
    }
    return flaggedCells
}

type Cell []int

func (cell Cell) isFlagged() bool {
    return cell[0] == 4
}

Because [][]int is unfamiliar, but []Cell mapped to life experience.

image

Linking concept from code to life experience is done via "name", which is just a string. Linking concept between code modules can rely on "reference". You define function, and reference the function. You define type, and reference the type. From IDE, you can click the ref to go to its definition. When the linking is strong and clear, we build up our vocabulary, in which we can read and think the code a lot quicker.

Concepts come from decomposition. Every time we decompose we give it a name. There are three kinds of decomposition:

  • Spatial Decomposition
  • Temporal Decomposition
  • Layer Decomposition

When we feel the problem is big, we can always break it down by this three methods, just remember to name the concept broken down properly.