JS1 Module Project: Code review

[illicitonion]

Which module(s) and week(s) does this change affect? Module(s): JS1

Goals

1. Drive insights around the idea that different people have different perspectives on code (e.g. author vs reader).
2. Get trainees used to both giving and getting feedback.
3. Get trainees thinking about how to usefully explain/suggest ideas/improvements.
4. Introduce trainees to the idea of code review.
5. Get some experience with the GitHub UI.

Idea

Each week, pair up trainees randomly, and give them each two exercises. For each exercise, one trainee will be the reviewer, and one the reviewee. Each trainee should do both roles for the two exercises in the week.

Both will be given some code which has problems in it.

The reviewer will additionally be given a list of problems, with explanations of what's wrong, and examples of better code.

The goal of the reviewer is to make comments on a pull request to guide the reviewee to improve the code. We expect this to take place over multiple iterations over a week, as part of coursework.

Ideally the material being reviewed should be material the trainees are fairly comfortable with, and reinforce, but should not be pushing their knowledge too far - we want the trainees to be able to mostly focus on the code review aspects, and not get bogged down in one or both of their pair not well understanding the code itself.

Rules of the game

We need rules for the reviewer, to avoid them just giving replacement code. Something along the lines of:

1. Every comment should contain a question pointing out a problem, not a solution.
2. No code is allowed in comments.
3. Links to reference material is strongly encouraged.

Suggested subject matter for each week

Week 1: HTML/CSS (e.g. adding/improving aria labels, factoring out common CSS into common rules, removing redundant CSS) Week 2: Variable / parameter naming, duplicate code Week 3: Loops, boundary conditions, more naming Week 4: Choosing between for loops/forEach/map, pulling out chains.

Throughout, but particularly in week 4, we should make clear how to escalate for more help if the trainees find themselves out of their depth, probably by @-ing a mentor to help out. The same rules of the game should apply to mentors as to trainees.

Open questions

• What's our measurable outcome of this work? Possibly something around a a portfolio of useful comments left, and something around the nature of the comments (e.g. STAR).

Who might need to know about this change?

@CodeYourFuture/syllabus-team - I'd love to talk this through in our meeting this week, I'll try to put together some example code before then.

[SallyMcGrath]

Notes from further discussions:

• provide 10 pre-set comments (cards they can play) instead of questions

[40thieves]

I like the general idea! 👍🏻 A couple of thoughts:

1. Definitely prefer the idea of "playing cards" over simply banning code in comments, which I think could be taken in quite the wrong way
2. The subject matter you've suggested seems reasonable to me, but let's not get too hung up on this as one of the goals for this year is to review/change the JS content?
3. Re: your suggestion of measurable outcomes, would this be a portfolio for employers? To me it seems like this would be difficult to quickly understand for hiring managers vs some "more realistic" code review
4. A possible suggestion for measurable outcomes: we could use this as a base for enforcing peer review of coursework PRs?
5. It seems like it would fit well with (other) Daniel's Learning Lab concept?

[illicitonion]

From discussion on slack, some action items:

• [ ] Add a "How to use the code review interface" cheat-sheet, and link to the CYF style guide too
• [ ] Let's frame it more as a collaborative game (or otherwise give some background as to "why")
• [x] How to know when to stop? Probably a time limit? Or a number of review rounds? Maybe "Stop after 3 back and forths, and if you think you're done before them, escalate to your education buddy". And we should probably expectation set to "Expect to spend 2-3 hours on your pair of exercises in the week".
• [x] s/reviewee/author/
• [x] We should devise some protocol for "Who's the review blocked on". Probably introduce setting the Assignee on the PR, to make clear when handover has happened.
• [x] On reviewer page, move "Links are great" from rules to recommendations
• [ ] Would be interesting to A/B test including and not-including the "Better code" section for the reviewer.

[40thieves]

For future reference, here's the draft we're working against: https://github.com/illicitonion/JS1-Code-Review-Draft

[40thieves]

Aiming to have this done by LDN9 JS1W4 (prob Jan?)