/code-review

name: "Delphi Code Review"
description: "Delphi code review checklist — quality, security, performance, SOLID, memory"

Delphi Code Review — Skill

Quick Checklist

Corretude

• [ ] Code does what it's supposed to do
• [ ] Edge cases handled (nil, empty list, zero value)
• [ ] Error handling implemented with specific exceptions
• [ ] No obvious bugs

Security

• [ ] Parameterized SQL queries (without string concatenation)
• [ ] Validated and sanitized input
• [ ] No hardcoded credentials or passwords
• [ ] No SQL injection via `Format` or concatenation in queries

Performance

• [ ] No N+1 queries (avoid loop with query inside)
• [ ] No unnecessary loops
• [ ] Large objects released as early as possible
• [ ] `TObjectList` with `OwnsObjects` configured correctly

Code Quality

• [ ] Self-descriptive names following Pascal Guide
• [ ] DRY — no duplicate code
• [ ] SOLID — principles respected
• [ ] Methods ≤ 20 lines
• [ ] Guard clauses instead of deep nesting

Memory Management

• [ ] `try/finally` with `Free` for temporary objects
• [ ] Interfaces for automatic reference counting
• [ ] `Assigned()` before accessing references that may be nil
• [ ] Destructor `Destroy` with `override` freeing owned fields
• [ ] No memory leaks in exception paths

Pascal Nomenclature

• [ ] PascalCase for all identifiers
• [ ] Prefix `T` in classes, `I` in interfaces, `E` in exceptions
• [ ] Prefix `F` in private fields, `A` in parameters, `L` in local variables
• [ ] Units: `Projeto.Camada.Dominio.Funcionalidade.pas`
• [ ] Components: 3-letter prefix (`btn`, `edt`, `lbl`, etc.)

Tests

• [ ] Unit tests for new code
• [ ] Edge cases tested
• [ ] Readable and maintainable tests

Documentation

• [ ] XMLDoc for public methods and properties
• [ ] Comments in Portuguese when necessary
• [ ] Do not comment self-explanatory code

Anti-Patterns to Flag

//❌ Magic numbers
if ACustomer.Age > 18 then

//✅ Named constants
const MINIMUM_AGE = 18;
if ACustomer.Age > MINIMUM_AGE then

//❌ with statement
with AQuery do begin
  SQL.Text := '...';
  Open;
end;

//✅ Explicit reference
AQuery.SQL.Text := '...';
AQuery.Open;

//❌ Generic Catch
except
  on E: Exception do ShowMessage(E.Message);

//✅ Specific exceptions
except
  on E: EFDDBEngineException do
    raise EDatabaseException.Create('Falha: ' + E.Message);

//❌ Logic in OnClick
procedure TfrmMain.btnSaveClick(Sender: TObject);
begin
  //50 lines of business logic here
end;

//✅ Delegate for Service
procedure TfrmMain.btnSaveClick(Sender: TObject);
begin
  FService.SaveCustomer(GetFormData);
end;

// ❌ Memory leak
function GetItems: TStringList;
begin
  Result := TStringList.Create;
  LoadItems(Result); //if LoadItems throws exception, leak!
end;

//✅ Safe
function GetItems: TStringList;
begin
  Result := TStringList.Create;
  try
    LoadItems(Result);
  except
    Result.Free;
    raise;
  end;
end;

Review Comments Guide

🔴 BLOQUEANTE: Memory leak — objeto não liberado em caso de exception
🔴 BLOQUEANTE: SQL injection — query usando concatenação de string

🟡 SUGESTÃO: Extrair método — este bloco tem 35 linhas
🟡 SUGESTÃO: Usar interface em vez de classe concreta (DIP)

🟢 NIT: Renomear variável 'S' para nome descritivo
🟢 NIT: Preferir guard clause a nesting

❓ PERGUNTA: O que acontece se ACustomer for nil aqui?
❓ PERGUNTA: Este objeto é liberado por quem?

Specific SOLID Checklist

| Principle | Check | |-----------|-----------| | **SRP** | Does class have ONE responsibility? Service does not access data? | | **OCP** | Do new features add classes, not modify existing ones? | | **LSP** | Does either implementation of the interface work in place of the other? | | **ISP** | Doesn't interface have methods that implementers don't use? | | **DIP** | Constructor takes interfaces, not concrete classes? |