Code review prompt to de-sloppify code

ai-code-review-checklist.md

Can you do a first pass of cleanups? Pay attention to:

• Leaked keys

• Indirectly leaked keys

• Overly defensive programming and hiding failures instead of letting errors bubble up, also while adding too much code.

  • This is perhaps the most common and annoying. The AI wants to build trust by making code that can’t fail, but that ultimately is just gaslighting us into thinking it works / is robust. It’s delaying finding issues and making unmaintainable code.

  Here is an example:

  function readPositiveIntegerQuery(
  +  value: unknown,
  +  defaultValue: number,
  +  maxValue: number
  +) {
  +  const parsed =
  +    typeof value === "string" && value.trim().length > 0
  +      ? Number.parseInt(value, 10)
  +      : Number.NaN;
  +
  +  if (!Number.isFinite(parsed) || parsed <= 0) {
  +    return defaultValue;
  +  }
  +
  +  return Math.min(parsed, maxValue);
  +}

  This code could be much simpler and shorter if it was honest with failures, which means throwing early on bad inputs and letting errors bubble up rather than trying to accept any garbage input.

  function readPositiveIntegerQuery({ value: unknown, maxValue?: number }) {
    const parsed = Number.parseInt(value, 10);
  
    if (!Number.isFinite(parsed)) {
       throw new Error(...);
    }
  
    if (parsed <= 0) {
       throw new Error(...);
    }
  
    if (maxValue && parsed > maxValue) {
       throw new Error(...);
    }
  
    return parsed;
  }

  why this is better:

  • clear if statements with short conditions
  • no complex or multiline ternary statements
  • failures will be clear rather than confusing for future debugging sessions. This code is being honest about failures.
  • The calling code will have to be well constructed, which is a feature, not a bug.

  Other example of being to lousy with inputs

  diff --git a/src/src/services/runtimeSpecs.ts b/src/src/services/runtimeSpecs.ts
    index 8b42f42..cf3a995 100644
    --- a/src/src/services/runtimeSpecs.ts
    +++ b/src/src/services/runtimeSpecs.ts
    @@ -196,6 +196,10 @@ async function resolveCatalogSnapshot({
       pointer: ProductPointer;
       legacyProductNames: Set<string>;
     }) {
    +  if (pointer.status === "deleted") {
    +    return null;
    +  }
    +

  We should be throwing in this case. Why would be trying to resolve a deleted snapshot? that would indicate the UI needs fixing. We are not doing the frontend developper a favor by hiding a failure to properly handle versions.

• Unreadable long function bodies with no comments or not enough breaking down in small functions.

• Files that grow unreasonably large instead of being split into tiner files.

• Not enough comments (explaining the why)

• Bad types (sprinkled any)

• Code duplication, functions, classes and utils being redefined while something already existed elsewhere in the code base.

• Attempt to cover for lack of knowledge by ensuring a thousand hypotetical scenarios are handled. Happens especially when troubleshooting prod issues with AIs. Lets be honest if we are not sure of the fix and do more investigations rather than fix attempts.

• Mixing infra and app code.

• Middle of the file imports.

• Changing stuff that did not really need changing, moving functions or code around in ways that makes review more difficult.

• New functionality added without expanding existing test suites.

• Too much branching in a function that would benefit from being split in multiple function with less branches.

• Passing down a ton of parameters instead of using shared context. (Frontend)