Code review

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

// ❌ Números mágicos
if ACustomer.Age > 18 then

// ✅ Constantes nomeadas
const MINIMUM_AGE = 18;
if ACustomer.Age > MINIMUM_AGE then

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

// ✅ Referência explícita
AQuery.SQL.Text := '...';
AQuery.Open;

// ❌ Catch genérico
except
  on E: Exception do ShowMessage(E.Message);

// ✅ Exceptions específicas
except
  on E: EFDDBEngineException do
    raise EDatabaseException.Create('Falha: ' + E.Message);

// ❌ Logic em OnClick
procedure TfrmMain.btnSaveClick(Sender: TObject);
begin
  // 50 linhas de logic de negócio aqui
end;

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

// ❌ Memory leak
function GetItems: TStringList;
begin
  Result := TStringList.Create;
  LoadItems(Result); // se LoadItems lançar exception, leak!
end;

// ✅ Seguro
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?