Code Review Workshop

Reading and evaluating code is a skill that improves with deliberate practice. Professional engineers spend a significant portion of their workday reading other people's code — understanding it, finding problems, suggesting improvements, and documenting their reasoning. This lesson is structured practice. You will apply the frameworks from earlier in this module to review concrete examples, make explicit judgments, and propose specific improvements.

How Professional Code Review Works

In a professional setting, code review is formalized: a developer submits their changes to a system (such as GitHub's pull request flow), and one or more colleagues read the diff, leave comments, and either approve the changes or request modifications before they are merged into the main codebase. A good review comment has three parts: 1. What: the specific line, block, or pattern being flagged. 2. Why: the concrete problem it creates — a correctness risk, a security vulnerability, a maintenance burden, or a performance issue. 3. How: a specific suggestion for improvement, or at minimum a question that prompts the author to reconsider. A comment that says 'this looks weird' is not a useful review comment. A comment that says 'line 14: user_input is passed directly to execute_query without sanitization — this is an SQL injection risk. Replace with a parameterized query.' is a useful one. The goal of code review is not to assert authority or demonstrate knowledge. It is to improve the code and share knowledge with the author. The best reviewers are precise, specific, constructive, and generous in their interpretations of ambiguous code.

Sample code for review — a function that processes a user-submitted comment on a website: def save_comment(user_id, text, db): if len(text) == 0: return False query = 'INSERT INTO comments (user_id, body) VALUES (' + str(user_id) + ', \'' + text + '\')' db.execute(query) return True Applying the review framework systematically: Logic: the function returns False for empty text — reasonable. It returns True after the insert. No return value if the insert fails. If db.execute raises an exception, it propagates uncaught to the caller. Edge cases: what if text is very long? No maximum length check. What if user_id is None or not a valid user? Not checked. Security: critical finding — user_id and text are concatenated directly into a SQL string. This is SQL injection. A malicious user could supply text like: '; DROP TABLE comments; -- and destroy the comments table. Fix: parameterized query. Clarity: variable names are reasonable. The function name is descriptive. No comments, but the code is short enough not to require them. Error handling: no try/except. If the database is unavailable, the exception propagates. Depending on context, this may be fine (let the caller handle it) or problematic (should return a structured error).

Writing Review Comments That Improve Code

For the function above, a professional review comment for the SQL injection finding might read: 'Line 3-4: SQL injection vulnerability. user_id and text are string-concatenated into the query, allowing an attacker to supply a crafted value that modifies or destroys data. Replace with a parameterized query: db.execute('INSERT INTO comments (user_id, body) VALUES (?, ?)', (user_id, text)) This eliminates the entire class of vulnerability without changing behavior for valid inputs.' Notice the structure: what (line reference, specific problem), why (allows attacker to modify/destroy data), how (exact replacement code). This is actionable — the author can apply the fix immediately. For the missing maximum length check, a comment might read: 'Line 2: only the empty-string case is checked. A comment with 50,000 characters could exceed column limits or degrade page rendering. Consider adding a maximum length check, e.g., if len(text) > 1000: return False, and document the chosen limit.'