Code review page ideas

[adam-carruthers]

We have recently been doing some code reviewing. Here are a few things that we think might make the page more helpful.

Code review before merge request

Code should be reviewed with someone before submitting a merge request. The reviewer should consider whether the code needs to be refactored or redesigned.

I'm not sure that I always agree with this. Merge requests make it really easy to leave comments on different parts of the code, and in some ways make the life of the reviewer and the merge request submitter easier. Maybe rephrase as

You don't have to save reviewing your code until the end. You can do small reviewing and also pair programming while developing the ticket. Seeking feedback sooner could mean you save time because you do not have to change as much when the final review happens later.

Different types of code review

There are different types of code review that you can get. It may be worth highlighting them.

1. Merge request code review

   A standard review process that checks whether changes to the codebase are acceptable. You focus only on the code that has changed. It should be relatively quick, and very regular (one every time you implement a new feature). Normally done by a member of the team.

2. Full code review

   A code review where someone looks at all your code together, and gives you overall feedback. This review allows someone to look at the bigger picture, rather than one individual feature. These reviews take longer, and are less regular. Normally done by members outside your team, so that it is a fresh pair of eyes.

3. Fitness to publish checks

   A code review to check the code is okay to publish. Note that, in the code review, you will normally limit yourself to making suggestions that you want completed before the code is published. This may mean you avoid suggesting big changes to the code, and instead focus in on checks like ensuring documentation is well written, or removing passwords from the code.

Maybe split code review checklist into beginner and advanced items?

One of the items on the code review checklist is

Documentation is hosted for easy access. GitHub Pages and Read the Docs provide a free service for hosting documentation publicly.

Even with advanced teams in data services I do not see them doing this. It might be worth prioritizing, so that the checklist is less overwhelming.

Maybe organise the checklist items by the RAP level the team is aiming for.

[adam-carruthers]

To add a really crazy idea, you could put examples of what kind of review comments to give.

Here's one off the top of my head

---

If you see

def get_dicts():

you could comment

Function name could be more specific, maybe rename load_hospitals_data_dictionary_df?

[SamHollings]

Sorry for the delay on this @goodyguts - made a jra ticket and should get on it soon. https://nhsd-jira.digital.nhs.uk/browse/NV-1415