Investing in code reviews for better research software

Part 0

What is a code review?

Code review?

Main benefits:

1. Catching bugs

2. Ensuring quality standard

3. Spreading knowledge

4. Training new developers

Not a peer review for code

• Code review throughout the research process:
  • Frequent
  • Informal
  • Low stakes
• Commonly referred as “Modern Code Review” in the SE literature. Bachelli and Bird 2013
• Can be asynchronous (GitHub’s Pull Requests) or synchronous (in person chat).

CODECHECK

From formal inspections to code review

Asynchronous Code Review

Synchronous Code Review

Elise Özalp, Yaxin, Defne Ozan, Daniel Kelshaw (https://magrilab.ae.ic.ac.uk), Thibault Lestang. Photo cred: Neil Montague.

Department of Aeronautics, Imperial College London

Two contexts

1. Individual developers writing their own specific software.
2. Developers collaborating on a common codebase.
   • Code review as gatekeeping.

Research on code reviews

Modern Code Review: A Case Study at Google (Sadowski, 2018)

Expectations, Outcomes, and Challenges of Modern Code Review (Bacchelli and Bird, 2013)

Code Reviewing in the Trenches: Understanding Challenges and Best Practices (McLeod et al, 2017)

Code review by and for scientists (Petre & Wilson, 2014)

Part 1

Benefits of code reviews for research software

Code review for software quality

1. Defects
2. Code improvements

Code review for software quality

Code reviews for understandability

More often than not source code is the only available form of documentation.

Understandability is key for code reuse and transparency.

Code reviews for team awareness

• Continuous knowledge exchange.
• Enhanced collaboration.
• Longer term resilience of project(s) (Bus factor!).

Code reviews for team awareness

Elise Özalp, Yaxin, Defne Ozan, Daniel Kelshaw (https://magrilab.ae.ic.ac.uk), Thibault Lestang. Photo cred: Neil Montague.

Department of Aeronautics, Imperial College London

Code reviews for knowledge transfer

Code review is peer learning.

• Spread of good practices.
• Homogeneisation of styles and practices.

filepath = "/my/own/specific/path/" + "data.csv"

from pathlib import Path
# ...
datadir_path = Path("/my/own/specific/path/")
filepath = datadir_path / "data.csv"

Part 2: Challenges

A lot of good practices around…

…but what about research software?

Code review is time and energy

Two complementary courses of actions:

Being protective about code

1. There can be some unhealthy competition going on.
2. A large number of researchers feel shy about their coding practices:

• Lack of training.
• Other priorities, often structural (e.g. funding).
• Why would I share my code if nobody else does?

Strong heterogeneity among team members

• Experience.
• Skills (e.g. programming languages).
• Interest & motivation.

Other challenges

• Finding reviewers.
• Finding guidance or mentors.

Part 3: Code review good practices

A lot of the good practices from software engineering industry are applicable, with a pinch of salt.

Keep it short

3 times 30’ instead of one time 90’

• Fit in a busy schedule.
• Doesn’t feel like a big commitment.
• Code review can be a very demanding activity.

Remember that software is often not the primary driver.

Avoid comfort mode

That doesn’t look right but I guess that’s okay…

I just must have missed something…

The author’s part

The author’s part

The author’s part

• Keep it small! (~30’).
• Provide a description of the purpose and structure of the code.
• Think ahead what reviewers will and will not be familiar with
  • Specific libraries?
  • Specific domain knowledge?
• Ensure minimum quality standard (e.g. style, naming).

As a reviewer, think from your reviewer(s)’ perspective.

Specify the feedback you are after

I’m not happy with this loop

for i in `seq 1 $NUMOFFIG`
do
  FIG=$(ls $IMDIR | head -n $i | tail -n 1)
  echo "    ${placeholderpath}/${FIG}" >> $FILE
done

Define (and enforce) a scope

Example default scope: understandability

• Obscure variable names.
• Complex conditionals.
• Duplicated code.
• Long parameter lists.
• Shallow modules.
• Standard compliance.
• Performance sinks.
• Security concerns.

Default scope can be overrriden at will.

Whether “it works” or not is irrelevant

• Code review is not an evaluation of a finished product.
• It is more rewarding to look at code that is WIP.
• The only expectation is that code is readable by reviewers.

Make it formal – but safe

Code review is more effective with a clear process (formal)

At the same time, Code review must occur within an inclusive and supporting space.

It’s about creating an environment where people feel confident about discussing their code with each other.

Overheard in the next meeting room

Author: This loop I wrote looks too complicated to me.

All feedback isn’t helpful

…at least for now.

Reviewers with more programming experience/enthusiasm must be careful not to overwhelm beginners.

Use a checklist

• Poor formatting.
• Dead code.
• Missing documentation.
• Obscure names.
• Complex conditionals.
• Obscure one-liners.
• Duplicated code.
• Long procedures.
• Long parameter lists.
• Global state.
• Abuse of primitive types.
• Data clumps.
• …

Critique the code, not the programmer

You clearly made little effort in naming things…

You should name this differently

This doesn’t make sense

Giving feedback is not trivial

1. Own you opinions.
2. Make it about the code.
3. Be specific.
4. Suggest an alternative.

Code review is both technical and social

Code reviews can drive both inclusion and exclusion.

Define (and refine) a policy

• Well defined process.
• Default scope.
• Moderator(s).
• Code of conduct.
• Conflict resolution.

A culture of openess and collaboration

• Components of a successful software project are
  • Code
  • People
  • Communication
• Research code review goes along with collective ownersip of research project.

Code Review at Anaconda

Code Review at Anaconda

(Asynch) Code Review plays a huge role in Anaconda

Code Review at Anaconda

(Author’s Checklist)

• Always communicate up-front changes to an existing code base, i.e. Open an Issue in the repo

  • This also works well because GH allows to refer & close related issues in a PR
  • Helps also with changes already discussed / in progress

Code Review in Anaconda

(Reviewer’s Checklist)

• TOP of the list: Being considerate; giving recommendations, and motivations (why!)

Thank you!

Questions?