When code doesn't communicate enough

 You have been assigned a code review, in Typescript, using Slonik to create semi-typed injection-safe SQL fragments. The purpose is to allow selecting rows where the foo column has a NULL value.

 The following code:

function getFooCondition(values: string[]) : FragmentSqlToken {

return sql.fragment`foo = ANY(${sql.array(values, 'text')})`;

}

is being changed into this:

function getFooCondition(values: (string | null)[]) : FragmentSqlToken {

const parts: FragmentSqlToken[] = [];

if (values.includes(null)) {

parts.push(sql.fragment`foo IS NULL`);

values = values.filter((v) => v !== null);

}

if (values.length) {

parts.push(sql.fragment`foo = ANY(${sql.array(values, 'text')})`;

}

return sql.join(parts, ' OR ');

}

You check the tests, they look good, they add data to a testContainer database and verify that the condition works as expected. Test coverage is 100%.

Stop and consider what criticism, if any, you might have of the above change.

Looks pretty good to me!

Except that this code was a near-miss production incident as it risked disastrous leakage of data under some conditions (which luckily did not end up actually happening).

The situation

Here is a sketch of the code that is using the above function:

connection.query(sql.type(resultSchema)`SELECT ... FROM ...

WHERE ... AND ${getFilterConditions()}`);

...

function getFilterConditions() : FragmentSqlToken {

const conditions = [

...

getFooCondition(fooValues),

...

];

return sql.join(conditions, ' AND ');

}

The fundamental issue is a mismatch between the assumptions of the calling code that conditions are "simple" (or at least joinable by AND), and the assumption of the modified code that it was fine to return a complex condition with an OR in it.

An oversimplification of the problem would be to dismiss it as just "bad assumptions". In hindsight, maybe the assumptions could have been avoided in this case. After all it wouldn't really matter if both the caller and the callee wrapped conditions in parentheses.

But some kinds of assumptions must always be made, so a lot may be learned by considering "how can those assumptions be communicated?".

The three levels of correctness

My friend Jimmy Koppel, founder of Mirdin, teaches that correctness of code can be evaluated on three levels.

The first level is that the code works on a particular run with a particular input. This level can be verified by a test case.

The second level is that the code works for all valid input. This is the level where the code could be shipped and there may be valid trade-offs for not going further, for example if the code will be thrown away. The computer scientist's dream scenario for verifying this level of correctness is through a formal proof.

It is often too much work to complete a full proof, but we can get some benefits from a type system. The stronger the type system and the types you define, the better the "proof", but there is probably a sweet spot beyond which stronger types become too restrictive to use daily.

Naming can help offset some deficits in the type system.

Usually, a probabilistic argument is made from the tests that are run, to make up for the deficits in the type definitions.

Yet another possibility, which sadly isn't integrated in most languages, is to define contract checks on preconditions required on inputs and postconditions required of outputs. They can be used to define more precise conditions than is practical or even possible with a type system. And they run on every call to the function, testing all real inputs, not just the fictional inputs in tests.

For completeness, contracts also usually include invariants on loops and classes, but IMO they start to border on too much effort for the value. We do have to consider these things, but formally specifying them feels at least an order of magnitude more difficult than informally reasoning about them. Maybe it gets easier with practice, and I have to admit I have not tried using them in real code.

In practice, type inference combined with some clever test cases is usually good enough for this level of correctness. Stricter typing and more tests do not seem to reduce bugs very much, but formulating them may help the programmer and reviewer think more clearly to better understand where bugs may be lurking.

The third level of correctness is that the code works for all future modifications of the code. An interesting observation here is that a formal proof doesn't necessarily say all that much about the next modification of the code. In order to achieve this level, we can say that the code must successfully communicate its design to the next programmer (which, as always, may be yourself in the future).

That isn't entirely satisfactory, because what constitutes design? And how does one communicate, really?

Peter Naur wrote an essay beginning with examples of the next set of maintainers failing to utilise the design despite ample and clear documentation. His suggestion is that it isn't the design itself, but the theory of the design, the reason the design is the way it is, that needs to be communicated. That is difficult, because it is lodged deep inside the original programmers' brains and cannot be easily described.

Improving communication in the example

What could have communicated the problem in our example? How could a signal have been given to the programmer or the reviewer?

Tests are one way of giving signals. In this case, it would be unlikely to be able to create a focused automated test for this problem because it would require setting up extraneous data that would be returned when the expectation was violated in precisely this way. If the programmer knew to set up a test to pre-empt this problem, surely they would have just fixed their expectation instead? Exploratory testing on a large enough dataset could possibly have found the error, but that is another story.

There is a small clue in the name, getFooCondition, perhaps, if you would interpret the word "condition" as meaning just a simple comparison, but I think an OR-expression also usually qualifies as a "condition". In the postgres manual, a "search_condition" is any value expression that returns a boolean value. I'm not sure the name could be improved much, maybe it could be called createFooComparison instead? Would that have been a stronger clue?

Considering the use of types, while the FragmentSqlToken return type of getFooCondition has some advantages over a plain string, it doesn't reflect the expected algebraic properties of the returned value, namely that it should be a simple condition that can be AND:ed with other conditions.

A good pattern when serializing data is to create values that keep all the required properties right up until the single point where all the values are encoded at the same time.

Actually, I misrepresented the original code slightly: getFooCondition was part of that final step and the input was actually a tagged union of FilterCondition. I suppose that was a better clue to the expected output of the function, but it doesn't provide a signal when violated, so it is easy to miss.

type FilterCondition =

{ operation: 'eq', value: string }

| { operation: 'in', values: string[] };

The reason for having separate functions for different filterable entities was that they were represented differently in the database, some in columns, some in json blobs. Maybe it still would have been better to just leave the code inline so that the final sql.join was easy to see in the flow. Especially since the name getFooCondition is not very descriptive, nor is the FragmentSqlToken return type.

If there is a strong desire to break out the functions creating the individual conditions, the following might work for the return type to explain the desired properties:

type ConditionFragment =

{ type: 'comparison' , fragment: FragmentSqlToken }

| { type: 'or', fragments: ConditionFragment[] }

| { type: 'and', fragments: ConditionFragment[]};

One further note on this example is that the input type was changed from string[] to (string | null)[]. While expedient, it doesn't accurately represent that only one null value is relevant, which may lead to confusion later. It certainly complicated the code a tiny bit, while a better type would have pushed the complication upstream instead, along with the increased clarity.

Communicating consciously through code

naming

Naming things in code is the most important way of communicating things about the code that are not immediately discernible from the operations of the code itself.

Names can be used not only to say what a value or function is, but why it is.

Spend a little extra time on deciding which things need a name and what the best name is. Even small nuances can matter. If it is very difficult to find a good name, it may be a clue that the design of the code should be tweaked.

A compiler or similar pre-processor will usually check that the names align. This is a very handy way to get signals that can be used as reminders to self while coding.

automated tests

Consider that the main purpose of an automated test is to communicate the requirements and the theory of the design to the next maintainer. When the test fails, a signal is sent. In the best case, the signal is very clear and has a relevant meaning.

All too often, I see tests that have a big input blob of data and a big output blob expectation, with no clear guidance as to how they relate. Or the test is full of mocks and verifications that a certain mock was called in a certain way. These kinds of tests only signal "the code changed" and it is often not even possible to get anything more meaningful out of them.

Make sure each test explains exactly why it needs to pass, otherwise it is probably not worth the cost to keep it.

Make sure to clearly show how the relevant parts of the input relate to the expected output. One useful technique is to extract variables for the parts that should be the same, or show by a transformation how the output is derived from the input.

Tests that aren't run don't give any signal at all. Run them as often as you can. Make sure they are fast so that you are willing to do so.

types, preconditions and postconditions

Specifying the requirements on the expected input and the expected output of a function is very helpful as documentation both as to how it should be implemented and how it should be used.

Types and contracts only give a signal if something is checking them, so languages with type checking communicate better than languages without.

Types, contracts, tests and names all interplay. A variable named the same as its type is a possibility to improve communication by renaming the variable, for example to show its role in the algorithm.

code structure

The order and layout of statements in your code can also be used to communicate things and make the theory of the design more apparent.

Putting variable declarations close to their use minimizes the amount of data needed to be kept in memory while reading the code. In general, put things close together that need to be understood together.

The choice between recursion, loops, streams and even what kind of loops conveys different things about how the algorithm is designed.

Making an if-else statement conveys equal importance to the branches, perhaps with a slight preference for the first. Making a guard-clause conveys an exceptional case.

Grouping of functionality in a file with clearly marked sections can be helpful and more lightweight than breaking out a separate file for each.

comments

Thoughtless comments are a waste of time for everybody. Unfortunately their prevalence has made many developers blind to comments in general.

Brief, thoughtful comments, carefully written to explain why the code is as it is or other aspects that weren't possible to communicate any other way can be a life-saver.

To sum up

To fully utilize the design of the code and to make sure code remains correct under future modifications, the future programmer needs to understand the theory of the design. This is difficult, perhaps inexpressible, as it is lodged deep in the original programmers' brains.

The best we can do is to leave clues in the code, guide the reader of the code, and, even better, make sure that signals are fired when the original theory of design is violated.

I have written previously about this subject and about meaningful tests, perhaps there is more to be learned from reading them. In a similar vein I have even longer ago written about clarity of code, assumptions and assumptions again.