code_review_checklist.rst

Python Code Review Checklist

General

• Code is blackened with black
• flake8 has been run with no errors
• mypy has been run with no errors
• Function complexity problems have been resolved using the default complexity index of flake8.
• Important core code can be loaded in iPython, ipdb easily.
• There is no dead code
• Comprehensions or generator expressions are used in place of for loops where appropriate
• Comprehensions and generator expressions produce state but they do not have side effects within the expression.
• Use zip(), any(), all(), etc. instead of for loops where appropriate
• Functions that take as parameters and mutate mutable variables don’t return these variables. They return None.
• Return immutable copies of mutable types instead of mutating the instances themselves when mutable types are passed as parameters with the intention of returning a mutated version of that variable.
• Avoid method cascading on objects with methods that return self.
• Function and method parameters never use empty collection or sequence instances like list [] or dict {}. Instead they must use None to indicate missing input
• Variables in a function body are initialised with empty sequences or collections by callables, list(), dict(), instead of [], {}, etc.
• Always use the Final type hint for class instance parameters that will not change.
• Context-dependent variables are not unnecessarily passed between functions or methods
• View functions either implement the business rules the view is repsonsible for or it passes data downstream to have this done by services and receives non-context dependent data back.
• View functions don’t pass request to called functions
• Functions including class methods don’t have too many local parameters or instance variables. Especially a class’ __init__() should not have too many parameters.
• Profiling code is minimal
• Logging is the minimum required for production use
• There are no home-brewed solutions for things that already exist in the PSL

Imports and modules

• Imports are sorted by isort or according to some standard that is consistent within the team
• Import packages or modules to qualify the use of functions or classes so that unqualified function calls can be assumed to be to functions in the current module

Documentation

• Modules have docstrings
• Classes have docstrings unless their purpose is immediately obvious
• Methods and functions have docstrings
• Comments and docstrings add non-obvious and helpful information that is not already present in the naming of functions and variables

General Complexity

• Functions as complex as they need to be but no more (as defined by flake8’s default complexity threshold)
• Classes have only as many methods as required and have a simple hierarchy

Context Freedom

• All important functionality can be loaded easily in ipython without having to construct dummy requests, etc.
• All important functionality can be loaded in pdb (or a variant, ipdb, etc.)

Types

Have immutable types, tuple, frozenset, Enum, etc. been used in place of mutable types whenever possible?

Functions

Functions are pure wherever possible, i.e. they take input and provide a return value with no side-effects or reliance on hidden state.

Modules

• Module level variables do not take context-dependent values like connection clients to remote systems unless the client is used immediately for another module level variable and not used again

Classes

• Every class has a single well-defined purpose. That is, the class does not mix up different tasks, like remote state acquisition, web sockets notification, data formatting, etc.
• Classes manage state and do not just represent the encapsulation of behaviour
• All methods access either cls or self in the body. If a method does not access cls or self, it should be a function at module level.
• @classmethod is used in preference to @staticmethod but only if the method body accesses cls otherwise the method should be a module level function.
• Constants are declared at module level not in methods or class level
• Constants are always upper case
• Abstract classes are derived from abc: from abc import ABC
• Abstract methods use the @abstractmethod decorator
• Abstract class properties use both @abstractmethod and @property decorators
• Classes do not use multiple inheritance
• Classes do not use mixins (use composition instead) except in rare cases
• Class names do not use the word “Base” to signal they are the single ancestor, like “BaseWhatever”
• Decorators are not used to replace classes as a design pattern
• __init__() does not define too many local variables. Use the Parameter Consolidation pattern instead.
• A factory class or function at module level is used for complex class construction (see Design Patterns) to achieve composition
• Classes are not dynamically created from strings except where forward reference requires this

Design Patterns

• Do not use designs that cause a typical Python developer to have to learn new semantics that are unexpected in Python
• Classes primarily use composition in preference to inheritance
• Beyond a very small number of simple variables, a class’ purpose is to acquire state for another class or it uses another class to acquire state in particular if the state is from a remote service.
• If you use the Context Parameter pattern, it is critical that the state of the context does not change after calling its __init__(), i.e. it should be immutable
• If a class’ purpose is to represent an external integration, you probably want numerous classes to compose the service: RemoteDataClient, DomainManager, ContextManager, Factory, NotificationController, DomainResponse, DataFormatter, etc.