I've seen this exact bug dozens of times in PRs over the last decade. At first glance, the code looks clean, logical, and perfectly Pythonic. But if you merge it, you're introducing a "time bomb" that will cause unpredictable behavior in production.

def add_user_to_group(user, group=[]):
    group.append(user)
    return group

# First call works as expected
print(add_user_to_group("Alice")) # Expected: ['Alice']
# Second call... wait, what?
print(add_user_to_group("Bob"))   # Expected: ['Bob'], Actual: ['Alice', 'Bob']

The Trap of the Mutable Default

If you're reviewing this code, your first instinct might be to say, "Looks good to me." But notice that Bob is suddenly in Alice's group. This happens because in Python, default arguments are evaluated once at the time the function is defined, not every time the function is called. That list [] isn't created fresh for every call; it's a single object living in memory that persists across the entire lifecycle of the application.

The Sentinel Fix

When I see this in a review, I ask the author to use a sentinel value—usually None. This ensures that a new list is created every single time the function is executed without a provided group.

def add_user_to_group(user, group=None):
    if group is None:
        group = []
    group.append(user)
    return group

print(add_user_to_group("Alice")) # ['Alice']
print(add_user_to_group("Bob"))   # ['Bob']

Looking Beyond the Syntax

Effective code review isn't just about catching bugs like the one above; it's about ensuring the code is maintainable for the person who has to touch it six months from now (which might be you). When I review Python code, I run through a mental checklist that goes beyond whether the code "works."

• Complexity vs. Cleverness: I always flag "clever" one-liners. If a list comprehension is longer than 80 characters or nested three levels deep, I'll ask for it to be broken out into a standard for loop. Readability beats brevity every time.
• Type Hinting: If a function takes four arguments and returns a complex dictionary, I want to see type hints. It transforms the code from "I hope this is a string" to "I know this is a string," which saves everyone time during debugging.
• Resource Management: I look for files or network connections being opened without a with statement. If I see f = open(...) without a corresponding f.close() in a finally block, it's an immediate request for change.
• Naming Intent: Variables like data or temp_list are too vague. I push for names that describe the content, like validated_user_emails.

The Human Element of the Review

Finally, remember that you are reviewing a human, not a compiler. Instead of saying "This is wrong," I prefer to ask questions: "What happens here if the input is None?" or "Would this be easier to read if we split it into two functions?" It shifts the conversation from a critique to a collaborative problem-solving session. This builds a culture where people actually want their code reviewed because they know they'll come out with a better product.

📋 Practical Task

Audit a User Permission Manager

You are reviewing a pull request for a PermissionManager class. The code currently "works" in the developer's local tests, but it contains several Pythonic "smells" and a critical bug similar to the one discussed in the lesson.

Your Task: Identify and fix the three main issues in the code below:

1. The mutable default argument bug.
2. The lack of resource management (improper file handling).
3. The "clever but unreadable" list comprehension.

class PermissionManager:
    def __init__(self):
        self.log_file = "permissions.log"

    def add_permission(self, user, permissions=[]):
        # BUG: Mutable default argument
        permissions.append("admin") 
        self.log_change(f"Added admin to {user}")
        return permissions

    def log_change(self, message):
        # BUG: Unsafe file handling
        f = open(self.log_file, "a")
        f.write(message + "\n")
        # Missing f.close()

    def get_active_admins(self, user_list):
        # BUG: Overly complex/unreadable one-liner
        return [u for u in user_list if u.get('status') == 'active' and u.get('role') == 'admin' and u.get('verified') == True]

# Test the fix
pm = PermissionManager()
print(pm.add_permission("Alice"))
print(pm.add_permission("Bob"))
print(pm.get_active_admins([{'name': 'Alice', 'status': 'active', 'role': 'admin', 'verified': True}]))