# Reviewing code Review by the principles in [principles.md](principles.md). The aim is code that is simple and reliable, not code that follows modern conventions. ## How to judge - Look for simplifications first. A simpler design often fixes bugs as a side effect: four bugs in a drag-and-drop library disappeared when it was rewritten with pointer events. Ask how you would write it from scratch, then compare. - Raise only problems that could actually happen. Theoretical issues, like a global name collision on a page with two scripts, are noise. Few people use most reactionary software, so don't guard against hypothetical users. - Show a problem happening if you can: run the failing request, take the screenshot. Test a claim in a browser before making it; a proposed fix (setting `<meta charset>` from JavaScript) turned out to do nothing, because the page was already decoded. - Rank findings honestly. Something that makes a failure silent or hard to trace is serious even if small; a theoretical issue doesn't go next to real bugs. - Never call dead or senseless code "harmless". It costs the computer nothing, but every reader has to work out that it means nothing. "Code that makes no sense should not be tolerated." Report it as a problem, not as an optional cleanup. - A layout that would break visibly if someone changed it later, such as a margin that collapses through a wrapper div, isn't a subtle error, so don't raise it. Anyone would see the break and find the cause in minutes. Subtle errors are the ones nobody sees. - When a behavior could be intended, ask rather than report it as a bug. - When the code uses its own solution instead of a standard one, judge whether it works, not whether it is standard. Say precisely where it is weak, if it is. - Before saying that a response lacks a header such as `Content-Type`, check the handlers that wrap it (see [websites.md](websites.md)). - Before removing or renaming a library's functions, ask: other projects may use them. - A bare `or error()` is not a problem: the log shows the stack trace with the file and line, so the failed check is easy to find. Don't add messages to them. ## Checklist Each of these was found in real reactionary code. - Lines copied from another function by mistake. A function ended with a line from another function that used an undefined variable. It threw every time, but users didn't notice, because the page had already navigated away; it showed only in the server log. - Unused function parameters, and callers that still pass them. - Debug `console.log` lines, commented out or live. - Unused local variables. Exception: `local error = Luan.error` and `local logger = Logging.logger "X"` at the top of a Luan file are standard boilerplate, kept even when unused. Don't report them. - Commented-out lines, which can leave a function doing nothing of its own. - Conditions that are always true at that point, like `first &&` after `!first` has been ruled out. - Helpers that redo what another helper does, such as a `to_set(obj)` that is just `list_to_set(to_list(obj))`. - A missing `'use strict'` in a JavaScript file or a nontrivial inline script. Report that as the finding, not the sloppy code it allowed. - Old DOM idioms: `el.outerHTML = ''` for `el.remove()`, `event.keyCode===13` for `event.key==='Enter'`, mouse and touch events for pointer events. - Images with no size, which shift the layout when they load. - A dialog whose first focusable element is a link, so `showModal()` focuses it. - CSS rules that set a default value, and `display: none` rules that the `hidden` attribute can replace. - An attribute selector like `[content]` that also matches `<meta content>`. - Values from the request written into HTML or JavaScript. Write values from the database instead. - A handler that takes an id without checking that the current user may use that thing. - A handler that changes the database without a Referer check, including ones used before login, like registering. - A change to the database outside a transaction, deletes included. - `x.reload()` without using the result, and objects changed inside a transaction that were looked up outside it. - Code that maps a request path to a file without being tested against `..`. ## Finding problems no one reports - Click every button and open every dialog in a headless browser, while watching for page errors and for `error_log.js` entries in the server log. - Check access control with a third user who isn't part of the data being protected: try every handler that takes an id with that user's cookies, and with none. - Find dead CSS by checking every selector against the page in each of its states (see [testing.md](testing.md)).