---
title: "Function audit"
description: "Checks one file or function: names, errors, concurrency, and security at the call level."
canonical: "https://goodcodereview.com/function"
updated: "2026-10-25"
---
# function
## fn-aria-role-has-state-and-keys
severity: should-fix
concerns: accessibility
applies_when: A custom widget sets `role` or an `aria-*` attribute because no native element fits.
**Rule.** When a native element cannot do the job, give the custom widget the state attribute and the keys its role promises.
**Why.** A role only tells assistive technology what the element is. It adds no keyboard behaviour, and it says nothing about state. A `role="switch"` with no `aria-checked` is announced as a switch of unknown state. Wrong ARIA is worse than none, because it gives a screen-reader user a false picture. The widget also needs an accessible name, from its text or a label, or it is announced by role alone.
**FAIL**
```jsx
Dark mode
// no aria-checked: announced as a switch of unknown state
```
**PASS**
```jsx
Dark mode
// a native already takes Tab, Enter, and Space
```
**Counterpoint.** "ARIA patterns are in the guide, so we copy them." Copy the keyboard table as well as the attributes. For a plain toggle, a `button` with `aria-pressed` is enough, and the label should stay the same when the state changes.
## fn-colour-not-sole-cue
severity: should-fix
concerns: accessibility, usability
applies_when: Styles or markup show an error, a required field, a status, or a link using colour.
**Rule.** Pair every colour that carries meaning with a second cue: text, an icon, a shape, or an underline.
**Why.** Colour-blind users, glare, greyscale mode, and a screen reader all remove colour. A red border on a bad field, or a link that differs from body text only by hue, tells those users nothing.
**FAIL**
```css
.field.error { border-color: red; }
.status-ok { color: green; }
a { color: #0a58ca; text-decoration: none; } /* no underline, no hover or focus cue */
```
**PASS**
```html
… Error: enter a date such as 27/03/2026
terms
```
**Counterpoint.** "Colour is the quickest cue, and the icon clutters the design." Colour can stay as the fast cue. The rule asks for a second one, not for removing the first. A text prefix such as "Error:" costs a few characters. A link in body text can also pass with 3:1 contrast against the text around it plus a non-colour cue on hover and focus. The underline is the simpler fix.
## fn-comments-explain-why
severity: should-fix
concerns: maintainability
applies_when: A comment sits next to code, or the code needs one to be understood.
**Rule.** Write comments that give a reason, a constraint, or a trade-off; rename before commenting, and never restate what the next line does.
**Why.** A restating comment rots silently and adds nothing. A missing reason makes the next person delete the code that mattered.
**FAIL**
```text
counter += 1 # increment the counter
retry = False # see the PR, currently disabled
```
**PASS**
```text
# Business decision: activation and entitlement update in one transaction,
# so a user never has access without an active subscription.
# TODO: remove when billing emits entitlement events itself.
```
**Counterpoint.** "Good names make comments unnecessary." Names carry *what*. They cannot carry why a branch exists, a unit, or what callers may rely on, and a missing reason costs more than a slightly stale one. Rename first, then comment what the name cannot say. A stale comment is a defect either way. The test is whether the comment adds information that the code cannot carry.
## fn-confine-file-path
severity: block
concerns: security
applies_when: Code opens, reads, writes or deletes a file whose name or path includes request data.
**Rule.** Resolve the final path and check that it stays inside the intended base directory before using it.
**Why.** A name like `../../etc/passwd`, or its percent-encoded form, walks out of the upload folder. Stripping `../` fails against `....//` and double encoding. The reliable check works on the resolved path. Better still, map a user choice to an ID and build the path in code.
**FAIL**
```python
return send_file(os.path.join(UPLOADS, request.args["name"]))
```
**PASS**
```python
p = (UPLOADS / request.args["name"]).resolve()
if not p.is_relative_to(UPLOADS.resolve()): abort(400)
return send_file(p)
```
**Counterpoint.** "We already strip `..`." String stripping is a blocklist and gets bypassed. Resolve first, compare second. Resolving catches a symlink that already points outside, but a symlink swapped in between the check and the open still escapes. For trees that untrusted users can write to, use a rooted open such as Go's `os.OpenInRoot`, which refuses any escape at open time.
## fn-csprng-for-secrets
severity: block
concerns: security
applies_when: Code generates a token, session ID, password-reset code, API key, nonce or IV.
**Rule.** Draw every security-relevant random value from the platform CSPRNG.
**Why.** A general-purpose PRNG is predictable from a few outputs, so an attacker who sees two tokens can guess the next. UUIDv1 is time-based and also not secret. The API call is the whole fix, so it is cheap to demand.
**FAIL**
```python
token = "".join(random.choices(string.ascii_letters, k=32))
reset = str(int(time.time()))
```
**PASS**
```python
token = secrets.token_urlsafe(32) # os.urandom under the hood
# JavaScript: crypto.getRandomValues(new Uint8Array(32))
```
**Counterpoint.** "Math.random is fine for a short-lived code." Short-lived codes still get brute-forced, and predictable ones need no brute force. Use the CSPRNG for anything an attacker gains from guessing.
## fn-destructive-confirm-or-undo
severity: should-fix
concerns: usability
applies_when: A handler deletes user data, spends money, or sends something that cannot be recalled.
**Rule.** Make a destructive action reversible with undo, or ask the user to confirm it before it runs.
**Why.** People click the wrong thing. If the action cannot be reversed and nothing stops it, one slip deletes their work. Undo costs a successful user nothing, while a dialog on every click trains them to click through. A dialog works best when its buttons name the outcome, such as "Delete project" and "Cancel", because "OK" and "Are you sure?" say nothing about what happens next.
**FAIL**
```jsx
api.deleteProject(id)}>Delete
```
**PASS**
```jsx
async function remove() {
await api.softDelete(id);
toast("Project deleted", { action: { label: "Undo", onClick: () => api.restore(id) } });
}
// irreversible case: a dialog "Delete project “Q3 plan”?" with buttons "Delete project" and "Cancel"
```
**Counterpoint.** "Confirm everything to be safe." Frequent prompts are dismissed without reading, so they protect nothing. Use undo for common actions and reserve the dialog for rare, irreversible ones. Do not make Cancel the default button.
## fn-dialog-focus-managed
severity: block
concerns: accessibility, usability
applies_when: A component opens a modal dialog, drawer, or menu that covers the page.
**Rule.** When a modal opens, move focus into it and keep Tab inside it. Make Escape close it.
**Why.** Without it, a keyboard user keeps tabbing through the page behind the dialog, or gets stuck inside it with no exit. When it closes, return focus to the control that opened it. Focus that falls to the top of the page makes them start over.
**FAIL**
```jsx
{open &&
Delete? Delete }
```
**PASS**
```html
Delete project
Delete project?
Cancel Delete ` with `showModal()`, which makes the page inert and closes on Escape. A dialog built from generic elements must recreate each of those.
## fn-error-says-what-and-how
severity: should-fix
concerns: usability, accessibility
applies_when: Code builds the message shown when user input fails validation.
**Rule.** Show each validation error next to its field, in plain words that name what is wrong and how to fix it.
**Why.** "Invalid input" or an error code leaves the user to guess which field and what to change. A message at the top of a long form, or in a toast that fades, sends a magnifier user searching. Naming the fix is the shortest path to a valid form.
**FAIL**
```html
Error 422: invalid data
```
**PASS**
```html
Date of birth
Error:
Enter a date of birth in the past, such as 27 3 1986
`, with `for` and a matching `id` or by wrapping it. A placeholder is not a label.
**Why.** A screen reader announces a bare field as "edit text". Voice-control users cannot say its name. A label also enlarges the click target. A placeholder vanishes when the user types, so it fails every user who looks away and comes back.
**FAIL**
```html
Name
Email
Name
```
**Counterpoint.** "The design shows a floating placeholder label." A floating label is a real label if it stays visible and is wired to the input. Use `aria-label` only where the purpose is already visible, such as a search field next to a Search button.
## fn-heading-structure
severity: should-fix
concerns: accessibility, usability
applies_when: Markup or a component renders a title or a section heading.
**Rule.** Mark every heading with a real `` to `` element, not a styled `div`, `span`, or `p`.
**Why.** Screen-reader users jump from heading to heading to scan a page. A `div` styled as a heading is invisible to that. Pick the level from the outline, not from the size you want: a skipped level, such as `h2` to `h4`, makes the outline look as if a section is missing.
**FAIL**
```html
Account
Billing
Invoices
```
**PASS**
```html
Account
Billing
Invoices
```
**Counterpoint.** "The design needs a small `h2`." That is a CSS problem. Keep the level that matches the outline and set the size in the stylesheet. Jumping back up a level, such as `h4` to `h2`, is fine when a section ends.
## fn-img-alt-text
severity: should-fix
concerns: accessibility
applies_when: Markup or a component renders an image, an icon, or a chart.
**Rule.** Give every `` for an action and `` for navigation. Do not attach a click handler to a `div`, `span`, or `a` with no `href`.
**Why.** A `div` is not in the tab order, has no role, and ignores Enter and Space. A keyboard or screen-reader user cannot reach or operate it. A fake link also loses open-in-new-tab and copy-link. A native element gives all of that with no code.
**FAIL**
```jsx
Save
Menu
navigate("/billing")}>Billing
```
**PASS**
```jsx
Save
Menu
Billing
```
**Counterpoint.** "Native buttons are hard to style." They are not: reset `border`, `background`, and `font` on the element. Rebuilding the keyboard, role, and focus behaviour by hand costs more and fails more often. Where a custom widget is unavoidable, follow the WAI-ARIA Authoring Practices pattern for it.
## fn-no-blocking-call-in-async-code
severity: block
concerns: performance, correctness
applies_when: Code runs on an event loop or an async runtime (asyncio, Node.js, Tokio, ASP.NET async handlers).
**Rule.** Never make a blocking I/O call or run long CPU work directly in async code; use the async API or hand the work to a thread or worker.
**Why.** An event loop runs one callback at a time. A `time.sleep`, a synchronous HTTP call, or a big hash inside a handler stalls every other request on that loop, not just the slow one.
**FAIL**
```python
async def handler(req):
data = requests.get(url).json() # blocks the loop
digest = hashlib.pbkdf2_hmac("sha256", pw, salt, 600_000)
```
**PASS**
```python
async def handler(req):
data = (await client.get(url)).json() # httpx.AsyncClient
digest = await asyncio.to_thread(hashlib.pbkdf2_hmac, "sha256", pw, salt, 600_000)
```
**Counterpoint.** "This call is fast enough." A short read from a local SSD can be quicker synchronously. Measure it with asyncio debug mode or the runtime's equivalent. A call that crosses the network, or that waits on anything outside the process, is never in that class.
## fn-no-mass-assignment
severity: block
concerns: security
applies_when: A handler copies request fields into a model, ORM entity or settings object.
**Rule.** Bind request data to a DTO or an explicit field allowlist, never straight onto the model.
**Why.** When the model has `is_admin`, `balance` or `owner_id`, a client that adds those keys to the JSON body sets them. A blocklist fails the day someone adds a new sensitive column. The allowlist still needs the object-level check from [fn-object-level-authz](function.md#fn-object-level-authz).
**FAIL**
```text
user = User(**request.json) # body {"name": "a", "is_admin": true}
user.save()
```
**PASS**
```text
class UserIn(BaseModel): # only editable fields exist here
name: str
user = User(**UserIn(**request.json).model_dump())
```
**Counterpoint.** "A DTO per endpoint is boilerplate." Generate DTOs from the API schema. The boilerplate costs less than one privilege escalation.
## fn-no-native-deserialise-untrusted
severity: block
concerns: security
applies_when: Code turns bytes or text from outside the process into objects.
**Rule.** Parse untrusted input with a data-only format, never with a native object deserialiser.
**Why.** `pickle`, `ObjectInputStream`, `BinaryFormatter` and full-power YAML loaders can run attacker-chosen code while they load, so validating after the call is too late. JSON cannot instantiate arbitrary classes. Validate the parsed data against a schema afterwards, as the PASS does.
**FAIL**
```python
obj = pickle.loads(request.data)
cfg = yaml.load(body, Loader=yaml.Loader)
```
**PASS**
```python
obj = Model.model_validate_json(request.data)
cfg = yaml.safe_load(body)
```
**Counterpoint.** "The data comes from our own queue." Anyone who can write to the queue can run code on every consumer, so the queue is a trust boundary. Signing the payload, or a Java serialisation filter that allowlists classes, narrows the risk while you migrate. The call still fails this check until it reads a data-only format.
## fn-no-positive-tabindex
severity: should-fix
concerns: accessibility, usability
applies_when: Markup sets `tabindex`, or a layout reorders content with CSS.
**Rule.** Write the DOM in reading order and never set `tabindex` above 0.
**Why.** A positive `tabindex` jumps those elements to the front of the tab order, ahead of everything else on the page. Each new control then needs a new number, and the order stops matching what the user sees. CSS that moves items visually without moving them in the DOM does the same thing.
**FAIL**
```html
Save
```
**PASS**
```html
Save
```
**Counterpoint.** "The visual layout puts the sidebar first, but the main content comes first in the DOM." Reorder the DOM to match, or accept the difference only if the order still makes sense to a keyboard user. Do not patch it with numbers.
## fn-no-raw-html-sink
severity: block
concerns: security
applies_when: Code writes a string into a page, a template or a rendered model-output view.
**Rule.** Write untrusted text through an encoding sink (`textContent`, auto-escaped template), never through a raw HTML sink.
**Why.** One `innerHTML` of a username or of model output runs the attacker's script with the victim's session. Encoding must match the context: body, attribute, script string and URL each differ. Framework escape hatches such as `dangerouslySetInnerHTML` have the same effect.
**FAIL**
```text
el.innerHTML = comment.body;
```
**PASS**
```text
el.textContent = comment.body;
{reply}
// escaped by the framework
// user-authored HTML: DOMPurify.sanitize(html), nothing edited afterwards
```
**Counterpoint.** "We need to render rich text." Then sanitise with a maintained library such as DOMPurify at render time and add a CSP as a second layer. A CSP alone is not a fix, because it is defence in depth.
## fn-no-shell-with-input
severity: block
concerns: security
applies_when: Code starts a process and some part of the command comes from a request, a file or model output.
**Rule.** Start processes with the program and each argument as separate list items, and never through a shell string.
**Why.** A shell reads `;`, `|` and `$()` as commands, so a filename of `a; rm -rf ~` runs. Argument arrays skip the shell. The values still need validation, since an argument beginning with `-` becomes an option; end options with `--` where the tool allows it.
**FAIL**
```python
os.system("convert " + upload_name + " out.png")
subprocess.run(f"git log {branch}", shell=True)
```
**PASS**
```python
subprocess.run(["convert", upload_path, "out.png"], check=True) # upload_path is a server-generated name
subprocess.run(["git", "log", branch], check=True) # branch checked against an allowlist
```
**Counterpoint.** "We need shell features like pipes." Chain two `subprocess` calls in code, or use the library that does the job. If a shell is unavoidable, build the string only from constants.
## fn-object-level-authz
severity: block
concerns: security
applies_when: A handler loads, changes or deletes a record by an ID that comes from the request.
**Rule.** Check that the caller may act on the specific record before returning or changing it.
**Why.** A login proves who the caller is, not which records they own. Without a per-object check, changing `id=41` to `id=42` reads or edits another user's data (IDOR, also called BOLA). Random IDs only hide the problem.
**FAIL**
```text
order = db.get(Order, request.args["id"]) # any logged-in user, any order
return order.to_json()
```
**PASS**
```text
order = db.get(Order, request.args["id"])
if order is None or order.owner_id != user.id: raise NotFound() # same error for both
return order.to_json()
```
**Counterpoint.** "The IDs are UUIDs, nobody can guess them." A UUID leaks through logs, links and shared screenshots, and it is not an access check. Keep it as a second layer. Scope the query itself (`WHERE owner_id = :me`) so the check cannot be forgotten on the next endpoint.
## fn-password-slow-hash
severity: block
concerns: security
applies_when: Code stores or verifies a user password.
**Rule.** Hash passwords with Argon2id (or scrypt, bcrypt, PBKDF2) and a per-password salt from the library.
**Why.** A fast hash such as SHA-256 lets an attacker with the database try billions of guesses a second. A slow, salted, memory-hard hash makes each guess cost real time. A common baseline is Argon2id with at least 19 MiB of memory and 2 iterations. Check the current numbers when you review.
**FAIL**
```text
stored = hashlib.sha256(password.encode()).hexdigest()
```
**PASS**
```text
ph = argon2.PasswordHasher() # salt and parameters stored in the hash string
stored = ph.hash(password)
ph.verify(stored, attempt) # rehash when check_needs_rehash()
```
**Counterpoint.** "Argon2 is expensive at login." That cost is the defence; tune it to about a second on production hardware. Where FIPS compliance rules Argon2 out, PBKDF2-HMAC-SHA256 with a high iteration count is the accepted fallback.
## fn-reduced-motion-respected
severity: should-fix
concerns: accessibility, usability
applies_when: Styles or script add a transition, an animation, a parallax effect, or autoplay motion.
**Rule.** Switch off or replace large, repeating, or interaction-triggered motion when `prefers-reduced-motion: reduce` is set.
**Why.** Scaling, panning, and parallax can trigger nausea and dizziness for people with vestibular disorders. They have already told their device so. A page that ignores the setting makes them leave.
**FAIL**
```css
.hero { animation: zoom-in 1.2s ease-out infinite alternate; }
.card { transition: transform 0.6s; }
.card:hover { transform: scale(1.3) rotate(4deg); }
```
**PASS**
```css
.hero { animation: zoom-in 1.2s ease-out infinite alternate; }
@media (prefers-reduced-motion: reduce) {
.hero { animation: none; }
.card { transition: opacity 0.2s; }
.card:hover { transform: none; opacity: 0.9; }
}
```
**Counterpoint.** "Motion carries meaning, such as a loading spinner." Keep what the user needs and make it gentler: swap movement for a fade. The rule is about decorative and large movement, not about every pixel that moves.
## fn-regex-compiled-once
severity: should-fix
concerns: performance
applies_when: Code builds a regular expression or similar compiled matcher inside a loop or a function that runs per request.
**Rule.** Compile a regular expression once, at module or startup level, and reuse it.
**Why.** Compiling costs far more than matching and throws away reusable allocations. Inside a loop or a per-request function, the cost repeats on every call.
**FAIL**
```go
func valid(s string) bool {
return regexp.MustCompile(`^[a-z]+$`).MatchString(s) // compiled per call
}
```
**PASS**
```go
var lower = regexp.MustCompile(`^[a-z]+$`)
func valid(s string) bool { return lower.MatchString(s) }
```
**Counterpoint.** "The library caches patterns." Python and .NET keep recently used patterns, so a few static calls are fine. A pattern built from input or an unbounded set of patterns defeats the cache.
## fn-rerun-safe-after-crash
severity: block
concerns: reliability
applies_when: A function or job changes state and can be run twice, or can be killed partway.
**Rule.** Make a state-changing operation give the same result on a second run and after a crash at any step, by deriving each step from current state and making leftovers recognisable as stale.
**Why.** Jobs get retried and processes get killed. An append, a counter bump, or a lock file with no owner and time leaves the next run unable to tell finished work from half-done work. Record "done" only after the last step succeeds, so a crash leaves the work visibly pending.
**FAIL**
```python
cur.execute("UPDATE accounts SET balance = balance + %s WHERE id = %s", (amount, acct)) # a retry applies it twice
open("out.json", "w").write(data) # a crash leaves a truncated file
```
**PASS**
```python
cur.execute("INSERT INTO applied_ops (op_id) VALUES (%s) ON CONFLICT DO NOTHING", (op_id,))
if cur.rowcount == 1: # first run of this operation; same transaction as the update
cur.execute("UPDATE accounts SET balance = balance + %s WHERE id = %s", (amount, acct))
tmp = path.with_suffix(".tmp")
tmp.write_text(data)
os.replace(tmp, path) # the reader sees the old file or the new one, never half of one
```
**Counterpoint.** "Retries are rare." Crashes are not. For API calls that cross a network, [svc-idempotency-key](service.md#svc-idempotency-key) covers the key-based form of this rule.
## fn-response-fields-explicit
severity: should-fix
concerns: security, compatibility
applies_when: A handler returns a stored object or entity to the client.
**Rule.** Return a response type that lists each field, never the stored model or a generic dump of it.
**Why.** A model gains `password_hash`, `internal_notes` or `email` and every endpoint that serialises it leaks the new field silently. Callers also start to depend on whatever is sent, so removing a field later is a breaking change. An explicit response type makes each exposed field a decision someone reviewed. The write side is [fn-no-mass-assignment](function.md#fn-no-mass-assignment).
**FAIL**
```text
return jsonify(user.__dict__)
return user_model.to_dict() # all columns
```
**PASS**
```text
class UserOut(BaseModel): id: int; display_name: str
return UserOut.model_validate(user)
```
**Counterpoint.** "A response type per endpoint duplicates the model." Derive it from the model with a field list. The duplication is one line per field and shows up in the diff when a field is added.
## fn-status-announced-live-region
severity: should-fix
concerns: accessibility, usability
applies_when: Script shows a message after an async action without moving focus, such as "Saved", a result count, or an error.
**Rule.** Announce an async status message through a live region (`role="status"` or `role="alert"`) that is already in the markup before its text changes.
**Why.** A sighted user sees "Saved" appear. A screen-reader user hears nothing, because focus did not move. Most readers announce a change only in a region that existed first, so a region created together with its message is often skipped.
**FAIL**
```js
const note = document.createElement("div");
note.textContent = "Changes saved";
document.body.append(note);
```
**PASS**
```html
```
**Counterpoint.** "Move focus to the message instead." Use that for a blocking error after submit, where the user must act. For a passive result, moving focus steals the user's place. Keep `assertive` and `role="alert"` for errors, because they interrupt speech.
## fn-target-size
severity: should-fix
concerns: accessibility, usability
applies_when: Styles size a button, icon button, checkbox, or link that a pointer or finger operates.
**Rule.** Make each pointer target at least 24 by 24 CSS pixels, or space it so that a 24 px circle centred on it touches no other target and no other undersized target's circle.
**Why.** Small, tight controls cause mis-taps for anyone with a tremor, a large thumb, or a bumpy train. A mis-tap on "Delete" next to "Edit" costs more than a mis-tap on a menu item.
**FAIL**
```css
.icon-btn { width: 16px; height: 16px; margin: 0; }
```
```html
✎ 🗑
```
**PASS**
```css
.icon-btn { min-width: 24px; min-height: 24px; padding: 4px; }
```
**Counterpoint.** "Inline links and dense tables are exempt." Targets inside a sentence are, and so are controls with an equivalent control that is big enough. A row of toolbar icons is neither, and nor are the action buttons on a table row. Touch platforms often ask for 44 px or more, so treat 24 px as the floor, not the goal.
## fn-tls-verify-on
severity: block
concerns: security
applies_when: Code makes an outbound HTTPS, database or message-broker connection.
**Rule.** Leave certificate and hostname verification on in every environment that ships.
**Why.** With verification off, any on-path attacker presents a certificate and reads or changes the traffic. The flag is usually added to silence a dev-box error and then ships. Fix the trust store or pin a private CA instead.
**FAIL**
```text
requests.get(url, verify=False) # Python
https.request({ rejectUnauthorized: false }) // Node.js
```
**PASS**
```python
requests.get(url, verify="/etc/ssl/company-ca.pem") # private CA added, check stays on
```
**Counterpoint.** "It is an internal network." Internal links get sniffed after one compromised host. A private CA gives you the same convenience with the check intact.