Bugless design

Uno degli aspetti più dolorosi nello scrivere software è dover risolvere i bug. Se i bug non esistessero, chiunque sarebbe in grado di programmare rapidamente ed efficientemente. Un programma che all’apparenza fa tutto quello che deve fare, ma in cui nessuno si è mai curato di fare dei test o risolvere dei problemi, può sembrare di primo acchito completo al 90%, ma la realtà è che il 90% è più probabilmente la quantità di lavoro necessario a completarlo. Diffidate da progetti che sembrano quasi finiti ma su cui non è stato eseguito un accurato testing.

Per queste ragioni, scrivere software con un numero minore possibile di bug è estremamente importante, sia per ottimizzare i nostri tempi di sviluppo e non passare interminabili ore a cercare un problema introvabile, sia per evitare che ci siano problemi in un programma messo in produzione, con tutti i guai che ne conseguono. Il problema è che la pretesa di scrivere codice senza bug è pari a quella di non sbagliare mai: non è realizzabile. Con più attenzione e concentrazione si possono ridurre solo in modo limitato gli errori, ed in una giornata lavorativa ci saranno inevitabilmente dei momenti di calo e deconcentrazione. Come possiamo quindi lottare contro questo insidioso problema?

La filosofia che esporrò in questo articolo è la seguente:

Il problema non è il bug, ma il codice scadente che ci sta intorno

In altre parole, l’obiettivo non è di cercare di non sbagliare mai, ma cercare di scrivere del codice più possibile pulito e chiaro, che renda difficile sbagliarsi quando si utilizza o modifica quel codice. Spesso si parla di “Clean code” per programmi con queste qualità, e molti sono i libri e le risorse (oltre ad articoli su questo portale) che descrivono i principi con cui realizzarlo. L’obiettivo è quindi creare codice che sia progettato per limitare la quantità di bug che si possono generare utilizzandolo. Rendendo il codice pulito, chiaro e lineare, la possibilità di fare errori scende drasticamente.

Per completezza, occorre dire che ci sono moltissime altre tecniche per limitare i bug di un programma. James Shore, autore di “The Art of Agile Development”, elenca tutta una serie di tecniche che se usate in congiunzione ci possono portare a scrivere programmi senza bug – oltre alla scrittura di buon codice, abbiamo il pair programming, il testing automatizzato e test-driven development, l’evitamento di orari di lavoro troppo lunghi, ed il dedicare del tempo per la riduzione del debito tecnico. Sono tutte ottime tecniche, ma, secondo me, il primo fattore da considerare è avere un codice pulito, ben scritto e facilmente riutilizzabile.

Spesso il modo migliore per imparare è partendo dall’esperienza. In questo articolo, partiremo quindi da frammenti di codice realistico, simili a quelli incontrati nel mio lavoro giornaliero. Per ognuno di questi, scopriremo il perché possa nascondere bug, o rendere il codice in altre parti dell’applicazione più vulnerabile ad errori e cattive interpretazioni. Faremo riferimento inoltre ai principi di Clean Code che, se utilizzati, avrebbero evitato questi problemi. Gli esempi riportati sono scritti in C#, ma i principi discussi valgono in qualsiasi linguaggio di programmazione.

Un po’ di codice da migliorare

Iniziamo da un frammento di codice che più mi ha generato problemi nel tempo:

public OrderDto CreateOrder(int productId, int userId)
{
    try
    {
        var order = new Order();
        // Several database operations
        return _mapper.Map(order);
    }
    catch (Exception e)
    {
        _logger.Error(e, "There was an error creating the order");
        return null;                
    }
}

Quale è il problema di questo codice?

È lodevole che in caso di errore venga loggata un’eccezione, a seconda di come è strutturata l’applicazione potrebbe essere giusto loggarla qua o in qualche metodo chiamante. Tuttavia, in caso di errore il flusso del programma continua. Certo, il chiamante potrebbe controllare il valore di ritorno – ma se il metodo fosse utilizzato in molti punti del codice dovremmo duplicare questo controllo, per verificare un caso di errore sulla creazione di un ordine, responsabilità di questo metodo. Perché imporre ad ogni chiamante di ricordarsi questo controllo, quando possiamo interrompere subito l’esecuzione? Questo è un tipico problema di codice a rischio.

In alcuni casi particolarmente negativi, può anche succedere che l’esecuzione non si interrompa affatto, e che lo stato del programma contenga valori inconsistenti. Un utente potrebbe richiedere la creazione di un ordine, e ricevere una pagina di conferma senza che l’ordine sia stato veramente creato. Questa è una delle situazioni più pericolose che si possono creare, perché mina la fiducia nell’applicazione: quando lo stesso utente effettua un’altra operazione, si chiederà se l’operazione è stata eseguita oppure no. In un certo senso, questo codice nasconde i problemi sotto al tappeto: al momento può sembrare che l’operazione abbia avuto successo quando ciò non è vero.

La soluzione rientra nel principio chiamato “fail-fast”: se qualcosa va storto, far emergere il problema prima possibile, nel punto del codice dove il problema si è verificato. Tecnicamente, in questo caso significa rilanciare immediatamente l’eccezione.

In generale, ritornare oggetti uguali a null è sconsigliabile, perché implica un controllo da parte di tutti i chiamanti. Se ci sono dei casi dove effettivamente non può essere ritornato un oggetto, specificarlo nella firma, in modo da essere chiari con i chiamanti. Pensate per esempio ad un metodo GetLatestOrder(int userId) per ottenere l’ultimo ordine di un utente: se vogliamo restituire null per utenti che non hanno ordini, potremmo almeno chiamare il metodo GetLatestOrderIfAny, in modo da ricordare ai chiamanti che un ordine potrebbe non essere restituito.

Consideriamo l’implementazione di un sistema di ticketing; abbiamo il seguente metodo

public ActionResult Close(int ticketId, 
                          string messageToUser, 
                          string internalMessage, 
                          string supMessage) {
    //...
    _service.CloseTicket(ticketId, userId, messageToUser, internalMessage, supMessage);
    return RedirectToAction("TicketClosed", new {ticketId});
}

Altrove:

ppublic void CloseTicket(
    int ticketId,
    int userId,            
    string internalNote,
    string messageToSupervisor,
    string messageToUser)
{
    // ...
}

In questo contesto, pare che abbiamo tre note da specificare alla chiusura di un ticket; una per l’utente, una nota interna per i colleghi, ed una per il supervisore. Il problema è che nella chiamata al nostro servizio, abbiamo scambiato l’ordine di queste stringhe, inoltrando all’utente una nota interna destinata a rimanere segreta. Se questo codice andasse in produzione, avremmo sicuramente dei problemi.

Secondo la filosofia di questo articolo, per quanto il bug sia nel codice chiamante, il problema principale è nel modo in cui è scritto il metodo CloseTicket – con tre stringhe di seguito nella firma, è molto facile scambiarle, ed in effetti guardando il codice chiamante il bug non è per nulla evidente. Rifattorizziamo il codice senza correggere il bug:

public classpublic class TicketCloseRequest
{
    public int TicketId { get; set; }
    public string MessageToUser { get; set; }
    public string InternalMessage { get; set; }
    public string MessageToSupervisor { get; set; }
}

public void CloseTicket(TicketCloseRequest request, int userId)
{
    // ...
}

public ActionResult Close(int ticketId, 
                          string messageToUser, 
                          string internalMessage, 
                          string supervisorMessage)
{
    //...
    _service.CloseTicket(new TicketCloseRequest()
    {
        TicketId = ticketId,
        InternalMessage = messageToUser,
        MessageToSupervisor = internalMessage,
        MessageToUser = supervisorMessage
    }, userId);
    return RedirectToAction("TicketClosed", new {ticketId});
}

Raggruppando i parametri in un oggetto, diventa lampante che stiamo facendo delle assegnazioni sbagliate: diventa impossibile scambiare accidentalmente una stringa per un’altra. Ci è di aiuto anche dare dei nomi parlanti alle variabili: se in ingresso chiamassimo i messaggi message1, message2 e message3 il problema non sarebbe risolto.

Questa tecnica è legata al principio di passare pochi parametri agli oggetti, raggruppandoli in oggetti soprattutto quando sono unibili sotto un’unica entità logica. Io raccomando di non abusare di questo metodo: se per ogni metodo che scriviamo dobbiamo creare una nuova classe, abbiamo un a proliferazione di oggetti che può creare grande confusione. Tuttavia, è spesso raccomandabile quando un metodo altrimenti accetterebbe diversi parametri dello stesso tipo che possono essere confusi tra loro, come nell’esempio. Può essere di aiuto per evitare questa situazione anche la tecnica descritta nel prossimo esempio.

Immaginiamo di implementare una piattaforma di e-commerce internazionale contenente il seguente codice:

public decimal ComputeOrderCost(Order order)
{
    decimal cost = GetShippingCosts(order);
    foreach (var product in order.Products)
    {
        cost += product.Price;
    }
    return cost;
}

Dove è il problema con questo codice? Potrebbe essere tutto ok, ma potrebbe anche nascondere un bug difficile da scorgere se i prodotti fossero definiti dalla seguente classe

public class Product
{
    //...
    public decimal Price { get; set; }
    public Currency Currency { get; set; }
}

Se il prezzo dei prodotti è anche definito in una certa valuta, allora sommando i prezzi rischiamo di sommare valori in valute diverse, ottenendo un totale privo di senso. Il fatto che per verificare la correttezza del computo dobbiamo guardare un’altra classe ci suggerisce che forse il design del codice può essere migliorato.

Il problema in questo codice è quella che si chiama primitive obsession: il rappresentare con un tipo primitivo (decimal) qualcosa che è di un tipo più complesso (un ammontare di denaro in valuta).

pupublic class MoneyAmount
{
    public decimal AmountInCurrency { get; set; }
    public Currency Currency { get; set; }
}

Sostituendo questa classe al campo “Price” dell’ordine, ovunque gestiamo operazioni che coinvolgono prezzi dei prodotti, siamo costretti a gestire eventuali valute – è qualcosa di cui non ci possiamo dimenticare, in quanto il compilatore stesso ci impedirà, per esempio, di sommare due MoneyAmount, a meno che non scriviamo noi del codice apposito.

Anche se presupponiamo che gli ordini siano sempre fatti con la stessa valuta grazie a vincoli imposti altrove nel programma, possiamo sempre implementare le somme con un codice come il seguente

public MoneyAmount SumMoneyAmounts(MoneyAmount amount1, MoneyAmount amount2)
{
    if (amount1.Currency != amount2.Currency)
    {
        throw new IncompatibleCurrenciesException("...");
    }
    else
    {
        return new MoneyAmount()
        {
            AmountInCurrency = amount1.AmountInCurrency + amount2.AmountInCurrency,
            Currency = amount1.Currency
        };
    }
}

Così siamo in grado di sommare quantità di denaro senza implementare un metodo di conversione che magari non ci serve. Se per qualche anomalia o cambiamento nel programma cerchiamo di sommare cifre in diverse valute otteniamo invece un errore. Seguiamo così uno stile “fail fast”, dove in alcuni casi solleveremo un’eccezione, magari mostrando un errore all’utente, ma dove è impossibile che sommiamo valute diverse senza convertirle, assegnando dei prezzi pressoché casuali ai nostri ordini. Per quanto generare errori non sia bello, un e-commerce che può sbagliare a calcolare il prezzo di un ordine porta a dei problemi ben maggiori.

Consideriamo la seguente vista, in un file cshtml

@if (Order.OrderSubmitted.Date >= DateTime.Now.AddHours(-24) && !Order.Shipped)
{
    <button>Revoke order</button>
}

<!-- some HTML code -->

@if (Order.OrderSubmitted.Date < DateTime.Now.AddHours(-24) || Order.Shipped)
{
    <span>Sorry, you can no longer change the order data</span>
}

<!-- some other HTML code -->

@if (Order.OrderSubmitted.Date >= DateTime.Now.AddHours(-48) && !Order.Shipped)
{
    <button>Change shipping address </button>
}

Sembrerebbe che il nostro programma permetta la modifica di un ordine se questo è stato fatto nelle ultime 24 ore e non è già stato spedito. Scorgete il bug? Avete notato che nella terza condizione consideriamo 48 ore, e probabilmente questo è errato? Certo, è facile quando tutte le condizioni sono vicine tra loro, ma se fossero spalmate in 12 diversi punti tra viste, servizi e controller ed immerse in migliaia di righe di codice non sarebbe così facile. Quello che qui è successo, probabilmente, è che una condizione di business è cambiata ed al buon programmatore è sfuggito un punto dove questa condizione veniva applicata.

La soluzione: implementare la logica predisposta ad una determinata funzione (es. determinare quando un ordine può essere modificato) in uno ed un solo punto del codice. Detta diversamente: niente copia e incolla! A volte è difficile evitare un po’ di duplicazione del codice, ma tenete presente che ad ogni copia incolla, il tempo di manutenzione della logica realizzata in quel codice raddoppia. Chi copia e incolla spesso, crea rapidamente, ma sarà molto lento a fare modifiche ed estensioni del codice.

In questo caso, basta isolare questa semplice logica in un metodo – per semplicità immaginiamo di poterlo mettere direttamente in una proprietà di Order

public bool CanBeChanged
{
    get { return OrderSubmitted.Date >= DateTime.Now.AddHours(-24) && !Shipped; }
}

Questo ci permette di riscrivere il codice della vista in modo più elegante, e di modificare la condizione di modificabilità dell’ordine in un solo punto per tutto il programma. Diventa più facile anche scrivere la sua negazione senza scrivere codice predisposto ad errori, come era nell’esempio precedente

@if@if (Order.CanBeChanged)
{
    <button>Revoke order</button>
}

<!-- some HTML code -->

@if (! Order.CanBeChanged)
{
    <span>Sorry, you can no longer change the order data</span>
}

<!-- some other HTML code -->

@if (Order.CanBeChanged)
{
    <button>Change invoicing information</button>
}

Consideriamo ora una questione più sottile, come nel seguente codice:

public void SendOrderInvoiceEmail(int orderId)
{
    var order = _service.GetOrder(orderId);
    order.Invoice = _invoiceGateway.GetInvoice(orderId);
    _mailService.SendInvoice(order);
}

public void SendInvoice(Order order)
{
    // ...
}

Vediamo che la fattura dell’ordine viene caricata in un secondo momento rispetto all’ordine stesso; questo può essere necessario in varie casistiche. La fattura potrebbe per esempio risiedere su un altro server ed essere gestita da un altro applicativo, con cui ci dobbiamo integrare. La chiamata potrebbe essere poco efficiente, e potremmo volerla effettuare solo quando ci è veramente necessaria.

Questo codice ha difetti minori dei precedenti, ma c’è un punto migliorabile: quando chiamiamo il metodo SendInvoice, dobbiamo sempre assicurarci di aver caricato la fattura. Più genericamente, negli oggetti di tipo Order avremo un campo che può essere valorizzato o meno, a seconda del flusso che è stato eseguito dal programma. Per sapere se quel campo è valorizzato, dovremmo sapere cosa ha fatto il codice che ha chiamato il nostro metodo – questo va chiaramente contro gli obiettivi della programmazione a oggetti. Anche se controlliamo se il campo è null, non abbiamo certezze: questo significa che non esiste fattura per questo ordine, o che semplicemente questa non è stata caricata?

Una soluzione: distinguere i due casi utilizzando il tipo di oggetto, e creando un nuovo tipo che estende Order per i casi in cui la fattura è stata caricata. Cambiando il tipo in SendInvoice, siamo sicuri che il metodo può essere chiamato solo per questi ordini. In qualsiasi punto del codice, a seconda del tipo di oggetto sappiamo se disponiamo della fattura o meno, e sarà il compilatore stesso a impedirci di confondere i due casi

public class OrderWithInvoice : Order
{
    public Invoice Invoice { get; set; }
    public OrderWithInvoice(Order order, Invoice invoice)
    {
        //...
    }
}

public void SendInvoice(OrderWithInvoice order)
{
//...
}

public void SendOrderInvoiceEmail(int orderId)
{
    var order = _service.GetOrder(orderId);
    var invoice = _invoiceGateway.GetInvoice(orderId);
    var orderWithInvoice = new OrderWithInvoice(order, invoice);
    _mailService.SendInvoice(orderWithInvoice);
}

Come principio generale, l’idea è che quando abbiamo un oggetto, tutti i suoi campi devono essere consistenti, indipendentemente dal flusso di esecuzione. Se in alcuni casi abbiamo oggetti di cui valorizziamo solo alcuni campi a seconda della situazione, ci troveremo presto ad avere del codice difficilmente manutenibile.

Vediamo adesso il seguente codice

public class Trip
{
    public DateTime StartDate { get; set; }
    public DateTime EndDate { get; set; }
    public int NumberOfDays { get; set; }
}

public Trip LoadTrip(int tripId)
{
    var trip = _service.GetTrip(tripId);
    trip.NumberOfDays = (int)Math.Floor((trip.EndDate - trip.StartDate).TotalDays) + 1;
    return trip;
}

L’oggetto contiene tre informazioni a proposito del viaggio che rappresenta. La data di inizio, la data di fine, ed i giorni di durata del viaggio. Il problema è che questi dati sono collegati tra loro: se cambiamo una delle date, la durata non sarà più allineata con i loro valori. Questo implica che ogni frammento di codice che modifica un suo campo si deve occupare di allineare anche gli altri. Qualsiasi dimenticanza in questo senso potrà facilmente causare dei bug.

Il problema di questo codice sta nella ridondanza dei dati: il campo NumberOfDays è ricavabile dagli altri, ma viene memorizzato come dato a sé stante. Così come il codice duplicato moltiplica il tempo di manutenzione, la ridondanza dei dati porta ad analoghi sforzi per mantenere i dati allineati. La soluzione: non salvare il risultato del calcolo in un campo, ma ricalcolarlo ad ogni richiesta a partire dagli altri campi.

public class Trip
{
    public DateTime StartDate { get; set; }
    public DateTime EndDate { get; set; }
    public int NumberOfDays
    {
        get { return (int)Math.Floor((EndDate - StartDate).TotalDays) + 1; }
    }
}

public Trip LoadTrip(int tripId)
{
    var trip = _service.GetTrip(tripId);
    return trip;
}

Se volessimo modificare il valore di NumberOfDays, non lo potremmo fare direttamente, ma dovremmo scrivere un metodo in Trip che gestisca il cambiamento. Questo è un bene, in quanto unificheremmo lì la logica della modifica: potremmo decidere per esempio che quando cambia la durata l’inizio resta uguale e cambia la durata di fine. Una volta definito un criterio del genere, questo verrebbe applicato a tutte le modifiche indipendentemente dal chiamante.

Prendiamo ora un esempio in un contesto specifico. Stiamo implementando un metodo di un controller, i cui parametri ci vengono inviati da un browser utilizzato da un utente tramite una richiesta HTTP.

[HttpGet, Authorize]
public ActionResult ViewOrder(int orderId, int userId)
{
    var operationAllowed = _service.OrderBelongsToUser(orderId, userId);
    if (!operationAllowed)
    {
        return new HttpStatusCodeResult(HttpStatusCode.Forbidden);
    }
    else
    {
        var order = _service.GetOrderViewModel(orderId);
        return View(order);
    }
}

In questo codice, abbiamo un problema di sicurezza. Viene controllato che l’ordine richiesto appartiene all’utente, ma l’identificativo dell’utente viene inviato dal client, e quindi è falsificabile. Tuttavia la risoluzione si può basare anche su un altro principio del buon codice: un metodo deve prendere in ingresso il numero minimo di parametri di cui necessita. In questo caso l’errore sta nel fatto che il controller deve già disporre dell’identificativo dell’utente per permettere l’accesso a questo metodo, quindi è errato inserirlo nella lista dei parametri. Un esempio corretto:

[HttpGet, Authorize]
public ActionResult ViewOrder(int orderId)
{
    var userId = User.Identity.GetUserId();
    var operationAllowed = _service.OrderBelongsToUser(orderId, userId);
    if (!operationAllowed)
    {
        return new HttpStatusCodeResult(HttpStatusCode.Forbidden);
    }
    else
    {
        var order = _service.GetOrderViewModel(orderId);
        return View(order);
    }
}

È interessante notare come in questo caso la determinazione dei parametri utili dipenda dal contesto. In contrapposizione, il metodo del servizio OrderBelongsToUser necessita invece dell’identificativo dell’utente, visto che il servizio non dispone tipicamente di questa informazione.

Conclusioni e materiali utili

Il numero di esempi che si potrebbero esporre è infinito, così come gli accorgimenti per scrivere codice migliore. Inoltre, spesso il contesto è determinante per capire quanto sia necessario lavorare sul debito tecnico e quanto, in alcuni casi, si può lasciar correre. E’ però importante porsi il problema, e chiedersi quanto il codice che scriviamo possa essere utilizzato e manutenuto in modo semplice e corretto, minimizzando la possibilità di errore al momento di future modifiche, nostre o di altri programmatori. Con questi esempi, spero di aver dato l’idea di come scrivere codice pulito non sia un esercizio di stile ma abbia conseguenze pratiche e tangibili sulla quantità di bug nella nostra applicazione.

Per chi volesse approfondire l’argomento, ci sono molte ottime risorse da studiare. L’opera più nota è probabilmente “Clean Code” di Robert C. Martin. Per scrivere buon codice, può essere inoltre utile conoscere e capire pattern architetturali, in modo da comprendere come si possano costruire soluzioni pulite a fronte di problemi complessi. Infine, è anche importante approfondire tutte le altre tecniche per la gestione e prevenzione dei bug, che coinvolgono per esempio il testing ed un buon lavoro di squadra – un ottimo approccio, per questo e per lo sviluppo Agile in generale, è descritto in “The Art of Agile Development” di James Shore.

Categorie

Giuneco Tech

Un po’ di codice da migliorare

Conclusioni e materiali utili

Blog