Exercise: Code Review Basics

ch02-03-code-review
⭐ beginner ⏱️ 20 min

You've been asked to review a colleague's MCP server code before it goes to production. The server is supposed to process user messages and return responses, but something isn't quite right.

This exercise develops a crucial skill: code review. When working with AI assistants, you'll often need to review generated code for issues. Even when you write code yourself, a critical eye catches bugs before users do.

Your task: Find at least 5 issues in this code, categorize them by severity, and suggest fixes.

🎯 Learning Objectives

Thinking

Doing

💬 Discussion

  • What's your usual approach when reviewing code?
  • What categories of issues should you look for?
  • How do you prioritize fixes?

💡 Hints

Hint 1: Where to look

Focus on these areas:

  1. How is the mutex being used?
  2. What happens with all those .unwrap() calls?
  3. Does the server actually run?
  4. What gets logged?
Hint 2: Critical issues

The most critical issues:

  • The mutex lock usage has a problem with mutable access
  • The server is built but never started with a transport
  • Multiple .unwrap() calls can panic
Hint 3: Full list

Issues to find:

  1. Critical: Mutex borrow issue - needs mut for *count += 1
  2. High: .lock().unwrap() panics if mutex poisoned
  3. High: Server never starts (no transport)
  4. High: Multiple .unwrap() calls can panic
  5. Medium: Global mutable state hurts testing/scaling
  6. Medium: Raw user input logged (security)
  7. Low: .len() > 0 should be !.is_empty()
  8. Low: Version "0.1" should be "0.1.0" for semver
  9. Low: main() should return Result
⚠️ Try the exercise first! Show Solution 
#![allow(unused)]
fn main() {
let count = MESSAGE_COUNT.lock().unwrap();
*count += 1;  // Error: count is not mutable!
}

Explanation

Fix: let mut count = MESSAGE_COUNT.lock().unwrap();

2. High - Panic on Poisoned Mutex Fix: Handle PoisonError or use lock().unwrap_or_else(|e| e.into_inner())

3. High - Server Never Starts Fix: Add server.run_stdio().await?; or HTTP transport

4. High - Unwrap on Serialization Fix: Use ? operator: Ok(serde_json::to_value(response)?)

5. Medium - Global Mutable State Fix: Use per-request or per-connection state, or Arc<Mutex<>> passed to handlers

6. Medium - Logging User Input Fix: Use structured logging (tracing), sanitize/truncate input

7. Low - Non-idiomatic Empty Check Fix: if !input.message.is_empty()

8. Low - Semver Version Format Fix: .version("0.1.0")

9. Low - main() Return Type Fix: async fn main() -> Result<(), Box<dyn std::error::Error>>

🤔 Reflection

  • What's your process for reviewing unfamiliar code?
  • How do you prioritize which issues to fix first?
  • How would you give feedback to the author without being discouraging?
  • What tools could help catch some of these issues automatically?