code-review

What this skill does

# Code Review Best Practices

This skill activates when reviewing code for quality, correctness, security, and maintainability.

## When to Use This Skill

Activate when:
- Reviewing pull requests
- Conducting code audits
- Providing feedback on code quality
- Identifying security vulnerabilities
- Suggesting refactoring improvements
- Checking adherence to coding standards

## Code Review Checklist

### 1. Correctness and Functionality

**Does the code do what it's supposed to do?**

- Logic is correct and handles all cases
- Edge cases are considered
- Error handling is appropriate
- No obvious bugs or logical errors
- Assertions and validations are present
- Return values are correct

**Questions to ask:**
- What happens if this receives null/nil?
- What if the list is empty?
- What if the number is negative/zero?
- Are there off-by-one errors?
- Are comparisons correct (>, >=, <, <=)?

### 2. Security

**Is the code secure?**

- No SQL injection vulnerabilities
- No XSS (Cross-Site Scripting) vulnerabilities
- No CSRF vulnerabilities (CSRF protection in place)
- User input is validated and sanitized
- Sensitive data is not logged
- Authentication and authorization are properly implemented
- No hardcoded secrets or credentials
- File uploads are validated (type, size, content)
- External URLs are validated
- Rate limiting is in place for APIs

**Common security issues:**

```elixir
# BAD: SQL injection vulnerability
query = "SELECT * FROM users WHERE id = #{user_id}"

# GOOD: Use parameterized queries
query = from u in User, where: u.id == ^user_id

# BAD: XSS vulnerability
raw("<div>#{user_input}</div>")

# GOOD: Escape user input
<div><%= user_input %></div>

# BAD: Hardcoded secrets
api_key = "sk_live_123456789"

# GOOD: Use environment variables
api_key = System.get_env("API_KEY")

# BAD: Mass assignment vulnerability
User.changeset(%User{}, params)

# GOOD: Whitelist allowed fields
User.changeset(%User{}, params)
# Where changeset only casts allowed fields:
# cast(user, attrs, [:name, :email])
```

### 3. Performance

**Is the code efficient?**

- No N+1 query problems
- Appropriate data structures chosen
- Algorithms are efficient
- Database indexes are used
- Caching is implemented where appropriate
- Large datasets are paginated or streamed
- Unnecessary computations are avoided
- Resources are cleaned up properly

**Common performance issues:**

```elixir
# BAD: N+1 query
posts = Repo.all(Post)
Enum.map(posts, fn post ->
  author = Repo.get(User, post.author_id)  # Query for each post!
  {post, author}
end)

# GOOD: Preload associations
posts = Post |> preload(:author) |> Repo.all()

# BAD: Loading entire dataset
users = Repo.all(User)  # Loads all millions of users
Enum.filter(users, & &1.active)

# GOOD: Query in database
users = User |> where(active: true) |> Repo.all()

# BAD: Inefficient data structure
list = [1, 2, 3, 4, 5]
if 3 in list do  # O(n) lookup in list
  # ...
end

# GOOD: Use set/map for lookups
set = MapSet.new([1, 2, 3, 4, 5])
if MapSet.member?(set, 3) do  # O(1) lookup
  # ...
end
```

### 4. Code Quality and Maintainability

**Is the code readable and maintainable?**

- Clear, descriptive variable and function names
- Functions are small and focused (single responsibility)
- No code duplication (DRY principle)
- Comments explain "why", not "what"
- Code follows project conventions and style guide
- Magic numbers are replaced with named constants
- Complexity is minimized
- Code is self-documenting

**Code quality issues:**

```elixir
# BAD: Unclear names
def calc(x, y, z) do
  r = x * y / z
  r * 1.2
end

# GOOD: Clear names
def calculate_discounted_price(quantity, unit_price, discount_percentage) do
  subtotal = quantity * unit_price
  discount_amount = subtotal * (discount_percentage / 100)
  subtotal - discount_amount
end

# BAD: Long function with multiple responsibilities
def process_order(order) do
  # Validate order (responsibility 1)
  # Calculate totals (responsibility 2)
  # Update inventory (responsibility 3)
  # Send email (responsibility 4)
  # Log analytics (responsibility 5)
end

# GOOD: Single responsibility functions
def process_order(order) do
  with {:ok, order} <- validate_order(order),
       {:ok, order} <- calculate_totals(order),
       {:ok, order} <- update_inventory(order),
       :ok <- send_confirmation_email(order),
       :ok <- log_order_analytics(order) do
    {:ok, order}
  end
end

# BAD: Magic numbers
if user.age >= 13 do
  # ...
end

# GOOD: Named constants
@minimum_age_coppa 13

if user.age >= @minimum_age_coppa do
  # ...
end
```

### 5. Error Handling

**Are errors handled properly?**

- Errors don't crash the system unexpectedly
- Error messages are helpful
- Errors are logged appropriately
- Happy path and error paths are both tested
- No swallowed errors (empty catch blocks)
- Proper error types are used

**Error handling patterns:**

```elixir
# BAD: Silent failure
try do
  dangerous_operation()
rescue
  _ -> nil  # Error is swallowed!
end

# GOOD: Handle errors explicitly
case dangerous_operation() do
  {:ok, result} -> result
  {:error, reason} ->
    Logger.error("Operation failed: #{inspect(reason)}")
    {:error, reason}
end

# BAD: Generic error message
{:error, "failed"}

# GOOD: Specific error
{:error, :invalid_email_format}
{:error, {:validation_failed, errors}}

# BAD: Let it crash when shouldn't
def parse_config(path) do
  File.read!(path)  # Crashes if file missing
  |> Jason.decode!()  # Crashes if invalid JSON
end

# GOOD: Handle expected errors
def parse_config(path) do
  with {:ok, content} <- File.read(path),
       {:ok, config} <- Jason.decode(content) do
    {:ok, config}
  else
    {:error, :enoent} -> {:error, :config_file_not_found}
    {:error, %Jason.DecodeError{}} -> {:error, :invalid_config_format}
  end
end
```

### 6. Testing

**Is the code properly tested?**

- New functionality has tests
- Edge cases are tested
- Error conditions are tested
- Tests are clear and focused
- Tests are deterministic (no flaky tests)
- Test names describe what they test
- Mocks are used appropriately
- Test coverage is adequate

**Testing concerns:**

```elixir
# BAD: Unclear test name
test "test1" do
  # ...
end

# GOOD: Descriptive test name
test "create_user/1 returns error when email is invalid" do
  # ...
end

# BAD: Testing too much at once
test "user workflow" do
  # Creates user
  # Updates user
  # Deletes user
  # All in one test!
end

# GOOD: Focused tests
test "create_user/1 creates user with valid attributes" do
  # ...
end

test "update_user/2 updates user name" do
  # ...
end

test "delete_user/1 removes user from database" do
  # ...
end

# BAD: Non-deterministic test
test "async operation completes" do
  start_async_operation()
  Process.sleep(100)  # Race condition!
  assert operation_completed?()
end

# GOOD: Deterministic test
test "async operation completes" do
  start_async_operation()
  assert_receive {:completed, _result}, 1000
end
```

### 7. Documentation

**Is the code documented?**

- Public APIs have documentation
- Complex logic has explanatory comments
- README is updated if needed
- Changelog is updated for user-facing changes
- API documentation is accurate
- Examples are provided

### 8. Dependencies

**Are dependencies handled properly?**

- New dependencies are justified
- Dependencies are up-to-date and maintained
- Licenses are compatible with project
- Security vulnerabilities are checked
- Dependency versions are pinned or bounded

## Review Process

### Before Reviewing

1. **Understand the context**
   - Read the PR description
   - Understand the problem being solved
   - Check related issues

2. **Build and test locally**
   - Pull the branch
   - Run tests
   - Test the functionality manually

### During Review

1. **Start with the big picture**
   - Is the approach sound?
   - Does it fit the architecture?
   - Is there a better way?

2. **Review for correctness**
   - Does it work as intended?
   - Are edge cases han