Make the code review section more engaging

[thomaskileyukaea]

Currently the code review section ends with a long bullet point list of best practices. I propose restructuring this section:

• Go through technical steps to raise a review
• Get user to raise a review based on an example branch that will be included in the repo template
• Go through technical steps of adding a comment, get user to add a comment
• Cover some of the things that a review should look for
• Get user to go through the code looking for those things
• Repeat last two points if there are more than 6 things to look for
• Suggest a different person then attempts to address their review comments (or address their own if doing on own)
• Talk about empathy (and any other review considerations)

Steps to do this:

• Identify the kind of things to look for in a review
• Propose new structure for the section
• Develop a piece of code that exhibits the problems
• Write the exercises and example solutions
• Add new conent
• Modify the content for creating a new pull request (this is quite different)
• Modify existing content
• Read through and make final tweaks
  (I intend to address this, raising issue for visibility and in case anyone disagrees).

[thomaskileyukaea]

Also want to remove the idea of a feedback sandwich as that is not considered good advice as it undermines future positive feedback (people think it is just to make them feel better) and obscures the constructive feedback you are trying to give. Give constructive feedback kindly but directly.

[thomaskileyukaea]

The things to look for in a review:

• Readability
• Minimal
• Clean structure - appropriate abstractions, functions doing one thing etc
• Are their suitable tests for all new and modified code
• Is the documentation appropriate

Things not to look for in review

• bugs (add a test)
• automatable things like linting (add to CI)
• full scale re-writes (discuss offline, ideally before large feature is written!)
• every imperfection (continuous improvement not perfection)

Other things that might be worth folding in:

• Making code easier to review
• Compassion in review comments
• Tips for making the process work well (including explicit communication about intended actions)
• Classification of kinds of changes (must change, ideas etc)
• Questions being answered by code change (if a function doesn't make sense then change the code to make it make sense)

Make sure all the benefits of code review are covered, in particular around the idea of sharing knowledge and best practices.

[thomaskileyukaea]

Proposed new structure based on old structure:

• Introduction
• Collaborative code models
• Exercise: Advantages of code review
• Benefits of code review - Elaborate a little on knowledge sharing
• Types of code review
• Introduction to GH pull requests
• How to raise a pull request
• Pull request in action explanation
• Adding someone as collaborator instructions
• Preparing local enviroment for a pull request (Developing a new feature)
• Write tests for someone elses code
• Submitting a pull request (merge with above)
• Short section on code review (elaborate a bit on specifics of adding a comment)
• ++ Things to look for when reviewing - readability, minimal, clean, documentaion
• ++ Exercise: Look for these issues in the PR and add comments
• ++ Things to look for - building a test list
• ++ Exercise: Build a missing test list and cross off the tests found
• ++ Things not to look for
• ++ Responding to review comments
• ++ Exercise: respond to review comments
• Closing a pull request
• Code review tips (probably reduced as most of these will be folded into other sections)
• ++ Making code easy to review
• ++ Empathy in review comments
• ++ Designing a code review process
• Exercise: Code Review In Your Own Working Enviroment (designing a code review process)

(Crossed out sections are removed, italics would be modified, and things with ++ would be an addition. Feedback welcome

[thomaskileyukaea]

I intend to put the following code failings into the code

Readability

• poor function name
• unclear variable name

Minimal

• reimplementing standard deviation

Clean structure

• returning a different structure into the view data

Documentation

• Slightly inaccurate

Tests

• ideally include a small bug not caught by tess
• a spuriously passing test

[bielsnohr]

@anenadic and @steve-crouch a new branch within our repo has now been created for this work by @thomaskileyukaea : rework-code-review-section.

It is on our plates to determine the specific points that need to change for it to get merged into the actual lesson content.