Mistral Coding Guidelines

Why learn more coding guidelines?

This document contains the description of the guidelines used for writing code on the Mistral project. Some of the guidelines follow from the nature of Python programming language (dynamic types, etc), some are the result of the consensus achieved by the project contributors during many contribution cycles. All contributors need to follow these guidelines when contributing to Mistral. The purpose of having such well described practices is improving communication between team members, reducing a number of controversial situations related to how a certain code snippet should be written, and letting contributors focus on unique engineering tasks rather than low level decisions that any engineer makes many times a day, like choosing a good name for a class or variable, or how to organise a loop, whether they need to put a blank line before “if” or “return”, in what cases and so on. The document, when accepted and followed by team members, aims to improve overall development speed and quality.

Note that the guidelines described below almost don’t conflict with the official PEP8 style guide (https://www.python.org/dev/peps/pep-0008/) describing how any Python program should be formatted. Mistral guidelines add more high level semantics on top of it. PEP8 still should be considered a necessary base for those who write Python programs. Strictly speaking, this guide is not exactly about style, it’s about writing maintainable code.

Some of the concepts being discussed below may seem a bit too philosophical but, in fact, they reflect our real experience of solving practical tasks. According to it, some decisions work well and some don’t given the Python programming nature and the nature of the project.

The guidelines are based on the three main values:

  • Communication. When writing code we always try to create it in a way that it’s easy to read and understand. This is important because most of the time developers spend on reading existing code, not writing new.

  • Simplicity. It makes sense to write code that uses minimal means that solves a task at hand.

  • Flexibility. In most cases it’s better to keep options open because in programming there’s no such thing as “done”. Pretty much all code gets modified during lifecycle of a program working in production. This is a very important difference form designing real buildings where we can’t rebuild a basement after a building is completed and inhabited by people.

In the document, there is a number of code snippets, some are considered incorrect and some are correct, with explanations why from the perspective of these key values.

Further sections are devoted to certain aspects of writing readable code. They will typically have a list of guidelines upfront so it’s easy to find them in the text and then more details on them, if needed.

Naming

Naming is always important in programing. Good naming should always serve for better communication, i.e. a name of a code entity (module, variable, class etc.) should convey a clear message to a code reader about the meaning of the entity. For dynamic languages naming is even more important than for languages with static types. Below it will be shown why.

Using Abbreviations

Guidelines

  • Use well-known abbreviations to name variables, method arguments, constants

Well-known abbreviations (shortcuts) used for names of constants, local variables and method arguments help simplify code, since it becomes less verbose, and simultaneously improve communication because all team members understand them the same way. Below is a list of abbreviations used on the Mistral project. The list is not final and is supposed to grow over time.

  • app - application

  • arg - argument

  • cfg - configuration

  • cls - class

  • cmd - command

  • cnt - count

  • ctx - context

  • db - database

  • desc - description

  • dest - destination

  • def - definition

  • defs - definitions

  • env - environment

  • ex - execution

  • execs - executions

  • exc - exception

  • log - logger

  • ns - namespace

  • info - information

  • obj - object

  • prog - program

  • spec - specification

  • sync - synchronous

  • wf - workflow

  • wfs - workflows

Note that when using a well-known abbreviation to name a constant we need to use capital letters only (just as required by PEP8).

Local Variables and Method Arguments

Guidelines

  • The name of a method argument should clearly communicate its semantics and purpose

  • The name of a method argument or a local variable should include a type name if it creates an ambiguity otherwise

  • The name of a local variable can be shortened from a full word (up to one letter) if the scope of the variable is very narrow (several lines) and if it doesn’t create ambiguities

Principles of choosing names in dynamic languages like Python are especially important because oftentimes we don’t clearly see of what type a certain variable is. Whereas a type itself (no matter if it’s primitive or represented by a class) carries very important information about all objects of this type. For example, if we see a variable in Java or C++ code it’s not a problem to find out of what type this variable is, we can easily find where the variable is defined. Any modern IDE also allows to navigate to a type declaration (e.g. if it’s a class) just by clicking on it and see all required info about it. In Python, it’s often much more problematic to find a variable type. Injecting a type name, at least a shortcut, into a variable name often helps mitigate this fundamental issue and improve code readability, and hence communication.

Below are the code snippets that help illustrate this problem. Let’s assume we have the following Python classes:

class TaskExecution(Execution):
    # Carries information about a running task.
    ...

class TaskSpec(BaseSpec):
    # Defines a work logic of a particular task.
    ...

For what we want to illustrate, it’s not important if they belong to the same module or different.

Problematic Code

def calculate_next_tasks(self, task, context):
    result = set()

    for clause in task.get_on_clauses():
        task_names = self.evaluate_on_clause(clause, context)

        tasks = [self.get_task_by_name(t_n) for t_n in task_names]

        result.update(tasks)

    return result

Is this method easy to understand? Well, if this code is part of a small program (e.g. a 200-300 lines script) then it may be ok. But when it’s a system with dozens of thousands lines of code then it has a number of issues.

The most important issue is that we don’t know the type of the “task” method argument. In order to find it we’ll have to see where this method is called and what is passed as an argument. Then, if an object is not created there we’ll have to go further and find callers of that method too, and so on until we find where the object is actually instantiated. The longer the method call chains are, the worse. So, jus by looking at this code we can’t determine whether the argument “task” is of type TaskSpec or TaskExecution. So for example, we can’t even say for sure if task.get_on_clauses() is a correct instruction. If we have hundreds of places like this, it is very challenging to read code and it is very easy to make a mistake when modifying it.

The other obvious issues are also related to naming:

  1. It’s not clear objects of what type are returned by the method

  2. It’s not clear objects of what type is returned by self.get_task_by_name(t_n)

Better Code

def calculate_next_task_specs(self, task_spec, context):
    result = set()

    for clause in task_spec.get_on_clauses():
        task_names = self.evaluate_on_clause(clause, context)

        task_specs = [self.get_task_spec_by_name(t_n) for t_n in task_names]

        result.update(task_specs)

    return result

Now we won’t be confused. Of course, we still have to remember we have those two classes but at least we have a clear pointer to a type.

Functions and Methods

Guidelines

  • The name of a method should clearly communicate its semantics and purpose

  • The name of a method should include a type name of a returned value when it creates an ambiguity otherwise

For example, there are two classes like these again:

class TaskExecution(Execution):
    # Carries information about a running task.
    ...

class TaskSpec(BaseSpec):
    # Defines a work logic of a particular task.
    ...

And we need a method that implements some logic and returns an instance of TaskSpec. How would we choose a good name for the method?

Problematic Code

def calculate_parent_task(self, task_spec):
    ...

Looking at this method signature it’s not clear what to expect as a returned value since in Python method declarations don’t contain a returned type value. Although there’s a temptation to use just “task” in the method name it leads to the naming ambiguity: a returned value may be either of TaskExecution or TaskSpec type. Strictly speaking, it may be any type since it’s Python but we can help a reader of this code a little bit and leave a hint about what’s going to be returned from the method when it’s called.

At first glance, it may seem a bit weird why we pay attention to such things. One may say that it’s already totally clear that need to use full “task_spec” in the method name instead of “task” just according to the name of the class. Why anybody may want to use “task”? So, the reality is that during informal communication within a team people tend to simplify/shorten words. That is, when several team members are working on something related to task specifications within a certain scope (module, class, etc.) they often move to using a simplified terminology and instead of saying “task specification” they start saying just “task”. And this kind of habit often also sneaks into code. And eventually it breaks communication between code author and code reader.

Better Code

def calculate_parent_task_spec(self, task_spec):
    ...

Although it’s more verbose, it allows to mitigate the naming ambiguity for a code reader.

Constants

Guidelines

  • All hardcoded values (strings, numbers, etc.) must be defined as constants, i.e. global module variables

  • Constants must be defined in the beginning of a module

Problematic Code

def print_task_report_line(self, line, level=0):
    self.app.stdout.write(
        "task: %s%s\n" % (' ' * (level * 4), line)
    )

...

def print_action_report_line(self, line, level=0):
    self.app.stdout.write(
        "action: %s%s\n" % (' ' * (level * 4), line)
    )

The problem of this code is that it uses hard-coded integer values at different places. Why is it a problem?

  • It’s hard to find hard-coded values while reading code

  • It’s hard to understand whether same values mean semantically the same thing, or different

  • It’s easy to make a mistake when changing a hard-coded value because we can change it at one place and miss other places

Better Code

REPORT_ENTRY_INDENT = 4
DEFAULT_ENTRY_LEVEL = 0

def print_task_report_line(self, line, level=DEFAULT_ENTRY_LEVEL):
    self.app.stdout.write(
        "task: %s%s\n" % (' ' * (level * REPORT_ENTRY_INDENT), line)
    )

...

def print_action_report_line(self, line, level=DEFAULT_ENTRY_LEVEL):
    self.app.stdout.write(
        "action: %s%s\n" % (' ' * (level * REPORT_ENTRY_INDENT), line)
    )

Now the code clearly communicates to a reader that value 4 in these two methods means exactly the same entity: an indent of any report entry. The other constant similarly adds clarity about value 0. Previously, these two integers were nameless. It’s now also easy to change values, we just need to set different values to the named constants.

Grouping and Blank Lines

Guidelines

  • Use blank lines to split individual steps (units) of algorithms.

  • Always put blank lines before and after “if”, “for”, “try” and “with” blocks if they don’t contain one another w/o anything else in between.

  • Always put a blank line before “return” unless it’s the only instruction in en enclosing code block like a method or an “if” block.

  • Use blank lines to split logically not symmetric lines, e.g. less abstract and more abstract lines like variable assignment and a method call.

  • Put a blank line after any call to a superclass.

Although for someone it may not seem important, blank lines consciously put in code can improve readability. The general recommendation is to use blank lines to separate different logical blocks. When writing code it is useful to ask ourselves the question “what are the main steps of the algorithm I’m implementing?”. When answered, it gives understanding of how code can be decomposed into sections. And in order to reflect that they are individual steps of the algorithm, the corresponding code blocks can be split by blank lines. Let’s consider examples.

Problematic Code

def update_task_state(self, state, state_info=None):
    old_task_state = self.task_ex.state
    if states.is_completed(self.task_ex.state):
        self.notify(old_task_state, self.task_ex.state)
        return
    if not states.is_valid_transition(self.task_ex.state, state):
        return
    child_states = [a_ex.state for a_ex in self.task_ex.executions]
    if state == states.RUNNING and states.PAUSED in child_states:
        return
    self.set_state(state, state_info)
    if states.is_completed(self.task_ex.state):
        self.register_workflow_completion_check()
    self.notify(old_task_state, self.task_ex.state)

Is this method easy to read? The method does a lot of things: it invokes other methods, checks conditions, calculates values and sets them to variables. Even more importantly, all of that are different computational steps of the method. However, they appear in the code one by one without any gaps. So when a human eye is reading this code there isn’t any element in the code that would tell us where the next step of the algorithm starts. And a blank line can naturally play a role of such element.

Better Code

def update_task_state(self, state, state_info=None):
    old_task_state = self.task_ex.state

    if states.is_completed(self.task_ex.state):
        self.notify(old_task_state, self.task_ex.state)

        return

    if not states.is_valid_transition(self.task_ex.state, state):
        return

    child_states = [a_ex.state for a_ex in self.task_ex.executions]

    if state == states.RUNNING and states.PAUSED in child_states:
        return

    self.set_state(state, state_info)

    if states.is_completed(self.task_ex.state):
        self.register_workflow_completion_check()

    self.notify(old_task_state, self.task_ex.state)

Now when we read the method, we clearly see the individual steps:

  • Saving an old task state for further usage

  • Check if task is already completed and if it is, notify clients about it and return

  • Check if the state transition we’re going to make is valid

  • Calculate state of the child entities

  • Do not proceed with updating the task state if there any running or paused child entities

  • Actually update the task state

  • If the task is now completed, schedule the corresponding workflow completion check

  • Notify clients about a task transition

Of course, when writing code like this it may be hard to format code this way in the first place. But once we already have some version of code, we should take care of people who will surely be reading it in future. After all, the author may be reading it after some time. Again: programs are read much more often than written. So we need to make sure our code tells a good story about what it serves to.

As far as putting a blank line before “if”, “try”, “for” and “with”, the reasoning is pretty straightforward: all these code flow controls already reflect separate computational steps because they do something that’s different from the previous command by nature. For example, “if” may route a program in a different direction at runtime. So all these blocks should be clearly visible to a reader. “return” is also an outstanding command since it stops the execution of the current method and gives control back to the caller of the method. So it also deserves to be well visible.

Using blank lines consciously can also make code more symmetric. That is, if we don’t mix up significantly different commands.

Problematic Code

var1 = "some value"
my_obj.method1()
my_obj.method2()
var2 = "another value"

... # The rest of the code snippet

What’s wrong with this code? The thing is that we mixed up lines where we do absolutely different things. Two of them just do set string values to the two new variables whereas the other two send messages to a different object, i.e. give it command to do something. In other words, the two lines here are more abstract than the two others since they don’t run any concrete calculation, it is hidden by method calls against other object. So this code is not symmetric, it doesn’t group commands of similar nature together and it doesn’t separate them from each other.

Better Code

var1 = "some value"
var2 = "another value"

my_obj.method1()
my_obj.method2()

... # The rest of the code snippet

This code fixes the mentioned issues and note that, again, a blank line clearly communicates that a more abstract block starts and that this block can and should be maintained separately.

Multi-line Method Calls

Guidelines

  • Long method calls that don’t fit into 80 characters must be written down in a way that each argument is located on an individual line, as well as the closing round bracket.

All code lines need to be not longer than 80 characters. Once in a while it’s required to break lines when we deal with long instructions. For example, when we need to write a method call with lots of arguments or the names of the arguments are long enough so the entire code instruction doesn’t fit into 80 characters.

Problematic Code

executor.run_action(self.action_ex.id, self.action_def.action_class,
    self.action_def.attributes or {}, self.action_ex.input,
    self.action_ex.runtime_context.get('safe_rerun', False),
    execution_context, target=target, timeout=timeout)

On many Python projects this way or breaking lines for long method calls is considered right. However, when we need to read and understand this code quickly we may experience the following issues:

  • Hard to see where one argument ends and where another one starts

  • Hard to check if the order of arguments is correct

  • If such method call declaration is followed by another code line (e.g. another method call) then it’s hard to see where the method call declaration ends

Better Code

executor.run_action(
    self.action_ex.id,
    self.action_def.action_class,
    self.action_def.attributes or {},
    self.action_ex.input,
    self.action_ex.runtime_context.get('safe_rerun', False),
    execution_context,
    target=target,
    timeout=timeout
)

Although the second version of the method call sacrifices conciseness to some extent, it eliminates the issues mentioned above. Every method argument is easily visible, it’s easy to check the number of the arguments and their order (e.g. to compare with the method signature) and it’s easy to see where the entire command ends because the ending round bracket on a separate line communicates it clearly.