mql5-code-review

Files (12)

• SKILL.md
• review-checklist.md · assets/
• review-report-template.md · assets/
• common-bugs-checklist.md · references/
• diagnostics-and-numeric.md · references/
• failure-modes.md · references/
• gui-ux-review.md · references/
• indicator-review.md · references/
• performance-review.md · references/
• tester-live-parity.md · references/
• trading-logic-review.md · references/
• mql5_redflags.py · scripts/

SKILL.md (first lines)

# MQL5 Code Review Skill

Find what a backtest will NOT tell you — by reading the source. This skill
reviews MQL5 EA code **statically**: failure modes that are rare or
account-state-dependent, inefficiencies that only hurt after weeks of
live run, silent numeric errors, and diagnosability gaps that make the
real incident untraceable. Runtime validation (backtests, optimization,
deal-level debugging) is the domain of the `mql5` skill — use this
skill first to shrink the search space before you burn tester time.

## 1. Purpose and Scope

**In scope** (static analysis of source):
- Correctness of trading logic, risk formulas, indicator/MTF semantics
- Failure modes: restart, weekend gaps, broker differences, event races
- Inefficiency: per-tick hot path, object churn, string/array growth
- Diagnosability: silent failures, log context, input validation
- GUI/UX: panel overflow, anchoring, update throttling, cleanup
- Readability: naming (vs `../mql5/references/quick-ref-code-style.md`),
  dead code, magic numbers, structure

**Out of scope** (hand off, do not do here):
- Running backtests / optimizations / report analysis → `mql5` skill
  (`mql5_helper.py tester`, `parse_tester_report.py`)
- Compiling as the primary verification → optional cross-check only
  (`mql5` skill's `mql5_helper.py check`)
- Live trading, signal monitoring → `mql-signals` skill

## 2. Review Mindset

**Goals**: catch failure modes before they cost money; share the "why"
behind each finding; leave the code more understandable.

**Not the goals**: showing off, blocking on style alone (label style as
🟢), rewriting to personal preference, reviewing what linters/compile
warnings already catch (report only the aggregate).

**Feedback rules**:
- Specific and actionable: name the file:line, the failure scenario, the fix
- Focused on the code, not the author — third-party/article code gets the
  same treatment as own code, but state assumptions explicitly
- Balanced: list strengths too (see report template)
- Prioritized: 🔴 blocking findings first, always with a concrete failure
  scenario ("what happens when…")

## 3. Severity Labels

| Label | Meaning | Action |
|-------|---------|--------|
| 🔴 `[blocking]` | Will fail, loses money, or is untraceable in a realistic scenario | Must fix before live |
| 🟡 `[important]` | Degraded behaviour / portability / diagnosability gap | Should fix, discuss if disagree |
| 🟢 `[nit]` | Readability, style, minor cleanup | Non-blocking |
| 💡 `[suggestion]` | Alternative approach worth considering | Optional |
| 📚 `[learning]` | Educational note, no action needed | — |
| 🎉 `[praise]` | Genuinely good pattern — say so | — |

A backtest that already passes does NOT downgrade a 🔴: the failure may
be exactly the state the backtest did not reach (restart, gap, stop-out,
broker change). See `references/failure-modes.md`.

## 4. Review Process (4 Phases)

### Phase 1 — Context (2–3 min)

Answer before reading code:

1. **Program type** — EA (`OnTick`)? This skill assumes EA; adapt for
   Indicator/Script/Service.
2. **Origin** — own EA / third-party / mql5.com article / signal vendor?
   For third-party code, assume no documentation: document your
   assumptions in the report.
3. **Account & symbols** — hedging or netting? single- or multi-symbol?
   Account currency vs symbol profit currency?
4. **State model** — what must survive an EA restart (position tracking,
   counters, pending-order state machine)?
5. **GUI** — does it draw panels/objects? Which notifications (Alert /
   push / e-mail) does it emit?
6. **Size triage** — >400 changed lines: review in slices (per file or
   per subsystem); do not skim.

### Phase 2 — High-Level (5–10 min)

1. **Architecture**: class split consistent with the project convention
   (`C`+PascalCase classes, `m_` members — baseline:
   `../mql5/references/quick-ref-code-style.md`)? Signal generation /
   risk sizing / execution / reporting separated, or a 2000-line OnTick?
2. **Risk model**: which of the three formulas (A: points→SL, B: risk%+lots→SL,
   C: risk%+SL→lots) — and is it applied consistently?
   `../mql5/SKILL.md` §5.
3. **Event model**: new-bar gate in OnTick? Timer? Reentrancy guard for
   trade functions shared by OnTick/OnTimer/OnTradeTransaction?
4. **Restart model**: is there a re-adoption path
   (`PositionsTotal()` + magic scan) or does state start from zero?
5. **GUI update model**: change-driven vs per-tick, timer-driven display?
   → `references/gui-ux-review.md`.

### Phase 3 — Line-by-Line (10–30 min)

Work through the dimension table in §6, one dimension at a time. Run
the scanner (§5) first and fold its findings in — but never copy them
blindly: the scanner is regex-level, it produces both false positives
(needs human dismissal) and false negatives (needs human eyes).

### Phase 4 — Summary and Verdict (2–3 min)

1. Group findings by severity; every 🔴 gets a concrete failure scenario.
2. List strengths.
3. Verdict: ✅ Approve / 💬 Comment / 🔄 Request Changes (any 🔴 ⇒ 🔄).
4. Copy into `assets/review-report-template.md` and deliver.

## 5. Automated Pre-Scan — mql5_redflags.py

Stdlib-only static scanner. Run it first, then review each finding
humanly:

```
python skills/mql5-code-review/scripts/mql5_redflags.py scan FILE.mq5 [MORE.mq5 ...]
python skills/mql5-code-review/scripts/mql5_redflags.py scan DIR/ --json -o findings.json
python skills/mql5-code-review/scripts/mql5_redflags.py --list-rules

From ZhijuCen/mql5-skills, MIT.

More from ZhijuCen/mql5-skills