Python Code Review

Python Code Review

Quick Reference

Review Checklist

PEP8 Style

• [ ] 4-space indentation (no tabs)
• [ ] Line length ≤79 characters (≤72 for docstrings/comments)
• [ ] Two blank lines around top-level definitions, one within classes
• [ ] Imports grouped: stdlib → third-party → local (blank line between groups)
• [ ] No whitespace inside brackets or before colons/commas
• [ ] Naming: snake_case for functions/variables, CamelCase for classes, UPPER_CASE for constants
• [ ] Inline comments separated by at least two spaces

Type Safety

• [ ] Type hints on all function parameters and return types
• [ ] No Any unless necessary (with comment explaining why)
• [ ] Proper T | None syntax (Python 3.10+)

Async Patterns

• [ ] No blocking calls (time.sleep, requests) in async functions
• [ ] Proper await on all coroutines

Error Handling

• [ ] No bare except: clauses
• [ ] Specific exception types with context
• [ ] raise ... from to preserve stack traces

Common Mistakes

• [ ] No mutable default arguments
• [ ] Using logger not print() for output
• [ ] f-strings preferred over .format() or %

Valid Patterns (Do NOT Flag)

These patterns are intentional and correct - do not report as issues:

• Type annotation vs type assertion - Annotations declare types but are not runtime assertions; don't confuse with missing validation
• Using `Any` when interacting with untyped libraries - Required when external libraries lack type stubs
• Empty `__init__.py` files - Valid for package structure, no code required
• `noqa` comments - Valid when linter rule doesn't apply to specific case
• Using `cast()` after runtime type check - Correct pattern to inform type checker of narrowed type

Context-Sensitive Rules

Only flag these issues when the specific conditions apply:

Gates (reporting workflow)

Complete in order. Do not advance until each pass condition is met.

1. Scope — Pass: You list every .py path (or explicit glob) you inspected this run. 2. False-positive screen — Pass: For each issue you plan to report, you checked Valid Patterns and Context-Sensitive Rules above; you drop or narrow the finding if those sections say not to flag it. 3. Evidence — Pass: Each remaining finding includes `[FILE:LINE]` (or a bounded line range). Symbols or short verbatim snippets may supplement the location anchor but do not replace it. 4. Verification protocol — Pass: You load review-verification-protocol and complete its mandatory steps for each reported issue before the user-facing write-up. 5. Ship — Pass: The user-visible output matches whatever structure that protocol requires (no issues-only dump that skips its checks).

When to Load References

• Reviewing code formatting/style → pep8-style.md
• Reviewing function signatures → type-safety.md
• Reviewing async def functions → async-patterns.md
• Reviewing try/except blocks → error-handling.md
• General Python review → common-mistakes.md

Review Questions

1. Does the code follow PEP8 formatting (indentation, line length, whitespace)? 2. Are imports properly grouped (stdlib → third-party → local)? 3. Do names follow conventions (snake_case, CamelCase, UPPER_CASE)? 4. Are all function signatures fully typed? 5. Are async functions truly non-blocking? 6. Do exceptions include meaningful context? 7. Are there any mutable default arguments?

Before reporting: complete Gates (reporting workflow) above (especially gate 4).

Async Patterns

Critical Anti-Patterns

1. Blocking Calls in Async Functions

Problem: Blocks the event loop, defeats async benefits.

# BAD - blocks event loop
async def fetch_data():
    response = requests.get(url)  # BLOCKING!
    time.sleep(1)  # BLOCKING!
    return response.json()

# GOOD - non-blocking
async def fetch_data():
    async with httpx.AsyncClient() as client:
        response = await client.get(url)
    await asyncio.sleep(1)
    return response.json()

2. Missing await on Coroutines

Problem: Coroutine never executes.

# BAD - coroutine created but never awaited
async def process():
    fetch_data()  # Returns coroutine, doesn't execute!

# GOOD
async def process():
    await fetch_data()

3. Sequential Instead of Concurrent

Problem: Misses parallelization opportunity.

# BAD - sequential (slow)
async def get_all():
    user = await get_user()
    posts = await get_posts()
    comments = await get_comments()
    return user, posts, comments

# GOOD - concurrent (fast)
async def get_all():
    user, posts, comments = await asyncio.gather(
        get_user(),
        get_posts(),
        get_comments()
    )
    return user, posts, comments

4. Missing async with for Async Context Managers

Problem: Resource not properly managed.

# BAD
async def query():
    session = aiosqlite.connect(db)  # Not entered!
    return await session.execute(sql)

# GOOD
async def query():
    async with aiosqlite.connect(db) as session:
        return await session.execute(sql)

5. Sync File I/O in Async Context

Problem: File operations block event loop.

# BAD - blocks event loop
async def read_config():
    with open("config.json") as f:
        return json.load(f)

# GOOD - use aiofiles
import aiofiles

async def read_config():
    async with aiofiles.open("config.json") as f:
        content = await f.read()
        return json.loads(content)

# ACCEPTABLE - for small files, run in executor
async def read_config():
    loop = asyncio.get_event_loop()
    return await loop.run_in_executor(None, load_config_sync)

Review Questions

1. Are there any requests, time.sleep, or open() calls in async functions? 2. Is every coroutine call awaited? 3. Are independent async calls parallelized with gather()? 4. Are async context managers used with async with?

Common Mistakes

Critical Anti-Patterns

1. Mutable Default Arguments

Problem: Default value is shared across all calls.

# BAD - same list reused!
def add_item(item, items=[]):
    items.append(item)
    return items

add_item("a")  # ["a"]
add_item("b")  # ["a", "b"] - unexpected!

# GOOD
def add_item(item, items=None):
    if items is None:
        items = []
    items.append(item)
    return items

# BETTER - using dataclass
from dataclasses import dataclass, field

@dataclass
class Container:
    items: list = field(default_factory=list)

2. Using print() for Logging

Problem: No log levels, no timestamps, hard to filter.

# BAD
print(f"Processing {item}")
print(f"Error: {e}")

# GOOD
from loguru import logger

logger.info(f"Processing {item}")
logger.error(f"Error: {e}")

3. String Formatting Inconsistency

Problem: Mixing formats reduces readability.

# BAD - mixed formats
msg = "Hello %s" % name
msg = "Hello {}".format(name)
msg = f"Hello {name}"

# GOOD - f-strings consistently
msg = f"Hello {name}"
total = f"Count: {count:,}"  # with formatting
path = f"{base}/{sub}/{file}"

4. Unused Variables

Problem: Dead code, confusing to readers.

# BAD
result = process()  # never used

# GOOD - use underscore for intentionally ignored
_, second, _ = get_triple()

# Or just don't assign
process()  # if result not needed

5. Import Order

Problem: Hard to scan, may cause issues.

# BAD - random order
from myapp.utils import helper
import os
from typing import Optional
import sys
from myapp.models import User

# GOOD - standard order
import os
import sys
from typing import Optional

from myapp.models import User
from myapp.utils import helper

6. Magic Numbers

Problem: Unclear intent, hard to maintain.

# BAD
if len(items) > 100:
    paginate()
time.sleep(3600)

# GOOD
MAX_PAGE_SIZE = 100
CACHE_TTL_SECONDS = 3600

if len(items) > MAX_PAGE_SIZE:
    paginate()
time.sleep(CACHE_TTL_SECONDS)

7. Nested Conditionals

Problem: Hard to read and maintain.

# BAD
def process(user):
    if user:
        if user.active:
            if user.verified:
                return do_work(user)
    return None

# GOOD - early returns
def process(user):
    if not user:
        return None
    if not user.active:
        return None
    if not user.verified:
        return None
    return do_work(user)

Review Questions

1. Are there any mutable default arguments (list, dict, set)? 2. Is print() used instead of logger? 3. Are f-strings used consistently? 4. Are there magic numbers that should be constants? 5. Are deeply nested conditionals flattened with early returns?

Error Handling

Critical Anti-Patterns

1. Bare Except Clause

Problem: Catches everything including KeyboardInterrupt, SystemExit.

# BAD
try:
    process()
except:
    pass

# GOOD - specific exception
try:
    process()
except ValueError as e:
    logger.error(f"Invalid value: {e}")
    raise

# ACCEPTABLE - if you must catch all
try:
    process()
except Exception as e:  # Still allows KeyboardInterrupt
    logger.error(f"Unexpected error: {e}")
    raise

2. Swallowing Exceptions

Problem: Hides errors, makes debugging impossible.

# BAD
try:
    result = risky_operation()
except Exception:
    pass  # Error silently ignored!

# GOOD - log and handle
try:
    result = risky_operation()
except OperationError as e:
    logger.warning(f"Operation failed: {e}")
    result = default_value

3. Losing Exception Context

Problem: Original stack trace lost.

# BAD - loses original traceback
try:
    parse_config()
except ValueError:
    raise ConfigError("Invalid config")

# GOOD - preserves chain
try:
    parse_config()
except ValueError as e:
    raise ConfigError("Invalid config") from e

4. Missing Context in Error Messages

Problem: Can't diagnose issue from logs.

# BAD
except KeyError:
    raise ValueError("Missing key")

# GOOD - include context
except KeyError as e:
    raise ValueError(f"Missing required key: {e.args[0]}") from e

5. Not Logging Before Re-raising

Problem: Exception might be caught elsewhere without logging.

# BAD - no record if caught upstream
try:
    process(item)
except ProcessError:
    raise

# GOOD - log before re-raising
try:
    process(item)
except ProcessError as e:
    logger.error(f"Failed to process item {item.id}: {e}")
    raise

Logging Best Practices

from loguru import logger

# BAD
print(f"Processing {item}")
print(f"Error: {e}")

# GOOD
logger.debug(f"Processing item {item.id}")
logger.info(f"Completed batch of {count} items")
logger.warning(f"Retry {attempt}/3 for {operation}")
logger.error(f"Failed to process {item.id}: {e}")

# With exception info
logger.exception(f"Unexpected error processing {item.id}")

Review Questions

1. Are there any bare except: clauses? 2. Is exception context preserved with raise ... from e? 3. Do error messages include enough context to diagnose? 4. Is logging used instead of print statements?

PEP8 Style Guide

Indentation

Rule: Use 4 spaces per indentation level. Never use tabs.

# BAD - 2 spaces
def foo():
  return bar

# BAD - tabs (shown with → for visibility)
def foo():
→   return bar  # ← actual code would have tab here

# GOOD - 4 spaces
def foo():
    return bar

Continuation Lines

# GOOD - aligned with opening delimiter
result = function_name(arg_one, arg_two,
                       arg_three, arg_four)

# GOOD - hanging indent
result = function_name(
    arg_one, arg_two,
    arg_three, arg_four,
)

# BAD - no alignment
result = function_name(arg_one, arg_two,
    arg_three, arg_four)

Line Length

Rule: Maximum 79 characters for code, 72 for docstrings/comments.

# BAD - too long
result = some_function(argument_one, argument_two, argument_three, argument_four, argument_five)

# GOOD - broken across lines
result = some_function(
    argument_one,
    argument_two,
    argument_three,
    argument_four,
    argument_five,
)

Blank Lines

Rule: Two blank lines around top-level definitions, one blank line around methods.

# GOOD
import os


class MyClass:
    """Docstring."""

    def method_one(self):
        pass

    def method_two(self):
        pass


def top_level_function():
    pass


def another_function():
    pass

Imports

Rule: Group imports in order: stdlib → third-party → local. One blank line between groups.

# GOOD
import os
import sys
from pathlib import Path

import requests
from pydantic import BaseModel

from myapp.models import User
from myapp.utils import helper

Rule: Avoid wildcard imports.

# BAD
from module import *

# GOOD
from module import specific_function, SpecificClass

Whitespace

Inside Brackets

# BAD
spam( ham[ 1 ], { eggs: 2 } )

# GOOD
spam(ham[1], {eggs: 2})

Before Colons and Commas

# BAD
if x == 4 :
    print(x , y)

# GOOD
if x == 4:
    print(x, y)

Around Operators

# BAD
x=1
y = x+1
z = x +1

# GOOD
x = 1
y = x + 1

# Exception: indicate precedence
result = x*2 + y*3

Function Arguments

# BAD
def function(arg1 = None, arg2 = 0):
    pass

# GOOD
def function(arg1=None, arg2=0):
    pass

Naming Conventions

# BAD
def MyFunction():  # should be snake_case
    pass

class my_class:  # should be CamelCase
    maxSize = 100  # should be MAX_SIZE if constant

# GOOD
def my_function():
    pass

class MyClass:
    MAX_SIZE = 100

Comments

Inline Comments

Rule: Separate by at least two spaces. Use sparingly.

# BAD
x = x + 1# increment

# GOOD
x = x + 1  # compensate for border

Block Comments

# GOOD - aligned with code, complete sentences
# This is a block comment explaining the
# following code section. Each sentence
# ends with a period.
result = complex_operation()

Docstrings

# GOOD
def fetch_users(limit: int = 100) -> list[User]:
    """Fetch users from the database.

    Args:
        limit: Maximum number of users to return.

    Returns:
        List of User objects.

    Raises:
        DatabaseError: If connection fails.
    """
    pass

Review Questions

1. Is indentation consistently 4 spaces (no tabs)? 2. Are lines ≤79 characters (≤72 for docstrings)? 3. Are there two blank lines around top-level definitions? 4. Are imports grouped correctly with blank lines between groups? 5. Is there extraneous whitespace inside brackets or around operators? 6. Do names follow conventions (snake_case, CamelCase, UPPER_CASE)? 7. Are inline comments separated by at least two spaces?

Type Safety

Critical Anti-Patterns

1. Missing Return Type

Problem: Callers don't know what to expect.

# BAD
def get_user(id: int):
    return User.query.get(id)

# GOOD
def get_user(id: int) -> User | None:
    return User.query.get(id)

2. Using Any Without Justification

Problem: Defeats the purpose of type checking.

# BAD
def process(data: Any) -> Any:
    return data

# GOOD - with justification
def process(data: Any) -> dict:  # Any: accepts JSON from external API
    return json.loads(data)

# BETTER - use proper types
def process(data: str | bytes) -> dict:
    return json.loads(data)

3. Optional vs Union Syntax

Problem: Inconsistent syntax, less readable.

# OLD (pre-3.10)
from typing import Optional, Union
def find(id: int) -> Optional[User]: ...
def parse(val: Union[str, int]) -> str: ...

# GOOD (3.10+)
def find(id: int) -> User | None: ...
def parse(val: str | int) -> str: ...

4. Missing Generic Types

Problem: Loses type information in collections.

# BAD
def get_items() -> list:
    return [Item(...)]

# GOOD
def get_items() -> list[Item]:
    return [Item(...)]

# BAD
def get_config() -> dict:
    return {"key": "value"}

# GOOD
def get_config() -> dict[str, str]:
    return {"key": "value"}

5. TypedDict for Structured Dicts

Problem: Plain dict loses key/value type information.

# BAD
def get_user_data() -> dict:
    return {"name": "Alice", "age": 30}

# GOOD
from typing import TypedDict

class UserData(TypedDict):
    name: str
    age: int

def get_user_data() -> UserData:
    return {"name": "Alice", "age": 30}

Review Questions

1. Are all function parameters typed? 2. Are all return types specified? 3. Is Any used only when necessary with a comment? 4. Are collection types generic (list[T], dict[K, V])? 5. Is T | None used instead of Optional[T]?