Python Code Review Exercises

Issues found:

• Vague function name. process tells you nothing about what this function does. A name like categorize_input or classify_user_data would communicate intent.
• No return value. The function uses print() for output instead of returning structured data. This makes the function impossible to test and unusable in any pipeline.
• Bare except clause. Catching every exception silently hides real errors. If data contains a non-string item like None, the "@" in item check will throw a TypeError, and it will be swallowed.
• Naive email detection. Checking for @ alone would match strings like "price@100" that are not email addresses.
• No docstring or type hints. The function signature gives no indication of expected input or output types.

Improved version:

def classify_user_data(data: list[str]) -> dict[str, list]:
    """Classify a list of string inputs into emails, numbers, and other values."""
    results = {"emails": [], "numbers": [], "other": []}

    for item in data:
        if "@" in item and "." in item.split("@")[-1]:
            results["emails"].append(item)
        else:
            try:
                results["numbers"].append(int(item) * 2)
            except ValueError:
                results["other"].append(item)

    return results

The bug: Mutable default arguments in Python are evaluated once at function definition time, not on each call. Every invocation of create_cart that relies on the default shares the same list object. This is one of the more well-known Python gotchas, and it causes real production bugs when developers use default lists or dictionaries as function parameters.

Improved version:

def create_cart(item: str, cart: list[str] | None = None) -> list[str]:
    """Add an item to a cart. Creates a new cart if none is provided."""
    if cart is None:
        cart = []
    cart.append(item)
    return cart

The fix is to use None as the default and create a new list inside the function body. This pattern applies to any mutable default: lists, dictionaries, and sets should all use None instead.

Issues found:

• Repeated string concatenation. Each + operation creates a new string object. For a simple function like this the performance hit is negligible, but the pattern is a bad habit that scales poorly when used in loops or with large datasets.
• Poor readability. The structure of the final output is not obvious from the code. You have to mentally trace through each concatenation to understand the format.
• No type hints or docstring.
• No input validation. Passing None for any argument will crash the function with a TypeError.

Improved version:

def format_log(level: str, timestamp: str, user: str, message: str) -> str:
    """Format a structured log entry string."""
    return f"[{level}] {timestamp} | User: {user} | {message}"

F-strings are the idiomatic way to build formatted strings in modern Python. They are faster than concatenation, and the output format is immediately visible in the code.

Issues found:

• No context manager. If json.loads() throws a json.JSONDecodeError, the file handle is never closed. The f.close() call is unreachable in the error path. Using a with statement guarantees cleanup regardless of exceptions.
• Redundant read step. json.loads() takes a string, but json.load() reads directly from a file object. The intermediate data variable is unnecessary.
• No error handling. A missing file will raise an unhandled FileNotFoundError. Corrupted JSON will raise a JSONDecodeError. Neither is caught or given a useful error message.
• No type hints or docstring.

Improved version:

import json
from pathlib import Path

def load_config(filepath: str | Path) -> dict:
    """Load and return a JSON configuration file as a dictionary.

    Raises:
        FileNotFoundError: If the config file does not exist.
        json.JSONDecodeError: If the file contains invalid JSON.
    """
    filepath = Path(filepath)
    if not filepath.exists():
        raise FileNotFoundError(f"Config file not found: {filepath}")

    with open(filepath, "r", encoding="utf-8") as f:
        return json.load(f)

Issues found:

• Class-level mutable attribute. users = [] defined at the class level means every instance of UserManager shares the same list. This is the class-variable version of the mutable default argument problem from Exercise 2.
• Plaintext passwords. Storing passwords as raw strings is a critical security flaw. Passwords should be hashed using a library like bcrypt or Python's built-in hashlib.
• Print statements instead of logging or exceptions. Using print() for error reporting makes the class impossible to integrate with any real application.
• File I/O mixed with business logic. The register method handles validation, storage in memory, and file persistence all at once. These responsibilities should be separated.
• Linear search for duplicate checking. Iterating through the full list to check for existing users is O(n). A dictionary or set lookup would be O(1).
• No __init__ method. Instance-level state should be initialized in the constructor.
• No type hints or docstrings.

Improved version (partial -- focusing on structure):

import hashlib
import secrets

class UserManager:
    def __init__(self):
        self._users: dict[str, str] = {}

    def register(self, username: str, password: str) -> None:
        """Register a new user with a hashed password."""
        if username in self._users:
            raise ValueError(f"Username already exists: {username}")

        salt = secrets.token_hex(16)
        hashed = hashlib.sha256((salt + password).encode()).hexdigest()
        self._users[username] = f"{salt}:{hashed}"

    def authenticate(self, username: str, password: str) -> bool:
        """Verify credentials against stored hash."""
        if username not in self._users:
            return False

        stored = self._users[username]
        salt, expected_hash = stored.split(":")
        actual_hash = hashlib.sha256((salt + password).encode()).hexdigest()
        return secrets.compare_digest(actual_hash, expected_hash)

Issues found:

• Overly broad exception handling. Catching Exception and continuing silently means any error -- a KeyError from a missing field, a TypeError from unexpected data, even a MemoryError -- gets swallowed without a trace. If one response in a list of a thousand is malformed, you will never know data was skipped.
• Silent data loss via continue. Combined with the broad except, continue means the function never reports partial failures. This makes debugging extremely difficult in production.
• Duplicate email overwrites. If two responses contain the same email address with different data, the second one silently overwrites the first. This may or may not be intentional, but it should be documented.
• No logging. Errors should at minimum be logged so you can trace data quality issues.

Improved version:

import logging

logger = logging.getLogger(__name__)

def extract_users(api_responses: list[dict]) -> dict[str, dict]:
    """Extract user records from a list of API response payloads."""
    users = {}

    for i, response in enumerate(api_responses):
        try:
            data = response["data"]
        except (KeyError, TypeError) as e:
            logger.warning("Response %d has invalid structure: %s", i, e)
            continue

        for user in data.get("users", []):
            try:
                email = user["email"]
                if email in users:
                    logger.info("Duplicate email %s in response %d, overwriting", email, i)
                users[email] = {
                    "name": user["name"],
                    "role": user.get("role", "unknown"),
                }
            except KeyError as e:
                logger.warning("User record missing required field %s in response %d", e, i)

    return users

Critical issue: SQL injection. The username is inserted directly into the query string using an f-string. An attacker could pass a value like ' OR '1'='1 to bypass authentication, or '; DROP TABLE users; -- to destroy data. This is one of the oldest and most dangerous classes of web application vulnerabilities.

Additional issues:

• No context manager for the database connection. If fetchone() raises an exception, the connection is never closed.
• Hardcoded database path. The connection string should come from configuration, not a literal in the function body.
• SELECT * is wasteful. Fetching all columns when you may only need a few wastes memory and exposes unnecessary data.

Improved version:

import sqlite3
from pathlib import Path

def get_user(username: str, db_path: str | Path = "app.db") -> tuple | None:
    """Retrieve a user record by username using parameterized queries."""
    with sqlite3.connect(db_path) as conn:
        cursor = conn.cursor()
        cursor.execute(
            "SELECT id, username, email, role FROM users WHERE username = ?",
            (username,)
        )
        return cursor.fetchone()

Parameterized queries (using ? placeholders) let the database driver handle escaping. The user input never becomes part of the SQL statement itself, which eliminates the injection vector entirely.

The bug: race condition. The record_event method performs a check-then-act sequence (check if the key exists, then increment) that is not atomic. When two threads call record_event("click") simultaneously, the following can happen:

• Thread A reads self.counts["click"] as 5
• Thread B reads self.counts["click"] as 5
• Thread A writes 6
• Thread B writes 6

One increment is lost. Over thousands of events, the count will drift significantly from the real number.

Improved version:

import threading
from collections import defaultdict

class EventCounter:
    def __init__(self):
        self._counts: dict[str, int] = defaultdict(int)
        self._lock = threading.Lock()

    def record_event(self, event_name: str) -> None:
        """Increment the count for the given event in a thread-safe manner."""
        with self._lock:
            self._counts[event_name] += 1

    def get_count(self, event_name: str) -> int:
        """Return the current count for an event."""
        with self._lock:
            return self._counts[event_name]

The threading.Lock ensures that only one thread at a time can read or modify the counts dictionary. Using defaultdict(int) also eliminates the need for the existence check, since missing keys automatically start at 0.

The performance trap: The in operator on a list performs a linear scan. For each of the 500,000 records, Python checks every item already in unique to determine membership. This makes the function O(n^2) -- for half a million records, that is up to 250 billion comparisons in the worst case.

Improved version:

def deduplicate(records: list[str]) -> list[str]:
    """Remove duplicate entries while preserving original order."""
    seen: set[str] = set()
    unique: list[str] = []
    for record in records:
        if record not in seen:
            seen.add(record)
            unique.append(record)
    return unique

Using a set for membership checks brings each lookup down to O(1) average time, making the entire function O(n). For 500,000 records, this is the difference between hours and milliseconds.

If order does not matter, an even simpler approach works:

def deduplicate(records: list[str]) -> list[str]:
    """Remove duplicates (order not guaranteed)."""
    return list(set(records))