Code Review

@RestController
public class OrderController {
    @Autowired private OrderRepository orderRepo;

    @GetMapping("/api/orders")
    public List<OrderDTO> listOrders(@RequestParam Long userId) {
        List<Order> orders = orderRepo.findByUserId(userId);
        List<OrderDTO> out = new ArrayList<>();
        for (Order o : orders) {
            OrderDTO dto = new OrderDTO();
            dto.id = o.getId();
            dto.total = o.getTotal();
            // Order has @OneToMany(fetch = LAZY) List<LineItem> items;
            dto.itemCount = o.getItems().size();
            out.add(dto);
        }
        return out;
    }
}

This endpoint lists a user's orders. What's the most serious issue?

Because items is LAZY, calling getItems().size() inside the loop issues one extra SELECT per order, classic N+1. Field injection is a style smell but not a correctness or performance issue at this scale.

import threading

class Counter:
    def __init__(self):
        self.value = 0

    def increment(self):
        self.value += 1

def run():
    c = Counter()
    threads = [threading.Thread(target=lambda: [c.increment() for _ in range(100000)])
               for _ in range(10)]
    for t in threads: t.start()
    for t in threads: t.join()
    assert c.value == 1000000, c.value

The assertion fails intermittently with a value less than 1000000. Why?

x += 1 compiles to LOAD, ADD, STORE, three bytecodes. A thread switch between LOAD and STORE drops the other thread's increment. The GIL guarantees one bytecode at a time, not that += is atomic. Wrap in threading.Lock or use itertools.count / atomic primitives.

public List<String> loadUserEmails(long userId) throws SQLException {
    Connection conn = dataSource.getConnection();
    PreparedStatement ps = conn.prepareStatement(
        "SELECT email FROM users WHERE org_id = ?");
    ps.setLong(1, userId);
    ResultSet rs = ps.executeQuery();
    List<String> out = new ArrayList<>();
    while (rs.next()) {
        out.add(rs.getString("email"));
    }
    rs.close();
    ps.close();
    conn.close();
    return out;
}

This runs fine in dev but the connection pool exhausts in production. What's wrong?

Manual close() calls don't run if executeQuery or rs.next() throws, the connection leaks back to nothing. Use try-with-resources so JDBC resources are closed even on exception. The parameter naming (d) is a real code smell but not what exhausts the pool.

import express from "express";
import { pool } from "./db";

const app = express();
app.use(express.json());

app.get("/api/users/search", async (req, res) => {
  const q = String(req.query.q ?? "");
  const sql = "SELECT id, email FROM users WHERE email LIKE '%" + q + "%' LIMIT 50";
  const { rows } = await pool.query(sql);
  res.json(rows);
});

What's the most serious issue with this search endpoint?

q comes straight from the query string into the SQL string. A request like ?q=%27 OR 1=1-- dumps every row, and worse payloads can drop tables or exfiltrate data. Use parameterized queries: pool.query('... LIKE $1 ...', ['%' + q + '%']). Index usage (a) is a perf concern, not a vuln.

async function renderComments(postId) {
  const res = await fetch("/api/comments?post=" + postId);
  const comments = await res.json();
  const container = document.getElementById("comments");
  container.innerHTML = comments
    .map(c => "<div class='comment'>" +
              "<b>" + c.author + "</b>: " + c.body +
              "</div>")
    .join("");
}

What's the most serious issue here?

A comment body of <img src=x onerror=fetch('/steal?c='+document.cookie)> executes the moment it's painted. Use textContent for plain text, or build elements with createElement and assign properties, never interpolate user data into an HTML string. URLSearchParams (a) is cleaner code but not a vuln in this shape.

from fastapi import FastAPI
import psycopg2

app = FastAPI()
conn = psycopg2.connect("dbname=app user=app")

@app.get("/users/{uid}")
async def get_user(uid: int):
    cur = conn.cursor()
    cur.execute("SELECT id, email FROM users WHERE id = %s", (uid,))
    row = cur.fetchone()
    cur.close()
    if row is None:
        return {"error": "not found"}
    return {"id": row[0], "email": row[1]}

This endpoint serves fine under low load but tail latency explodes under concurrency. Why?

FastAPI runs async def handlers directly on the event loop. A blocking call there freezes every other in-flight request until the query returns, so latency snowballs under concurrency. Use an async driver (asyncpg, psycopg3 async) or define the handler as def so FastAPI runs it in a threadpool. The shared connection (a) is also wrong but the event-loop block is the load-bearing bug.

public void writeReport(String userPath, byte[] data) throws IOException {
    File f = new File("/var/reports/" + userPath);
    if (f.exists()) {
        throw new IOException("Report already exists: " + userPath);
    }
    try (FileOutputStream out = new FileOutputStream(f)) {
        out.write(data);
    }
}

What is the most serious correctness issue (assume userPath is already validated to be a safe filename)?

exists() and the open() are two separate syscalls. Between them, anything can create the file, including a malicious symlink, and FileOutputStream will happily overwrite it. The atomic fix is Files.newOutputStream(path, StandardOpenOption.CREATE_NEW), which fails atomically if the file exists.

from flask import Flask, request, jsonify

app = Flask(__name__)

def flatten(node):
    out = []
    if isinstance(node, list):
        for child in node:
            out.extend(flatten(child))
    else:
        out.append(node)
    return out

@app.post("/flatten")
def handler():
    body = request.get_json()
    return jsonify(flatten(body))

An attacker can crash this worker with a single small request. What's the issue?

A payload like [[[[...[1]...]]]] with ~1000 levels overflows the default recursion limit and kills the worker with RecursionError. Convert to an explicit stack-based iterative flatten, or cap depth before recursing. (c) is wrong, list.extend is amortized linear.

const sessions = new Map<string, { lastSeen: number }>();

function reapExpired(now: number, ttlMs: number) {
  for (const [id, session] of sessions) {
    if (now - session.lastSeen > ttlMs) {
      sessions.delete(id);
    }
  }
}

function touch(id: string) {
  sessions.set(id, { lastSeen: Date.now() });
}

What's the subtle bug in reapExpired?

JS Map iteration explicitly supports delete-during-iteration, option (a) is wrong. The real hazard is concurrency: in Node, an await or a callback during reaping (or, more concretely, code that hands the Map to multiple async contexts) can race touch() against delete(), losing live sessions. Snapshot keys or guard the section.

import java.math.BigDecimal;

public class PriceCheck {
    public static boolean isFreeShipping(BigDecimal subtotal) {
        BigDecimal threshold = new BigDecimal("50.00");
        return subtotal.equals(threshold) || subtotal.compareTo(threshold) > 0;
    }

    public static void main(String[] args) {
        BigDecimal s = new BigDecimal("50");
        System.out.println(isFreeShipping(s));
    }
}

main prints true, but a similar-looking input prints false. Where's the bug?

BigDecimal.equals considers both value AND scale: new BigDecimal("50").equals(new BigDecimal("50.00")) is false. Use compareTo(other) == 0 for numeric equality. The compareTo > 0 branch in this code happens to mask the bug when subtotal is greater, but the equals call itself is unreliable.

function dedupePreserveOrder(items) {
  const out = [];
  for (const item of items) {
    if (out.indexOf(item) === -1) {
      out.push(item);
    }
  }
  return out;
}

function deduplicateEvents(events) {
  return dedupePreserveOrder(events.map(e => e.id));
}

This works correctly but melts at production scale. What's the issue?

For each of n items, indexOf scans up to n previously-added items: total work is n*(n+1)/2. At n=100k that's ~5 billion comparisons. Use a Set for O(1) membership: const seen = new Set(); for (...) if (!seen.has(item)) { seen.add(item); out.push(item); }

def withdraw(balance: float, amount: float) -> float:
    if balance == amount:
        return 0.0
    if balance < amount:
        raise ValueError("insufficient funds")
    return balance - amount

balance = 0.1 + 0.2
print(withdraw(balance, 0.3))

Why does this raise ValueError instead of returning 0.0?

0.1 and 0.2 have no exact binary representation; their sum is slightly greater than 0.3. == on floats almost never does what you want. Use math.isclose(a, b) for tolerance-based comparison, or switch to decimal.Decimal for money. Annotations (d) don't coerce at runtime.

@RestController
public class AuthController {
    @Value("${admin.token}") private String adminToken;

    @PostMapping("/admin/login")
    public ResponseEntity<String> login(@RequestBody LoginReq req) {
        if (req.token != null && req.token.equals(adminToken)) {
            return ResponseEntity.ok(issueSession());
        }
        return ResponseEntity.status(401).body("nope");
    }

    private String issueSession() { /* ... */ return "session-cookie"; }
}

What's the security flaw here?

String.equals returns at the first byte that differs, so comparing 'a*****' takes measurably less time than 'admin1***'. An attacker who can measure response time can recover the token byte by byte. Use MessageDigest.isEqual(a.getBytes(), b.getBytes()) which is constant-time. Rate limiting (d) is good defense in depth but doesn't fix the side channel.

const userCache = new Map<string, User>();

export async function getUser(id: string): Promise<User> {
  const cached = userCache.get(id);
  if (cached) return cached;
  const user = await db.users.findById(id);
  userCache.set(id, user);
  return user;
}

export async function updateUserEmail(id: string, email: string): Promise<void> {
  await db.users.update(id, { email });
}

Users report their email change doesn't show up until the process restarts. Why?

Classic cache-invalidation bug: writes go to the source of truth, reads come from a stale snapshot. Either userCache.delete(id) after the update, or write-through (userCache.set with the new value). WeakMap (d) keys by object identity, which doesn't help here.

const express = require("express");
const cookieParser = require("cookie-parser");
const app = express();
app.use(cookieParser());
app.use(express.json());

function requireSession(req, res, next) {
  const sid = req.cookies.sid;
  if (!sid || !sessions.has(sid)) return res.status(401).end();
  req.user = sessions.get(sid);
  next();
}

app.post("/account/transfer", requireSession, async (req, res) => {
  const { toAccount, amount } = req.body;
  await bank.transfer(req.user.id, toAccount, amount);
  res.json({ ok: true });
});

A logged-in user visits evil.com and money leaves their account. What's missing?

Cookie auth alone identifies the user but doesn't prove the request was intentional. evil.com can submit a form (or fetch with credentials) to /account/transfer; the browser attaches the session cookie. Mitigations: SameSite=Lax/Strict on the cookie (modern default), a CSRF token the server checks, or requiring a custom header that browsers won't add cross-origin without a preflight.

function chunk(items, size) {
  const out = [];
  for (let i = 0; i < items.length - size; i += size) {
    out.push(items.slice(i, i + size));
  }
  return out;
}

// Send the newsletter in batches of 2
for (const batch of chunk(recipients, 2)) {
  sendBatch(batch);
}

With four recipients, only the first two get the newsletter. What's the bug?

With 4 items and a size of 2, the condition is i < 2, so the loop runs once and returns [[1, 2]]. With 5 items it returns [[1, 2], [3, 4]] and drops the 5. The bound should be i < items.length; slice already clamps at the end of the array, so a short final batch comes out right. slice's exclusive end is exactly what a batch needs, and it returns a copy.

def latest_order_date(customer):
    orders = customer.get("orders")
    latest = max(orders, key=lambda o: o["placed_at"])
    return latest["placed_at"]

@app.get("/account/<int:customer_id>")
def account(customer_id):
    customer = api.fetch_customer(customer_id)
    return render("account.html", last_ordered=latest_order_date(customer))

This fills the Last Ordered line on the account page. It passed every test, then crashed for brand-new accounts. What did the review miss?

customer.get("orders") returns None when the key is missing, and max(None) raises TypeError; an empty list raises ValueError: max() arg is an empty sequence. Handle both before calling max: orders = customer.get("orders") or [], then if not orders: return None. get never raises KeyError, which is the point of using it, and key runs once per order as intended.

function applyDiscount(cart, percent) {
  for (const item of cart.items) {
    item.price = item.price * (1 - percent / 100);
  }
  return cart;
}

// Checkout page
const preview = applyDiscount(cart, 10);
showPreview(preview);
showCart(cart);

Every time the discount preview refreshes, the prices in the cart drop by another 10 percent. What's the cause?

The function receives a reference to the caller's cart, so item.price = ... writes into the same objects the cart holds. preview === cart is true, the real cart is discounted, and the next refresh discounts the already discounted prices: 20 becomes 18, then 16.2. Build new objects instead: return { ...cart, items: cart.items.map((item) => ({ ...item, price: item.price * (1 - percent / 100) })) }. for...of hands the loop the real item objects, which is why the write sticks.

const fs = require("fs");

function readHeader(path) {
  const fd = fs.openSync(path, "r");
  const buf = Buffer.alloc(16);
  fs.readSync(fd, buf, 0, 16, 0);
  return buf.toString("hex");
}

// Check the first 16 bytes of every upload
for (const file of uploadedFiles) {
  checkSignature(readHeader(file));
}

This batch job works on small folders and fails on large ones with EMFILE: too many open files. Why?

Each openSync returns a descriptor the process holds until it calls fs.closeSync, and Node does not close raw descriptors for you. With a limit of 256 open files, the loop fails after roughly 250 uploads. Put the read in a try block and call fs.closeSync(fd) in finally, so the descriptor is released even when readSync throws. Buffer.alloc zero-fills (Buffer.allocUnsafe is the one that doesn't), and a positioned read takes no lock.

import json

DEFAULTS = {"currency": "USD", "send_receipts": True}

def load_settings(path):
    try:
        with open(path) as f:
            return json.load(f)
    except Exception:
        return {}

settings = {**DEFAULTS, **load_settings("settings.json")}

After someone edited settings.json, the service started running on its defaults, and nothing in the logs says why. What's the most important review comment?

A trailing comma in the edited file makes json.load raise JSONDecodeError, and the broad except turns that into {}, so the service quietly falls back to DEFAULTS. Catch only the case you expect and can handle, here except FileNotFoundError: return {}, and let everything else raise so a bad file fails loudly at startup. with closes the file on return, and KeyboardInterrupt derives from BaseException, which except Exception does not catch.

function calc(d: Order[]): number {
  let t = 0;
  for (const x of d) {
    if (x.status === "paid") t += x.amount * 1.08;
  }
  return t;
}

function calc2(d: Order[]): number {
  let t = 0;
  for (const x of d) {
    if (x.status === "refunded") t += x.amount * 1.08;
  }
  return t;
}

Both functions return the right totals. What's the most useful review comment?

A reader has to trace each body to learn that calc means the paid total with tax, and when the rate changes, someone will update one copy and miss the other. One function fixes both: const TAX_RATE = 0.08 and totalWithTax(orders: Order[], status: OrderStatus). const can't hold a value the loop reassigns, the speed of the two loop styles is the same in practice, and returning paid minus refunded changes what callers receive, which belongs in a separate change.