diff --git a/_episodes/41-code-review.md b/_episodes/41-code-review.md
index f9414206e..2ea636de5 100644
--- a/_episodes/41-code-review.md
+++ b/_episodes/41-code-review.md
@@ -1,7 +1,7 @@
---
title: "Developing Software In a Team: Code Review"
-teaching: 15
-exercises: 30
+teaching: 30
+exercises: 40
questions:
- "How do we develop software in a team?"
- "What is code review and how it can improve the quality of code?"
@@ -12,6 +12,7 @@ keypoints:
- "Code review is a team software quality assurance practice where team members look at parts of the codebase in order to improve their code's readability, understandability, quality and maintainability."
- "It is important to agree on a set of best practices and establish a code review process in a team to help to
sustain a good, stable and maintainable code for many years."
+layout: episode_mermaid
---
## Introduction
@@ -71,32 +72,6 @@ and something you should adopt in your development process too.
where one or several people from the team (different from the code's author)
check the software by viewing parts of its source code.
-> ## Group Exercise: Advantages of Code Review
-> Discuss as a group: what do you think are the reasons behind, and advantages of, code review?
-> > ## Solution
-> > The purposes of code review include:
-> > - improving internal code readability, understandability, quality and maintainability
-> > - checking for coding standards compliance, code uniformity and consistency
-> > - checking for test coverage and detecting bugs and code defects early
-> > - detecting performance problems and identifying code optimisation points
-> > - finding alternative/better solutions.
-> >
-> > An effective code review prevents errors from creeping into your software
-> > by improving code quality at an early stage of the software development process.
-> > It helps with learning, i.e. sharing knowledge about the codebase,
-> > solution approaches,
-> > expectations regarding quality,
-> > coding standards, etc.
-> > Developers use code review feedback from more senior developers
-> > to improve their own coding practices and expertise.
-> > Finally, it helps increase the sense of collective code ownership and responsibility,
-> > which in turn helps increase the "bus factor"
-> > and reduce the risk resulting from information and capabilities
-> > being held by a single person "responsible" for a certain part of the codebase
-> > and not being shared among team members.
-> {: .solution}
-{: .challenge}
-
Code review is one of the most useful team code development practices -
someone checks your design or code for errors, they get to learn from your solution,
having to explain code to someone else clarifies
@@ -118,48 +93,21 @@ as close as possible to the point where they were introduced.
There are several **code review techniques** with various degree of formality
and the use of a technical infrastructure, including:
-- **Over-the-shoulder code review**
- is the most common and informal of code review techniques and involves
- one or more team members standing over the code author's shoulder
- while the author walks the reviewers through a set of code changes.
-- **Email pass-around code review**
- is another form of lightweight code review where the code author
- packages up a set of changes and files and sends them over to reviewers via email.
- Reviewers examine the files and differences against the code base,
- ask questions and discuss with the author and other developers,
- and suggest changes over email.
- The difficult part of this process is the manual collection the files under review
- and noting differences.
-- **Pair programming**
- is a code development process that incorporates continuous code review -
- two developers sit together at a computer,
- but only one of them actively codes whereas the other provides real-time feedback.
- It is a great way to inspect new code and train developers,
- especially if an experienced team member walks a younger developer through the new code,
- providing explanations and suggestions through a conversation.
- It is conducted in-person and synchronously but it can be time-consuming
- as the reviewer cannot do any other work during the pair programming period.
-- **Fagan code inspection**
- is a formal and heavyweight process of finding defects in specifications or designs
- during various phases of the software development process.
- There are several roles taken by different team members in a Fagan inspection
- and each inspection is a formal 7-step process with a predefined entry and exit criteria.
- See [Fagan inspection](https://en.wikipedia.org/wiki/Fagan_inspection)
- for full details on this method.
-- **Tool-assisted code review**
- process uses a specialised tool to facilitate the process of code review,
- which typically helps with the following tasks:
- (1) collecting and displaying the updated files and highlighting what has changed,
- (2) facilitating a conversation between team members (reviewers and developers), and
- (3) allowing code administrators and product managers
- a certain control and overview of the code development workflow.
- Modern tools may provide a handful of other functionalities too, such as metrics
- (e.g. inspection rate, defect rate, defect density).
-
-Each of the above techniques have their pros and cons and varying degrees practicality -
-it is up to the team to decide which ones are most suitable for the project and when to use them.
-We will have a look at the **tool-assisted code review process**
-using GitHub's built-in code review tool - **pull requests**.
+ - [Over-the-shoulder code review](https://about.gitlab.com/topics/version-control/what-is-code-review/#Over-the-shoulder%20reviews) -
+ have one developer talk the other developer through the changes whilst sat at the same machine.
+ - [Pair programming](https://about.gitlab.com/topics/version-control/what-is-code-review/#Pair%20programming) -
+ have two developers work on the code at the same time.
+ - [Formal code inspection](https://en.wikipedia.org/wiki/Fagan_inspection) -
+ have up to 6 partipants go through a formalised process to inspect the code for defects.
+ - [Tool assisted code review](https://about.gitlab.com/topics/version-control/what-is-code-review/#Tool-assisted%20reviews) -
+ have a developer uses tools such as GitHub to review the code independently and give feedback.
+
+You can read more about these techniques in [Five Types of Review](https://www.khoury.northeastern.edu/home/lieber/courses/cs4500/f07/lectures/code-review-types.pdf)
+
+It is worth trying multiple code review techniques to see what works
+best for you and your team.
+We will have a look at the **tool-assisted code review process**, which is likely to be the most effective and efficient.
+We will use GitHub's built-in code review tool - **pull requests**, or PRs.
It is a lightweight tool, included with GitHub's core service for free
and has gained popularity within the software development community in recent years.
@@ -188,7 +136,7 @@ create a pull request to bring the changes back to the branch that you started f
In this context, the branch from which you branched off to do your work
and where the changes should be applied back to
is called the **base branch**,
-while the feature branch that contains changes you would like to be applied is the **head branch**.
+while the feature branch that contains changes you would like to be applied is the **compare branch**.
How you create your feature branches and open pull requests in GitHub will depend on
your collaborative code development model:
@@ -214,286 +162,495 @@ and update your proposed changes within a self-contained bundle.
The only difference in creating a pull request between the two models is
how you create the feature branch.
In either model, once you are ready to merge your changes in -
-you will need to specify the base branch and the head branch.
+you will need to specify the base branch and the compare branch.
## Code Review and Pull Requests In Action
Let's see this in action -
-you and your fellow learners are going to be organised in small teams
-and assume to be collaborating in the shared repository model.
-You will be added as a collaborator to another team member's repository
-(which becomes the shared repository in this context)
-and, likewise, you will add other team members as collaborators on your repository.
-You can form teams of two and work on each other's repositories.
-If there are 3 members in your group you can go in a round robin fashion
-(the first team member does a pull request on the second member's repository
-and receives a pull request on their repository from the third team member).
-If you are going through the material on your own and do not have a collaborator,
-you can do pull requests on your own repository from one to another branch.
-
-Recall [solution requirements SR1.1.1 and SR1.2.1](../31-software-requirements/index.html#solution-requirements)
+you are going to act as a reviewer on a change to the codebase raised by a
+fictional colleague on one of your course mate's repository.
+Once this is done, you will then take on the role of the fictional colleague
+and respond to the review on your repository.
+If you are completing this course by yourself, you can raise the review on
+your own repository, review it there and finally respond to your own review
+comments. This is actually a very sensible thing to do in general - looking
+at your own code in a review window will allow you to spot mistakes you
+didn't see before!
+
+Here is an outline of the process of a tool based code review that we will be following:
+
+```mermaid
+%% Must disable useMaxWidth to have the diagram centre
+%%{init: { 'sequence': {'useMaxWidth':false} } }%%
+sequenceDiagram
+ participant A as Author
+ participant R as Reviewer
+ A->>A: Write some code
+ A->>R: Raise a pull request
+ R->>R: Add comments to a review
+ R->>A: Submit a review
+ loop Until approved
+ A->>R: Address or respond to review comments
+ R->>A: Clarify or resolve comments
+ end
+ R->>A: Approve pull request
+ A->>A: Merge pull request
+```
+
+Recall [solution requirements SR1.1.1](../31-software-requirements/index.html#solution-requirements)
from an earlier episode.
-Your team member has implemented one of them according to the specification
-(let's call it `feature-x`)
-but tests are still missing.
-You are now tasked with implementing tests on top of that existing implementation
-to make sure the new feature indeed satisfies the requirements.
-You will propose changes to their repository
-(the shared repository in this context)
-via pull request (acting as the code author)
-and engage in code review with your team member (acting as a code reviewer).
-Similarly, you will receive a pull request on your repository from another team member,
-in which case the roles will be reversed.
-The following diagram depicts the branches that you should have in the repository.
-
-{: .image-with-shadow width="800px"}
-
-Adapted from Git Tutorial by sillevl (Creative Commons Attribution 4.0 International License)
-
-
-To achieve this, the following steps are needed.
-
-#### Step 1: Adding Collaborators to a Shared Repository
-
-You need to add the other team member(s) as collaborator(s) on your repository
-to enable them to create branches and pull requests.
-To do so, each repository owner needs to:
-
-1. Head over to Settings section of your software project's repository in GitHub.
- {: .image-with-shadow width="900px"}
-2. Select the **vertical** tab 'Collaborators' from the left and click the 'Add people' button.
- {: .image-with-shadow width="900px"}
-3. Add your collaborator(s) by their GitHub username(s), full name(s) or email address(es).
- {: .image-with-shadow width="900px"}
-4. Collaborator(s) will be notified of your invitation to join your repository
- based on their notification preferences.
-5. Once they accept the invitation, they will have the collaborator-level access to your repository
- and will show up in the list of your collaborators.
-
-See the full details on
-[collaborator permissions for personal repositories](https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-github-user-account/managing-user-account-settings/permission-levels-for-a-user-account-repository)
-to understand what collaborators will be able to do within your repository.
-Note that repositories owned by an organisation have a
-[more granular access control](https://docs.github.com/en/get-started/learning-about-github/access-permissions-on-github)
-compared to that of personal repositories.
-
-#### Step 2: Preparing Your Local Environment for a Pull Request
-
-1. Obtain the GitHub URL of the shared repository you will be working on and clone it locally
- (make sure you do it outside your software repository's folder you have been working on so far).
- This will create a copy of the repository locally on your machine
- along with all of its (remote) branches.
- ~~~
- $ git clone
- $ cd
- ~~~
- {: .language-bash}
-2. Check with the repository owner (your team member)
- which feature (SR1.1.1 or SR1.2.1) they implemented in the
- [previous exercise](/32-software-design/index.html#implement-requirements)
- and what is the name of the branch they worked on.
- Let's assume the name of the branch was `feature-x`
- (you should amend the branch name for your case accordingly).
-3. Your task is to add tests for the code on `feature-x` branch.
- You should do so on a separate branch called `feature-x-tests`,
- which will branch off `feature-x`.
- This is to enable you later on to create a pull request
- from your `feature-x-tests` branch with your changes
- that can then easily be reviewed and compared with `feature-x`
- by the team member who created it.
-
- To do so, branch off a new local branch `feature-x-tests` from the remote `feature-x` branch
- (making sure you use the branch names that match your case).
- Also note that, while we say "remote" branch `feature-x` -
- you have actually obtained it locally on your machine when you cloned the remote repository.
- ~~~
- $ git checkout -b feature-x-tests origin/feature-x
- ~~~
- {: .language-bash}
-
- You are now located in the new (local) `feature-x-tests` branch
- and are ready to start adding your code.
-
-#### Step 3: Adding New Code
-
-> ## Exercise: Implement Tests for the New Feature
-> Look back at the
-> [solution requirements](/31-software-requirements/index.html#solution-requirements)
-> (SR1.1.1 or SR1.2.1)
-> for the feature that was implemented in your shared repository.
-> Implement tests against the appropriate specification in your local feature branch.
+Your fictional colleague has implemented it according to the specification
+and pushed it to a branch `feature-std-dev`.
+You will turn this branch into a pull request for your fictional colleague on your
+repository.
+You will then engage in code review for the change (acting as a code reviewer) on
+a course mate's repository.
+Once complete, you will respond to the pull request on your repository from another team member.
+
+## Raising a pull request for your fictional colleague
+
+1. Head over to your remote repository in GitHub.
+2. Navigate to the pull requests tab
+3. Create a new pull request by clicking the green `New pull request` button.
+ {: .image-with-shadow width="900px"}
+4. Select the base and the compare branch, e.g. `main` and `feature-std-dev`, respectively.
+ Recall that the base branch is where you want your changes to be merged
+ and the compare branch contains your changes.
+5. Click `Create pull request` to open the request
+ {: .image-with-shadow width="900px"}
+6. Add a comment describing the nature of the changes,
+ and then submit the pull request by clicking `Create pull request`.
+{: .image-with-shadow width="900px"}
+7. At this point, the code review process is initiated.
+
+We will now discuss what to look for in a code review,
+before practising this on this fictional change.
+
+## Reviewing a pull request
+
+Once a review has been raised it is over to the reviewer to review the code
+and submit a review.
+
+### Things to look for in a code review
+
+Reviewing code effectively takes practice.
+However, here is some guidance on the kinds of things you should
+be looking for when reviewing a piece of code.
+
+Start by understanding what the code _should_ do, by reading the specification/user requirements,
+the pull request description and talking to the developer.
+In this case, understand what [SR1.1.1](../31-software-requirements/index.html#solution-requirements) means.
+
+Once you're happy, start reading the code (skip the test code for now). You're
+going to be assessing the code in 4 key areas:
+
+* Is the code readable
+* Is the code a minimal change
+* Is the structure of the code clear
+* Is there appropriate and up-to-date documentation
+
+**Is the code readable**
+
+* Think about do the names of the variables, do they [follow guidelines for good
+names?](../15-coding-conventions/index.html#l#naming-conventions)
+* Do you understand what conditions in each if statements are for?
+* Do the function names match the behavior of the function.
+
+**Is the code a minimal change**
+
+* Does the code reimplement anything that already exists, either
+elsewhere in the codebase or in a library you know about?
+* Does the code implement something that isn't on the ticket?
+
+**Is the structure of the code clear**
+
+* Do functions do just one thing?
+* Is the code using the right level of modularity?
+* Is the code consistent with the structure of the rest of the code?
+
+**Is there appropriate and up-to-date documentation**
+
+* If functionality has changed, has corresponding documentation been
+updated?
+* If new functions have been added, do they have appropriate
+levels of documentation?
+* Does the documentation make sense?
+* Are there clear and useful comments that explain complex designs
+and focus on the "why/because" rather than the "what/how"?
+
+### Adding a review comment
+
+To add comments to a review, you need to open up the pull request.
+Then go to the `Files changed` tab.
+{: .image-with-shadow }
+When you find a line that you want to add a comment to, click on the
+plus (+) icon next to line.
+{: .image-with-shadow }
+You can also add comments referring to multiple lines by clicking the plus and
+dragging down over the relevant lines.
+
+If you want to make a concrete suggestion, such as renaming a variable, you
+can click the `Add a suggestion button` (which looks like a document with a plus and a minus in it).
+This will populate the comment with the existing code, and you can edit it to be
+what you think the code should be.
+{: .image-with-shadow }
+GitHub will then provide a button for the author to apply your change directly.
+
+Write your comment in the box, and then click `Start review`.
+This will save your comment, but not publish it yet.
+
+You can use `Add single comment` to immediately post a comment.
+However, it is best to batch the comments into a single review, so that the author
+knows when you have finished adding comments
+(and avoid spamming their email with notifications).
+
+Continue adding comments in this way, using the `Add review comment` button
+on subsequent comments.
+
+> ## Effective review comments
>
-> *Note: Try not to not fall into the trap of
-> writing the tests to test the existing code/implementation -
-> you should write the tests to make sure the code satisfies the requirements
-> regardless of the actual implementation.
-> You can treat the implementation as a
-> [black box](https://en.wikipedia.org/wiki/Black-box_testing) -
-> a typical approach to software testing -
-> as a way to make sure it is properly tested against its requirements
-> without introducing assumptions into the tests about its implementation.*
+> * Make sure your review comments are specific and actionable.
+> * Try to be as specific as you can, rather than "this code is unclear"
+> prefer, "I don't understand what values this variable can hold".
+> * Make it clear in the comment if you want something to change as part
+> of this pull request.
+> * Ideally provide a concrete suggestion (e.g. better variable name).
+{: .callout}
+
+> ## Exercise: review some code
+>
+> Pair up in the group and go to the pull request they created on their repo.
+> If there are an odd number of people in your group, three people can go in a round robin fashion
+> (the first team member will review the pull request on the second member’s repository
+> and receives comments on the pull request on their repository from
+> the third team member).
+> If you are going through the material on your own and do not have a collaborator,
+> you can be the reviewer on the pull requests on your own repository.
+>
+> Review the code, looking for the kinds of problems that we have just discussed.
+> There are examples of all the 4 main areas in the pull request,
+> so try to make at least one suggestion for each area.
+>
+> Don't submit your review just yet.
+>
+>> ## Solution
+>>
+>> Here are some of the things you might have found were wrong with the code:
+>>
+>> **Is the code readable**
+>>
+>> * Unclear function name `s_dev` - uses an uncommon abbreviation increasing mental load
+>> when reading code that calls this function, prefer `standard_deviation`.
+>> * Variable `number` not clear what it contains --- prefer business-logic name like `mean` or `mean_of_data`
+>>
+>> **Is the code minimal**
+>>
+>> * Could have used `np.std` to compute standard deviation of data without having to reimplement
+>> from scratch.
+>>
+>> **Does the code have a clean structure**
+>>
+>> * Have the function return the data, rather than having the graph name (a view layer consideration)
+>> leak into the model code.
+>>
+>> **Is the documentation up to date and correct**
+>>
+>> * The docs say it returns the standard deviation, but it actually returns a dictionary containing
+>> the standard deviation.
+> {: .solution}
{: .challenge}
-> ## Testing Based on Requirements
-> Tests should test functionality,
-> which stem from the software requirements,
-> rather than an implementation.
-> Tests can be seen as a reflection of those requirements -
-> checking if the requirements are satisfied.
+### Making sure code is valid
+
+The other key thing you want to verify in code review is that the code is correct and
+well tested.
+One approach to do this is to build up a list of tests you expect to see
+(and the results you'd expect them to have),
+and then verify that all these tests are present and correct.
+
+Start by listing out all the tests you'd expect to see based on the specification.
+
+As you are going through the code, add to this list with any more tests you think
+of, making sure to add tests for:
+
+* All paths through the code.
+* Making each `if` statement be evaluated as `True` and `False`.
+* Executing loops with empty, single and multi-element sequences.
+* Edge cases that you spot.
+* Any circumstances that you're not sure how certain code would behave.
+
+Once you have built the list, go through the tests in the pull request. Make sure
+the tests test what you expect (so inspect them closely!).
+
+## Submit a review
+
+Once you have a list of tests you want the author to add, it is time to
+submit your review.
+
+To do this, click the `Finish your review` button at the top of the Files
+changed tab.
+
+
+
+In the comment box, you can add any comments that aren't
+associated with a specific line.
+For example, you can put the list of tests that you want to see
+added here.
+
+Next you will select to one of `Comment`, `Approve` or `Request changes`.
+
+* Use `Approve` if you would be happy for the code to
+go in with no further changes.
+* Use `Request changes` to communicate to the author that
+they should address your comments before you will approve it.
+* Use `Comment` if you don't want to express a decision on
+whether the code should be accepted. For example, if you've been asked
+to look at a specific part of the code, or if you are part way through
+a review, but wanted to share some comments sooner.
+
+Finally, you can press `Submit review`.
+This will publish all the comments you've made on the review and
+let the author know that the review is complete and it is their
+turn for action.
+
+> ## Exercise: review the code for suitable tests
+>
+> Remind yourself of the specification of SR1.1.1 and write a list of
+> tests you'd expect to see for this feature.
+> Review the code again and expand this list to include any other
+> edge cases the code makes you think of.
+> Go through the tests in the pull request and work out which tests are present.
+>
+> Once you are happy, you can submit your review.
+> Select `Request changes` to let the author know they need to address your comments.
+>
+>> ## Solution
+>>
+>> Your list might include the following:
+>>
+>> 1. Standard deviation for one patient with multiple observations.
+>> 2. Standard deviation for two patients.
+>> 3. Graph includes a standard deviation graph.
+>> 4. Standard deviation function should raise an error if given empty data.
+>> 5. Computing standard deviation where deviation is different from variance.
+>> 6. Standard deviation function should give correct result given negative inputs.
+>> 7. Function should work with numpy arrays
+>>
+>> Looking at the tests in the PR, you might be content that tests for 1, 4 and 7 are present
+>> so you would request changes to add tests 2, 3, 5 and 6.
+>>
+>> In looking at the test you hopefully noticed that the test for numpy arrays is currently
+>> spuriously passing as it does not use the return value from the function in the assert.
+>>
+>> You may have spotted that the function actually computes the variance rather than
+>> the standard deviation. Perhaps that is even what made you think to add the test
+>> for some data where the variance and standard deviation are different.
+>> In more complex examples, it is often easier to spot code that looks like it could be wrong
+>> and think of a test that will exercise it. This saves embarrassment if the code turns out
+>> to be right, means you have the missing test written if it is wrong, and is often quicker
+>> than trying to execute the code in your head to find out if it is correct.
+>>
+> {: .solution}
+{: .challenge}
+
+> ## What *not* to look for in a code review
+>
+> The overriding priority for reviewing code should be making sure progress is being made -
+> don't let perfect be the enemy of good here.
+> According to [“Best Kept Secrets of Peer Code Review” (Cohen, 2006)](https://www.amazon.co.uk/Best-Kept-Secrets-Peer-Review/dp/1599160676)
+> it has been shown that the first hour of reviewing code is the most effective,
+> with diminishing returns after that.
+>
+> To that end, here are a few things you *shouldn't* be trying to spot when reviewing:
+>
+> * Linting issues, or anything else that an automated tool can spot - get CI to do it.
+> * Bugs - instead make sure there are tests for all cases.
+> * Issues that pre-date the change - raise a PR fixing these issues separately to avoid heading down a rabbit hole.
+> * Architecture re-writes - try to have design discussions upfront,
+ or else have a meeting to decide whether the code needs to be rewritten.
{: .callout}
-Remember to commit your new code to your branch `feature-x-tests`.
-
-~~~
-$ git add -A
-$ git commit -m "Added tests for feature-x."
-~~~
-{: .language-bash}
-
-#### Step 4: Submitting a Pull Request
-
-When you have finished adding your tests
-and committed the changes to your local `feature-x-tests`,
-and are ready for the others in the team to review them,
-you have to do the following:
-
-1. Push your local feature branch `feature-x-tests` remotely to the shared repository.
- ~~~
- $ git push -u origin feature-x-tests
- ~~~
- {: .language-bash}
-2. Head over to the remote repository in GitHub
- and locate your new (`feature-x-tests`) branch from the dropdown box on the Code tab
- (you can search for your branch or use the "View all branches" option).
- {: .image-with-shadow width="600px"}
-3. Open a pull request by clicking "Compare & pull request" button.
- {: .image-with-shadow width="900px"}
-4. Select the base and the head branch, e.g. `feature-x` and `feature-x-tests`, respectively.
- Recall that the base branch is where you want your changes to be merged
- and the head branch contains your changes.
-5. Add a comment describing the nature of the changes,
- and then submit the pull request.
-6. Repository moderator and other collaborators on the repository (code reviewers)
- will be notified of your pull request by GitHub.
-7. At this point, the code review process is initiated.
+## Responding to review comments
+
+When you receive comments on your review,
+there are a few different things that you will want to do.
-You should receive a similar pull request from other team members on your repository.
+With some, you will understand and agree with what the reviewer is saying.
+With these comments, you should make the change to your code on your branch.
+Once you've made the change you can commit it.
+It might be helpful to add a thumbs up reaction to the comment, so the reviewer knows
+you have addressed it.
-#### Step 5: Code Review
+
-1. The repository moderator/code reviewers reviews your changes
- and provides feedback to you in the form of comments.
-2. Respond to their comments and do any subsequent commits,
- as requested by reviewers.
-3. It may take a few rounds of exchanging comments and discussions until
- the team is ready to accept your changes.
+With some, the comment might not make total sense. You can reply to comments for clarification.
-Perform the above actions on the pull request you received,
-this time acting as the moderator/code reviewer.
+
-#### Step 6: Closing a Pull Request
+However, if you disagree, or are really lost on what they are driving it, it will be best to
+talk to them in person. Discussions done on code reviews can often feel quite adversarial -
+discussing what the best solution is in person can often defuse this.
-1. Once the moderator approves your changes, either one of you can merge onto the base branch.
+> ## Exercise: responding and addressing comments
+>
+> Look at the PR that you created on your repo, that should now have someone elses comments
+> on it.
+> For each comment, either reply explaining why you don't think the change is necessary
+> or make the change and push a commit fixing it. You can reply to the comment indicating you
+> have done it.
+>
+> At the same time, people will be addressing your comments. If you're happy that your
+> comment has been suitably addressed, you can mark it as resolved.
+> Once you're happy they have all been addressed, you can approve the PR.
+> To approve a PR, submit a new review and this time select `Approve`.
+> This tells the author you are happy for them to merge the pull request.
+{: .challenge}
+
+## Approve a pull request
+
+1. Once the reviewer approves the changes, the person whose repository it is can
+ merge onto the base branch.
Typically, it is the responsibility of the code's author to do the merge
but this may differ from team to team.
+ In our case, you will merge the changes on the PR on your repository.
{: .image-with-shadow width="900px"}
2. Delete the merged branch to reduce the clutter in the repository.
-Repeat the above actions for the pull request you received.
+## Making code easy to review
+
+There are a few things you can do when raising a pull request to make it
+as easy as possible for the reviewer to review your code:
+
+* Keep the changes **small**.
+* Keep each commit as **one logical change**.
+* Provide a **clear description** of the change.
+* **Review your code yourself**, before requesting a review.
+
+The most important thing to keep in mind is how long your pull request is.
+Smaller changes, that just make one small improvement, will be much quicker and easier to
+review.
+There is no golden rule, but [studies into code review](https://smartbear.com/resources/ebooks/the-state-of-code-review-2020-report/) show that you should not review more
+than 400 lines of code at a time, so this is a reasonable target to aim for.
+You can refer to some [studies](https://jserd.springeropen.com/articles/10.1186/s40411-018-0058-0)
+and [Google recommendations](https://google.github.io/eng-practices/review/developer/small-cls.html)
+as to what a "large pull request" is but be aware that it is not an exact science.
+
+Even within a single review, try to keep each commit to be making one logical change.
+This can help if your review would otherwise be too large.
+In particular, if you've reformatted, refactored and changed the behavior of the
+code make sure each of these is in a separate commit
+(i.e reformat the code, commit, refactor the code, commit, alter the behavior of the code, commit).
+
+Make sure you write a clear description of the content and purpose of the change.
+This should be provided as the pull request description.
+This should provide the context needed to read the code.
+
+It is also a good idea to review your code yourself,
+before requesting a review.
+In doing this you will spot the more obvious issues with your code,
+allowing your reviewer to focus on the things you cannot spot.
+
+## Empathy in review comments
+
+Code is written by humans (mostly!), and code review is a form of communication.
+As such, empathy is important for effective reviewing.
+
+When reviewing code, it can be sometimes frustrating when code is confusing, particularly
+as it will be implemented differently to how you would have done it.
+However, it is important as a reviewer to be compassionate to the
+person whose code you are reviewing.
+Specifically:
+
+* Identify positives in code as and when you find them (particularly if it is an improvement on
+ something you've previously fed back on in a previous review).
+* Remember different doesn't mean better - only request changes if the code is wrong or hard to understand.
+* Only provide a few non-critical suggestions - you are aiming for better rather than perfect.
+* Ask questions to understand why something has been done a certain way rather than assuming you
+ know a better way.
+* If a conversation is taking place on a review and hasn't been resolved by a
+ single back-and-forth exchange, then schedule a conversation to discuss instead
+ (recording the results of the discussion in the PR).
+
+## Designing a review process
+
+To be effective, code review needs to be a process that is followed by everyone
+developing the code.
+Everyone should believe that the process provides value.
+
+One way to foster this is to design the process as a team.
+When you're doing this you should consider:
+
+* Do all changes need to go through code review
+* What technologies will you use to manage the review process
+* How quickly do you expect someone to review the code once you've raised a PR?
+* How long should be spent reviewing code?
+* What kind of issues are (and aren't) appropriate to raise in a PR?
+* How will someone know when they are expected to take action (e.g. review a PR).
+
+You could also consider using pull request states in GitHub:
+ - Open a pull request in a `DRAFT` state to show progress or request early feedback;
+ - `READY FOR REVIEW` when you are ready for feedback
+ - `CHANGES REQUESTED` to let the author know
+ they need to fix the requested changes or discuss more;
+ - `APPROVED` to let the author they can merge their pull request.
-If the work on the feature branch is completed and it is sufficiently tested,
-the feature branch can now be merged into the `develop` branch.
+Once you've introduced a review process, you should monitor (either formally or
+informally) how well it is working.
-## Best Practice for Code Review
-
-There are multiple perspectives to a code review process -
-from general practices to technical details relating to different roles involved in the process.
-It is critical for the code's quality, stability and maintainability
-that the team decides on this process and sticks to it.
-Here are some examples of best practices for you to consider
-(also check these useful code review blogs from [
-Swarmia](https://www.swarmia.com/blog/a-complete-guide-to-code-reviews/?utm_term=code%20review&utm_campaign=Code+review+best+practices&utm_source=adwords&utm_medium=ppc&hsa_acc=6644081770&hsa_cam=14940336179&hsa_grp=131344939434&hsa_ad=552679672005&hsa_src=g&hsa_tgt=kwd-17740433&hsa_kw=code%20review&hsa_mt=b&hsa_net=adwords&hsa_ver=3&gclid=Cj0KCQiAw9qOBhC-ARIsAG-rdn7_nhMMyE7aeSzosRRqZ52vafBOyMrpL4Ypru0PHWK4Rl8QLIhkeA0aAsxqEALw_wcB)
-and [Smartbear](https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/)):
-
-1. Decide the focus of your code review process, e.g., consider some of the following:
- - code design and functionality -
- does the code fit in the overall design and does it do what was intended?
- - code understandability and complexity -
- is the code readable and would another developer be able to understand it?
- - tests - does the code have automated tests?
- - naming - are names used for variables and functions descriptive,
- do they follow naming conventions?
- - comments and documentation -
- are there clear and useful comments that explain complex designs well
- and focus on the "why/because" rather than the "what/how"?
-2. Do not review code too quickly and do not review for too long in one sitting.
- According to
- [“Best Kept Secrets of Peer Code Review” (Cohen, 2006)](https://www.amazon.co.uk/Best-Kept-Secrets-Peer-Review/dp/1599160676) -
- the first hour of review matters the most as
- detection of defects significantly drops after this period.
- [Studies into code review](https://smartbear.com/resources/ebooks/the-state-of-code-review-2020-report/)
- also show that you should not review more than 400 lines of code at a time.
- Conducting more frequent shorter reviews seems to be more effective.
-3. Decide on the level of depth for code reviews
- to maintain the balance between the creation time and time spent reviewing code -
- e.g. reserve them for critical portions of code and avoid nit-picking on small details.
- Try using automated checks and linters when possible,
- e.g. for consistent usage of certain terminology across the code and code styles.
-4. Communicate clearly and effectively -
- when reviewing code, be explicit about the action you request from the author.
-5. Foster a positive feedback culture:
- - give feedback about the code, not about the author
- - accept that there are multiple correct solutions to a problem
- - sandwich criticism with positive comments and praise
-7. Utilise multiple code review techniques -
- use email,
- pair programming,
- over-the-shoulder,
- team discussions and
- tool-assisted or
- any combination that works for your team.
- However, for the most effective and efficient code reviews,
- tool-assisted process is recommended.
-9. From a more technical perspective:
- - use a feature branch for pull requests as you can push follow-up commits
- if you need to update your proposed changes
- - avoid large pull requests as they are more difficult to review.
- You can refer to some [studies](https://jserd.springeropen.com/articles/10.1186/s40411-018-0058-0)
- and [Google recommendations](https://google.github.io/eng-practices/review/developer/small-cls.html)
- as to what a "large pull request" is but be aware that it is not exact science.
- - don't force push to a pull request as it changes the repository history
- and can corrupt your pull request for other collaborators
- - use pull request states in GitHub effectively (based on your team's code review process) -
- e.g. in GitHub you can open a pull request in a `DRAFT` state
- to show progress or request early feedback;
- `READY FOR REVIEW` when you are ready for feedback;
- `CHANGES REQUESTED` to let the author know
- they need to fix the requested changes or discuss more;
- `APPROVED` to let the author they can merge their pull request.
+It is important that reviews are processed quickly, to avoid costly context switching.
+We recommend aiming for 3 hours to get a first review, with the PR being merged the same
+day in most cases. If you are regularly missing these targets, then you should review
+where things are getting stuck and work out what you can do to move things along.
> ## Exercise: Code Review in Your Own Working Environment
>
-> At the start of this episode we briefly looked at a number of techniques for doing code review,
-> and as an example,
-> went on to see how we can use GitHub Pull Requests to review team member code changes.
-> Finally, we also looked at some best practices for doing code reviews in general.
+> In this episode we have looked at why and how to use a tool driven code review process
+> using GitHub pull requests. We've also looked at some best practices for doing
+> code reviews in general.
>
-> Now think about how you typically develop code,
-> and how you might institute code review practices within your own working environment.
-> Write down briefly for your own reference (perhaps using bullet points)
-> some answers to the following questions:
+> Now think about how you typically develop code.
+> What benefits do you think you would see for introducing a code review process in
+> your work environment.
+> How you might institute code review practices within your environment.
+> Write down a process for a tool assisted code review, answering the questions
+> above.
>
-> - Which 2 or 3 key circumstances would code review be most useful for you and your colleagues?
-> - Referring to the first section of this episode above,
-> which type of code review would be most useful for each circumstance
-> (and would work best within your own working environment)?
-> - Taking one of these circumstances where code review would be most beneficial,
-> how would you organise such a code review, e.g.:
-> - Which aspects of the codebase would be the most useful to cover?
-> - How often would you do them?
-> - How long would the activity take?
-> - Who would ideally be involved?
-> - Any particular practices you would use?
+> Once complete, discuss with the rest of the class what are the advantages of
+> a code review process and
+> what challenges you think you'd face in implementing this process in your own working environment.
+> > ## Solution
+> > The purposes of code review include:
+> > - improving internal code readability, understandability, quality and maintainability
+> > - checking for coding standards compliance, code uniformity and consistency
+> > - checking for test coverage and detecting bugs and code defects early
+> > - detecting performance problems and identifying code optimisation points
+> > - finding alternative/better solutions.
+> > - sharing knowledge of the code, and of coding standards and expectations of quality
+> >
+> > Finally, it helps increase the sense of collective code ownership and responsibility,
+> > which in turn helps increase the "bus factor"
+> > and reduce the risk resulting from information and capabilities
+> > being held by a single person "responsible" for a certain part of the codebase
+> > and not being shared among team members.
+> >
+> > Challenges you might face introducing a code review process:
+> > - Complaints that it is a waste of time
+> > - Creating a negative atmosphere where people are overly critical of each others work, or are defensive of their own
+> > - Perfectionism leading to slower development
+> > - People not sharing code to avoid the review process
+> >
+> > Make sure to monitor whether these are happening, and adjust the process accordingly.
+> {: .solution}
{: .challenge}
+## Other reading
+
+There are multiple perspectives to a code review process -
+from general practices to technical details relating to different roles involved in the process.
+We have discussed the main points, but do check these useful code review blogs from [
+Swarmia](https://www.swarmia.com/blog/a-complete-guide-to-code-reviews/?utm_term=code%20review&utm_campaign=Code+review+best+practices&utm_source=adwords&utm_medium=ppc&hsa_acc=6644081770&hsa_cam=14940336179&hsa_grp=131344939434&hsa_ad=552679672005&hsa_src=g&hsa_tgt=kwd-17740433&hsa_kw=code%20review&hsa_mt=b&hsa_net=adwords&hsa_ver=3&gclid=Cj0KCQiAw9qOBhC-ARIsAG-rdn7_nhMMyE7aeSzosRRqZ52vafBOyMrpL4Ypru0PHWK4Rl8QLIhkeA0aAsxqEALw_wcB)
+and [Smartbear](https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/).
+
+The key thing is to try it, and iterate the process until it works well for your team.
+
{% include links.md %}
diff --git a/_layouts/episode_mermaid.html b/_layouts/episode_mermaid.html
new file mode 100644
index 000000000..b5ffe5179
--- /dev/null
+++ b/_layouts/episode_mermaid.html
@@ -0,0 +1,22 @@
+---
+layout: episode
+---
+{{ content }}
+
+
+
+
diff --git a/fig/exercise-feature-branch.svg b/fig/exercise-feature-branch.svg
deleted file mode 100644
index 569ecadd3..000000000
--- a/fig/exercise-feature-branch.svg
+++ /dev/null
@@ -1,433 +0,0 @@
-
-
diff --git a/fig/github-add-emoji.png b/fig/github-add-emoji.png
new file mode 100644
index 000000000..34a5062e6
Binary files /dev/null and b/fig/github-add-emoji.png differ
diff --git a/fig/github-create-pull-request.png b/fig/github-create-pull-request.png
index 02cf249c7..e66c222f9 100644
Binary files a/fig/github-create-pull-request.png and b/fig/github-create-pull-request.png differ
diff --git a/fig/github-merge-pull-request.png b/fig/github-merge-pull-request.png
index a31a85a96..63abd02d1 100644
Binary files a/fig/github-merge-pull-request.png and b/fig/github-merge-pull-request.png differ
diff --git a/fig/github-pull-request-add-comment.png b/fig/github-pull-request-add-comment.png
new file mode 100644
index 000000000..90f30710c
Binary files /dev/null and b/fig/github-pull-request-add-comment.png differ
diff --git a/fig/github-pull-request-add-suggestion.png b/fig/github-pull-request-add-suggestion.png
new file mode 100644
index 000000000..9d872b4f5
Binary files /dev/null and b/fig/github-pull-request-add-suggestion.png differ
diff --git a/fig/github-pull-request-files-changed.png b/fig/github-pull-request-files-changed.png
new file mode 100644
index 000000000..c520fbddd
Binary files /dev/null and b/fig/github-pull-request-files-changed.png differ
diff --git a/fig/github-pull-request-tab.png b/fig/github-pull-request-tab.png
new file mode 100644
index 000000000..046eeae51
Binary files /dev/null and b/fig/github-pull-request-tab.png differ
diff --git a/fig/github-respond-to-review-comment.png b/fig/github-respond-to-review-comment.png
new file mode 100644
index 000000000..726f3951c
Binary files /dev/null and b/fig/github-respond-to-review-comment.png differ
diff --git a/fig/github-submit-pr-review.png b/fig/github-submit-pr-review.png
new file mode 100644
index 000000000..ebf5ada72
Binary files /dev/null and b/fig/github-submit-pr-review.png differ
diff --git a/fig/github-submit-pullrequest.png b/fig/github-submit-pullrequest.png
new file mode 100644
index 000000000..9d031b070
Binary files /dev/null and b/fig/github-submit-pullrequest.png differ