Development Progress and Code Review

A team reviews its own work as it goes, which is what a pull request is for (Chapter 62): every change is read by someone who did not write it before it joins the main branch. The guide's review is the same thing from outside, and in the worked project it happened on Thursday 1 October, between the second increment and the testing.

A checklist to review against

Use the same list every time, so that nothing depends on what the reviewer happens to notice:

Reviewing against the document, mechanically

Chapter 36's architecture document made one rule for the backend: each layer may use only the layers below it. A rule like that is worth checking by machine rather than by eye, because it is exactly the sort of thing that drifts one require at a time. The worked team wrote a short script, which reads every file's own require lines and reports what the code does, not what anybody remembers:

'use strict';

// npm run layers
// Checks the one rule the architecture document gives the
// backend (section 5.1): each layer may use only the layers
// below it. It reads every file's own require() lines, so it
// reports what the code does, not what anybody remembers.
//
// The one allowed exception is written down here, as it is in
// the document: the health check asks the database pool
// directly, because testing that connection is its whole job.

const fs = require('node:fs');
const path = require('node:path');

const SRC = path.join(__dirname, '..', 'src');

// Which layer each file belongs to, by where it lives.
const LAYER_OF = [
  ['routes/', 'routes'],
  ['middleware/', 'middleware'],
  ['services/', 'services'],
  ['store/', 'store'],
  ['validate.js', 'validation'],
  ['rules.js', 'rules'],
  ['db.js', 'db'],
  ['errors.js', 'errors'],
  ['passwords.js', 'shared'],
  ['attempts.js', 'shared'],
  ['config.js', 'shared'],
  ['app.js', 'wiring'],
  ['server.js', 'wiring'],
];

// What each layer may require. `errors` and `shared` are
// small leaves every layer may use.
const MAY_USE = {
  wiring: ['routes', 'middleware', 'services', 'store', 'rules',
    'validation', 'db', 'errors', 'shared', 'express'],
  routes: ['services', 'validation', 'middleware', 'errors',
    'shared', 'express'],
  middleware: ['services', 'errors', 'shared', 'express'],
  validation: ['rules', 'errors'],
  services: ['store', 'rules', 'db', 'errors', 'shared'],
  rules: [],
  store: ['errors'],
  db: ['shared', 'mysql2/promise'],
  errors: [],
  shared: ['errors'],
};

// The exception the architecture document records.
const EXCEPTIONS = [
  ['routes/health.js', 'db',
    'the health check asks the pool whether the database answers'],
];

// What must not appear in a layer at all, whatever it
// requires: a service that touches a request can never be
// tested without a server, and a rule that reads the clock
// cannot be tested at any other time of day. SQL is looked
// for as a statement, not as a word: `update` and `insert`
// are ordinary method names.
const SQL = [/SELECT\s+[^;]*\bFROM\b/i, /INSERT\s+INTO\b/i,
  /UPDATE\s+\w+\s+SET\b/i, /DELETE\s+FROM\b/i];
const FORBIDDEN = {
  services: [/\breq\b/, /\bres\b/, ...SQL],
  rules: [/\breq\b/, /\bres\b/, /new Date\(\)/, ...SQL],
  store: [/\breq\b/, /\bres\b/],
  validation: [/\breq\b/, /\bres\b/, ...SQL],
};

function walk(dir) {
  return fs.readdirSync(dir, { withFileTypes: true }).flatMap((e) => {
    const full = path.join(dir, e.name);
    return e.isDirectory() ? walk(full)
      : (e.name.endsWith('.js') ? [full] : []);
  });
}

function layerOf(relative) {
  const found = LAYER_OF.find(([start]) => relative.startsWith(start));
  return found ? found[1] : 'unknown';
}

// The layer a require() names, or null for a node: builtin.
function required(spec, fromRelative) {
  if (spec.startsWith('node:')) return null;
  if (!spec.startsWith('.')) return spec; // express, mysql2
  const target = path.normalize(
    path.join(path.dirname(fromRelative), spec));
  // A require names a file or a folder: './errors' is
  // errors.js, './store' is store/index.js.
  const tried = [target, `${target}.js`, `${target}/`];
  const found = tried.map(layerOf).find((l) => l !== 'unknown');
  return found || 'unknown';
}

// Everything wrong with one file's text.
function faults(relative, layer, text) {
  const found = [];
  const allowed = MAY_USE[layer] || [];
  for (const m of text.matchAll(/require\('([^']+)'\)/g)) {
    const used = required(m[1], relative);
    if (used === null || used === layer) continue;
    const excused = EXCEPTIONS.some(
      ([file, target]) => file === relative && target === used);
    if (excused || allowed.includes(used)) continue;
    found.push(`${relative} (${layer}) requires ${used}`);
  }
  // Comments explain the rules; only real code breaks them.
  const code = text.replace(/\/\/[^\n]*/g, '');
  for (const bad of FORBIDDEN[layer] || []) {
    if (bad.test(code)) {
      found.push(`${relative} (${layer}) contains ${bad}`);
    }
  }
  return found;
}

function check() {
  const problems = [];
  let files = 0;
  let lines = 0;
  for (const full of walk(SRC)) {
    const relative = path.relative(SRC, full);
    const text = fs.readFileSync(full, 'utf8');
    files += 1;
    lines += text.split('\n').length;
    problems.push(...faults(relative, layerOf(relative), text));
  }
  return { problems, files, lines };
}

// A check that has never failed has not been tested. Five
// breaches are planted in COPIES of the text (nothing on disk
// is touched) and every one must be reported.
function selfTest() {
  const planted = [
    ['routes/orders.js', 'routes', "const s = require('../store');"],
    ['services/menu.js', 'services',
      "const q = 'SELECT name FROM menu_items';"],
    ['rules.js', 'rules', 'const now = new Date();'],
    ['services/orders.js', 'services',
      'function handler(req, res) { return res; }'],
    ['store/menu.js', 'store', "const e = require('express');"],
  ];
  for (const [file, layer, line] of planted) {
    const found = faults(file, layer, `'use strict';\n${line}\n`);
    if (found.length === 0) {
      throw new Error(`check-layers misses: ${line}`);
    }
  }
}

selfTest();
const { problems, files, lines } = check();
for (const p of problems) console.error('LAYER: ' + p);
if (problems.length > 0) {
  console.error(`${problems.length} break(s) of the layer rule.`);
  process.exitCode = 1;
} else {
  console.info(`layers: ${files} files, ${lines} lines, every `
    + 'require() allowed by the architecture document, with its '
    + 'one recorded exception.');
}

Two things about it are worth copying. It carries the one exception the architecture document records, the health check asking the pool directly, so the exception is in the code as well as the prose. And it plants five breaches of the rule in copies of the text on every run, and refuses to report anything if it misses one: a check that has never failed has not been tested.

$ cd ~/canteen-preorder
$ npm run layers

> canteen-preorder@1.0.0 layers
> node scripts/check-layers.js

layers: 27 files, 1698 lines, every require() allowed by the architecture document, with its one recorded exception.

The worked review

Thursday 1 October, forty minutes in the lab, Prof. Iyer with all four members and the code on the screen.

Before it, the team did three things, and a team that does them turns a review into a conversation about design rather than a hunt for obvious faults:

• ran the whole suite and the layer check, so nothing failing was on the screen;
• closed the issues that were done, so the board showed the truth;
• wrote down the two questions they wanted answered.

What the guide looked at, and what he asked:

The action from the design review, shown. Chapter 37 recorded that Farhan, with Rohan, would show the concurrency test passing at this review. They ran it:

$ cd ~/canteen-preorder
$ node --test --test-reporter=./test/reporter.js \
>   test/integration/concurrency.test.js
test/integration/concurrency.test.js
  pass  sells the last 5 plates to exactly 5 of 20 students
  pass  accepts only one of two orders one student sends at once

2 tests: 2 passed, 0 failed

Twenty students, five plates, exactly five accepted: NFR-2, which no amount of clicking could have shown.

What the review found. Three things, none of them a disaster, which is what a healthy review looks like:

1. A comment that had become false. mock.js said it answered "every request the pages make"; it answers the student's six (Chapter 40). Corrected the same day.
2. A message with numbers in it. A sold-out refusal read "Only 2 Chicken Biryani left. 3 2", because the page printed every detail of the error, including the item's id and the stock. Found by Aditi while showing the counter's screen. It became an issue, and Chapter 56 follows it from report to fix.
3. A name that misled. move in the order service does both cancelling and the counter's changes; the guide read it as "move to the next status". It kept its name, with a comment saying what it covers, and both callers were listed above it.

What the guide said about the history, which is the part worth repeating: he could see, without asking, that the frontend and the backend had been built side by side from 11 September, because the commits alternate between public/ and src/ through those two weeks.

Do this for your project

1. Commit on the day you write the code, with a message that says what changed and why.
2. Open an issue for every piece of work, and close it when it is done.
3. Have every change read by someone who did not write it, before it joins the main branch.
4. Review against a checklist, not against your memory.
5. Make the rules of your design checkable by a script where you can, and run it before every review.
6. Before the guide's review: run everything, close what is done, and bring your own questions.
7. Write down what the review found, and turn each finding into an issue.

Mistakes that cost marks

One commit, on the last night, called "project".

A repository where only one member ever committed, in a project of four.

Reviewing by "looks fine to me", which finds nothing and teaches nobody.

Defending the code in a review instead of listening: the reviewer is the first reader, and if they misread it, so will the examiner.

Findings that go nowhere, because nobody wrote them down.

A design rule nothing checks, which drifts one require at a time until the layers are a memory.

Quick revision

• The component: Development Progress & Code Review, 5 marks, the guide's; MU prints no criteria.
• Progress is shown over time: commits by everyone across the weeks, issues closed, something running at every meeting, tests that grow.
• A code review: someone else reads your code and asks about it; a checklist every time.
• Make design rules checkable: the layer rule is a script that also plants breaches to prove it works.
• Before the guide's review: everything green, the board true, your own questions ready.
• Every finding becomes an issue.

Questions you must be able to answer

1. What does the "Development Progress" half of this component reward, and how is it shown? Work done over the semester rather than at the end, shown by a version control history with commits from every member spread across the weeks, issues opened and closed, tests that grow with the code, and something that runs at every meeting with the guide.

2. What is a code review, and what should it look for? One or more people other than the author reading the code and asking about it, against a fixed checklist: is it correct, including the awkward cases; is it tested; is it in the right layer; is it clear and consistently named; is it safe; is it one change; and are its comments true.

3. How can a design rule be enforced rather than merely written down? By making it checkable. The layer rule, that each layer may use only the layers below it, is checked by a script that reads every require in the source and reports any that the architecture document does not allow, with its one recorded exception.

4. Why does that script plant faults in its own input? Because a check that has never failed has not been tested: if a mistake in the script made it report nothing, a passing run would look exactly the same as a correct system. Planting breaches it must catch proves it can still fail.

5. What should a team do before the guide's code review? Run the whole test suite and any design checks, so nothing failing is on the screen; close the issues that are finished, so the board shows the truth; and prepare the questions they want the guide's opinion on.

6. The review found a comment that was no longer true. Why does that matter as much as a bug? Because the next person to read the code will believe it, and act on it. A false comment is a bug that has been written down and signed.