moonbit-code-review

name: moonbit-code-review description: Use when reviewing MoonBit code changes. Scores 0-100 with deductions per category.

MoonBit Code Review

Overview

Score every code review from 100 → 0. Start at 100 and apply deductions.

Issues detectable by the MoonBit compiler or moon check are NOT scored here.

Scoring Summary

Architecture

Bidirectional Dependency (-20 per occurrence)

Packages must depend in one direction only. Types-only packages are allowed as shared dependencies.

// ❌ BAD: bidirectional — order imports payment, payment imports order
// order/order.mbt
fn create_order(p : @payment.Processor) -> Order { ... }

// payment/payment.mbt
fn charge(o : @order.Order) -> Unit { ... }

// ✅ GOOD: unidirectional with shared types
// types/types.mbt
pub struct Order {
  id : String
  amount : Int
}

// payment/payment.mbt — depends on types only
pub fn charge(o : @types.Order) -> Result[Unit, PaymentError] { ... }

// order/order.mbt — depends on types and payment
pub fn place(o : @types.Order) -> Result[Unit, Error] {
  @payment.charge(o)
}

Duplicated Logic (-10 per occurrence)

Extract shared behavior into a function.

// ❌ BAD: duplicated validation
pub fn create_user(name : String) -> Result[Unit, AppError] {
  if name.length() == 0 || name.length() > 100 {
    return Err(AppError::InvalidName)
  }
  ...
}

pub fn update_user(name : String) -> Result[Unit, AppError] {
  if name.length() == 0 || name.length() > 100 {
    return Err(AppError::InvalidName)
  }
  ...
}

// ✅ GOOD: shared validation
fn validate_name(name : String) -> Result[Unit, AppError] {
  if name.length() == 0 || name.length() > 100 {
    return Err(AppError::InvalidName)
  }
  Ok(())
}

pub fn create_user(name : String) -> Result[Unit, AppError] {
  validate_name(name)!
  ...
}

pub fn update_user(name : String) -> Result[Unit, AppError] {
  validate_name(name)!
  ...
}

Project Design Violation (-10 per occurrence)

Code must follow the architecture described in the project's AGENTS.md. Check package boundaries, naming conventions, and responsibilities.

Unit Testing

Non-Refactoring-Tolerant Test (-3 per test case)

Tests must assert results, not how the code works internally.

// ❌ BAD: asserts internal call behavior
test "transfer calls debit and credit" {
  let mock = MockRepo::new()
  let svc = Service::new(mock)
  svc.transfer("alice", "bob", 100)
  // Asserting internal calls = breaks on refactor
  assert_eq!(mock.debit_called_with, ("alice", 100))
  assert_eq!(mock.credit_called_with, ("bob", 100))
}

// ✅ GOOD: asserts observable outcome
test "transfer moves balance between accounts" {
  let repo = InMemoryRepo::new()
  repo.set_balance("alice", 200)
  repo.set_balance("bob", 50)
  let svc = Service::new(repo)

  svc.transfer("alice", "bob", 100) |> ignore

  assert_eq!(repo.get_balance("alice"), 100)
  assert_eq!(repo.get_balance("bob"), 150)
}

WhiteBox vs BlackBox Test File (-3 per test case / per file)

MoonBit has explicit file conventions:

• *_test.mbt → BlackBox test (only accesses pub members) ✅
• *_wbtest.mbt → WhiteBox test (accesses all members)

Always use BlackBox tests (_test.mbt). WhiteBox tests are a deduction.

// ❌ BAD: WhiteBox test accessing private internals
// wallet_wbtest.mbt
test "internal ledger" {
  let w = { ledger: [] }   // accessing private field
  add_entry(w, 100)         // calling private function
  assert_eq!(w.ledger.length(), 1)
}

// ✅ GOOD: BlackBox test using public API
// wallet_test.mbt
test "deposit increases balance" {
  let w = @wallet.new()

  @wallet.deposit(w, 100) |> ignore

  assert_eq!(@wallet.balance(w), 100)
}

Uncovered Code (-1 per line)

Every line of production code should be exercised by tests. Run:

moon test --coverage

Security

Review security against the project context (README.md / AGENTS.md).

// ❌ HIGH: string-concatenated query
pub fn get_user(db : Database, id : String) -> Result[User, DbError] {
  db.query("SELECT * FROM users WHERE id = '" + id + "'")
}

// ✅ GOOD: parameterized query
pub fn get_user(db : Database, id : String) -> Result[User, DbError] {
  db.query_with_params("SELECT * FROM users WHERE id = $1", [id])
}

Specification

Spec Not Met (-100)

If the code does not satisfy the specification, the review score is effectively 0. Verify behavior against the requirements before anything else.

MoonBit Best Practices

Ignoring Result / Error (-5 per occurrence)

// ❌ BAD: silently ignoring error
fn process(data : String) -> Unit {
  let _ = parse(data)  // error discarded
}

// ✅ GOOD: propagate or handle
fn process(data : String) -> Result[Unit, ParseError] {
  let parsed = parse(data)!
  do_something(parsed)
  Ok(())
}

Not Using Pattern Matching (-3 per occurrence)

// ❌ BAD: manual if-else chain on enum
fn describe(shape : Shape) -> String {
  if shape.is_circle() {
    "circle"
  } else if shape.is_rect() {
    "rectangle"
  } else {
    "unknown"
  }
}

// ✅ GOOD: exhaustive pattern matching
fn describe(shape : Shape) -> String {
  match shape {
    Circle(_) => "circle"
    Rect(_) => "rectangle"
  }
}

Not Using Pipeline Operator (-2 per occurrence)

// ❌ BAD: deeply nested function calls
fn process(input : String) -> String {
  trim(to_lowercase(remove_spaces(input)))
}

// ✅ GOOD: pipeline for readability
fn process(input : String) -> String {
  input
  |> remove_spaces
  |> to_lowercase
  |> trim
}

Stringly-Typed API (-3 per occurrence)

// ❌ BAD: status as string
pub fn set_status(order : Order, status : String) -> Unit {
  order.status = status
}

// ✅ GOOD: enum for known variants
pub enum OrderStatus {
  Pending
  Shipped
  Delivered
}

pub fn set_status(order : Order, status : OrderStatus) -> Unit {
  order.status = status
}

Not Using Snapshot Testing (-2 per occurrence)

When verifying complex output, prefer inspect over manual assertions.

// ❌ BAD: fragile manual assertion on complex structure
test "parse config" {
  let config = parse("key=value\nfoo=bar")
  assert_eq!(config.entries.length(), 2)
  assert_eq!(config.entries[0].key, "key")
  assert_eq!(config.entries[0].value, "value")
  assert_eq!(config.entries[1].key, "foo")
  assert_eq!(config.entries[1].value, "bar")
}

// ✅ GOOD: snapshot testing — auto-updated with `moon test --update`
test "parse config" {
  let config = parse("key=value\nfoo=bar")
  inspect!(config, content="{entries: [{key: \"key\", value: \"value\"}, {key: \"foo\", value: \"bar\"}]}")
}

Trait Interface Design (-5 per occurrence)

Keep traits small and focused. Define them where they are consumed.

// ❌ BAD: large trait with unrelated methods
pub trait Storage {
  get(Self, String) -> Option[Bytes]
  set(Self, String, Bytes) -> Unit
  delete(Self, String) -> Unit
  list(Self, String) -> Array[String]
  backup(Self) -> Unit
  restore(Self, String) -> Unit
}

// ✅ GOOD: small, focused traits
pub trait ItemReader {
  get(Self, String) -> Option[Bytes]
}

pub trait ItemWriter {
  set(Self, String, Bytes) -> Unit
}

Mutable When Immutable Suffices (-2 per occurrence)

// ❌ BAD: using mutable ref when not needed
fn total(items : Array[Item]) -> Int {
  let mut sum = 0
  for item in items {
    sum = sum + item.price
  }
  sum
}

// ✅ GOOD: functional approach
fn total(items : Array[Item]) -> Int {
  items.fold(init=0, fn(acc, item) { acc + item.price })
}

Review Procedure

1. Spec — Does the code meet the requirements? If not, stop (-100).
2. Architecture — Check dependency direction, duplication, project design.
3. Unit Tests — BlackBox (_test.mbt)? Refactoring-tolerant? Snapshot tests? Coverage?
4. Security — Review against project context for vulnerabilities.
5. MoonBit Best Practices — Error handling, pattern matching, pipelines, traits, immutability.
6. Calculate Score — Start at 100, apply all deductions. Minimum is 0.

Output Format

## Code Review: [component/file]

**Score: XX/100**

### Deductions

| # | Category | Detail | Deduction |
|---|----------|--------|-----------|
| 1 | Architecture | Bidirectional dep: order ↔ payment | -20 |
| 2 | Unit Test | WhiteBox test: wallet_wbtest.mbt | -3 |
| ... | ... | ... | ... |

**Total Deductions: -XX**

### Summary
[brief summary of key issues and recommendations]