Reading & Reviewing Code

How to approach an unfamiliar codebase

When you see a new codebase — whether it's AI-generated or inherited from a colleague — there's a systematic way to read it that takes 10 minutes and gives you 80% of the picture.

Step 1: Read the entry point first

Every program has an entry point — the place where execution begins. In Python, it's usually the if __name__ == "__main__": block, or the main API file in a web service. Start there. Don't read random files — follow the flow from where it starts.

Step 2: Trace the data

Pick one key piece of data — a user ID, a flight record, a request body — and follow it through the system. Where does it come in? What happens to it? Is it validated before use? Does it get stored? Where? This trace reveals the architecture and most of the important bugs.

Step 3: Look at the structure

How many files? How are they named? Is there clear separation of concerns (data access vs. business logic vs. presentation)? Or is everything mixed together in one 800-line file? The structure tells you whether the code will be maintainable.

Step 4: Read the tests

If there are tests, read them before the implementation. Tests tell you what the code is supposed to do — they're better documentation than comments. If there are no tests, that's already a flag.

What to look for: the flaw checklist

This is the practical checklist you'll use in every code review. Build it into your mental model now.

🔴 Security: the highest-priority category

SQL injection — any time user input is directly concatenated into a SQL string, the database is vulnerable to being completely compromised.

# ❌ SQL injection — attacker can input "'; DROP TABLE users; --"
query = f"SELECT * FROM users WHERE email = '{user_input}'"
cursor.execute(query)

# ✓ Parameterized query — input is treated as data, not code
cursor.execute("SELECT * FROM users WHERE email = %s", (user_input,))

Hardcoded secrets — API keys, passwords, database credentials in the code. Any time you see a string that looks like sk-, Bearer , a connection string with a password, or anything labelled _key, _secret, _token with a real value, flag it immediately.

No authentication check — an endpoint that takes a user ID and returns their data without checking that the caller is authorized to see that user's data. Classic: GET /users/{id}/profile with no auth check. Any logged-in user could see any other user's data.

Missing input validation — any user-provided data that goes directly into business logic or a database without being validated for type, length, and allowed values.

# ❌ No validation — what if age is -1? Or "drop table"? Or 999999?
def create_user(name: str, age: int) -> User:
    return db.insert("users", name=name, age=age)

# ✓ Validate before using
def create_user(name: str, age: int) -> User:
    if not name.strip():
        raise ValidationError("name", "cannot be empty")
    if not (0 <= age <= 150):
        raise ValidationError("age", f"must be 0-150, got {age}")
    return db.insert("users", name=name.strip(), age=age)

🟠 Reliability: errors that cause silent failures

Bare except / eating errors — from Module A1. Any except: pass or except Exception: return None without logging is hiding a problem. Future-you will spend hours debugging a failure that left no trace because someone ate the exception.

Resource leaks — files, database connections, network sockets that are opened but might not be closed if an exception occurs.

# ❌ File won't be closed if process() raises an exception
f = open("data.txt")
data = f.read()
process(data)    # if this raises, f.close() never runs
f.close()

# ✓ Context manager guarantees closure even on exception
with open("data.txt") as f:
    data = f.read()
process(data)    # f is closed whether or not this raises

Off-by-one errors — boundary conditions: is it < or <=? Should the loop go to len(items) or len(items) - 1? Does the range start at 0 or 1? These are responsible for a disproportionate share of real bugs.

None/null dereference — accessing an attribute or calling a method on something that might be None. Look for result.field where result could be None. Common in AI-generated code that doesn't handle the "not found" case.

# ❌ Crashes with AttributeError if find_user returns None
user = find_user(user_id)
print(user.name)   # boom if user is None

# ✓ Check first
user = find_user(user_id)
if user is None:
    raise NotFoundError("User", user_id)
print(user.name)

🟡 Design: code that works today but creates problems tomorrow

Magic strings and numbers — bare strings or numbers in logic that have no obvious meaning. What does status == "a" mean? What does if score > 85 mean? Use Enums and named constants.

Functions doing too much — a 200-line function that validates input, fetches from a database, applies business logic, formats a response, and logs everything. Each function should do one thing. When you see a function this long, ask: what are the natural split points?

Inconsistent error handling — some functions return None on error, some raise exceptions, some return error strings. A codebase should have one consistent pattern — pick one and stick to it.

Writing a useful code review

A code review that says "looks good 👍" teaches nothing. A review that lists 20 style nitpicks without addressing real issues is noise. Aim for something in between: a review that addresses the most important things, explains why they matter, and suggests a fix rather than just identifying the problem.

The priority order in a review:

1. Correctness bugs — code that will produce wrong results in certain inputs
2. Security issues — any of the patterns from the security section above
3. Error handling — silent failures, resource leaks
4. Design issues — things that will be painful to change in 3 months
5. Readability — naming, comments, structure. Important, but last.

The bus factor test

Ask this about any piece of code: "If the person who wrote this got hit by a bus tomorrow, how long would it take someone else to understand it?" Good code minimises that time through clear naming, logical structure, and helpful comments where the "why" isn't obvious.

Apply this test to AI-generated code especially. AI produces code that works but often has poor naming (generic variable names like data, result, temp) and no explanation of non-obvious decisions.

from flask import Flask, request, jsonify
import sqlite3

app = Flask(__name__)
SECRET_KEY = "my-super-secret-key-1234"          # ← look here

def get_db():
    return sqlite3.connect("users.db")

@app.route("/users/<user_id>")
def get_user(user_id):
    db = get_db()                                  # ← look here
    cursor = db.cursor()
    query = f"SELECT * FROM users WHERE id = {user_id}"  # ← look here
    try:
        cursor.execute(query)
        user = cursor.fetchone()
        return jsonify({"user": user})
    except:                                        # ← look here
        return jsonify({"error": "failed"}), 500

@app.route("/users", methods=["POST"])
def create_user():
    data = request.json
    name = data["name"]                          # ← look here
    age = data["age"]
    db = get_db()
    db.execute(
        f"INSERT INTO users (name, age) VALUES ('{name}', {age})",  # ← look here
    )
    db.commit()
    return jsonify({"status": "created"}), 201