Clean Code en Harbour — I

Este es un artículo de Manuel Calero Solís para Harbour Magazine.

Introducción

Principios de ingeniería de software, del libro de Robert C. Martin Código limpio , adaptado para Harbour. Esta no es una guía de estilo, es una guía para producir software legible, reutilizable y refactorizable. No todos los principios de este documento deben seguirse estrictamente, y aún menos serán universalmente acordados. Estas son pautas y nada más, pero están codificadas durante muchos años de experiencia colectiva por los autores de Clean Code.

Inspirado en clean-code-php

Basado en la traducción de Francisco Bórquez

Variables

Use nombres de variables significativos y fáciles de pronunciar.

Mal:

$cdt = DateTime();

Bien:

$currentDateTime = DateTime();

Usa el mismo vocabulario para el mismo tipo de variable

Mal:

getUserInfo()
getUserData()
getUserRecord()
getUserProfile()

Bien:

getUser()

Use los nombres para las búsqueda (parte 1)

Es importante que el código que escribimos sea legible y pueda buscarse. Al no nombrar variables que terminan siendo significativas para comprender nuestro programa, molestamos a los futuros lectores de nuestro código. Hagamos que los nombres sean faciles de buscar y entender.

Mal:

// Que significa realmente el dos
fileHandle = fOpen( "fopen.prg", 2)

Bien:

fileHandle = fOpen( "fopen.prg", FO_READWRITE)

Use los nombres para las búsqueda (parte 2)

Mal:

// What the heck is 4 for?
if (user:access == 4) 
    // do ...
end if

Bien:

#define ACCESS_READ = 1
#define ACCESS_CREATE = 2
#define ACCESS_UPDATE = 4
#define ACCESS_DELETE = 8
​
if (user:access == ACCESS_UPDATE) 
    // do ...
end if

Usar variables explicativas

Mal:

programsDirectory := Directory( "*.prg" )
​
aeval( programsDirectory, {|x| sendFile( x[1], x[2] ) } )

Bien:

programsDirectory := Directory( "*.prg" )
​
aeval( programsDirectory, {|programFile| sendFile( programFile[F_NAME], programFile[F_SIZE] ) } )

Evite anidar demasiado y regrese pronto (parte 1)

Demasiadas sentencias ‘if’ pueden hacer que su código sea difícil de seguir. Hay autores que llegan a decir que si tu funcion tiene muchos else (incluso solo uno), debe ser reescrita.

Explícito es mejor que implícito.

Mal:

function isShopOpen(day)
​
    if !empty(day) 
        if hb_isstring(day)
            day = lower(day)
            if (day == 'friday')
                return .t.
            elseif (day == 'saturday') 
                return .t.
            elseif (day == 'sunday')
                return .t.
            else 
                return .f.
            end if 
        else 
            return .f.
        end if 
    else 
        return .f.
    end if
​
return .f.

Bien:

function isShopOpen(day)
    
    local openingDays := {'friday', 'saturday', 'sunday'}
​
    if empty(day)
        return .f.
    end if 
​
return ( ascan( openingDays, lower(day) ) == 0 )

Evite anidar demasiado y regrese pronto (parte 2)

Mal:

function fibonacci( n )
​
    if (n < 50) 
        if (n != 0) 
            if (n != 1) 
                return fibonacci(n - 1) + fibonacci(n - 2)
            else 
                return 1
            end if 
        else 
            return 0
        end if
    else
        return 'Not supported'
    end if
​
return n

Bien:

function fibonacci( n )
​
    if (n == 0 .or. n == 1) 
        return n
    end if 
​
    if (n > 50) 
        return 'Not supported'
    end if 
​
    return fibonacci(n - 1) + fibonacci(n - 2)
​
return n
​

Evitar el mapeo mental

No obligue al lector de su código a traducir lo que significa la variable. Explícito es mejor que implícito.

Mal:

local li
local l := {'Austin', 'New York', 'San Francisco'}
​
for (i := 1 to len( l )) 
    li = l[i]
    doStuff()
    doSomeOtherStuff()
    // ...
    // ...
    // ...
    // Espera, ¿qué es `li`?
    dispatch(li)
next

Bien:

​
local locations := {'Austin', 'New York', 'San Francisco'}
​
foreach location in location
    doStuff()
    doSomeOtherStuff()
    // ...
    // ...
    // ...
    dispatch(location)
next

No agregue contexto innecesario

Si su nombre de clase / objeto le dice algo, no lo repita en su nombre de variable.

Mal:

CLASS Car
​
    DATA carMake
    DATA carModel
    DATA carColor
​
    //...
ENDCLASS

Bien:

CLASS Car
​
    DATA make
    DATA model
    DATA color
​
    //...
ENDCLASS

Funciones

Parametros de función (lo ideal es 2 o menos)

Limitar la cantidad de parámetros de función es increíblemente importante porque hace que probarla sea más fácil. Tener más de tres conduce a una explosión combinatoriadonde tienes que probar toneladas de casos diferentes con cada argumento por separado.

Cero argumentos es el caso ideal. Uno o dos argumentos están bien, y se deben evitar tres.Algo más que eso debe consolidarse. Por lo general, si tiene más de dosargumentos entonces su función está tratando de hacer demasiado. En los casos en que no lo es, la mayoríadel tiempo, un objeto de nivel superior bastará como argumento.

Mal:

function createMenu(title, body, buttonText, cancellable)
​
    // ...
return

Bien:

class MenuConfig
​
    DATA title
    DATA body
    DATA buttonText
    DATA cancellable INIT .f.
​
end class
​
config              := MenuConfig():New()
config:title        := 'Foo'
config:body         := 'Bar'
config:buttonText   := 'Baz'
config:cancellable  := .t.
​
function createMenu( config )
    // ...
return

Las funciones deberían hacer una cosa

Esta es, con mucho, la regla más importante en ingeniería de software. Cuando las funciones hacen másnde una cosa, son más difíciles de componer, probar y razonar. Cuando puedes aislaruna función para una sola acción, se pueden refactorizar fácilmente y su código leerá mucholimpio. Si no quita nada más de esta guía que no sea esto, estará adelantede muchos desarrolladores.

Mal:

function emailClients(clients)
​
    foreach client in clients
        clientRecord = clientModel():find(client)
        if clientRecord:isActive()
            email(client)
        end if 
    next
return

Bien:

function emailClients(clients)
​
    local activeClients := activeClients( clients )
    
    aeval( activeClients, {|client| email( client ) } )
​
return nil
​
function activeClients( clients )
​
    local activeClients := {}
    
    aeval( clients, {|client| if( isClientActive( client ), aadd( activeClients, client ), ) } )
​
return ( activeClients )
​
function isClientActive( client )
​
    if clientModel():find( client )
        return ( clientRecord:isActive() )
    end if 
​
return ( .f. )

Los nombres de las funciones deberían decir lo que hacen

Mal:

CLASS Email
​
    //...
​
    METHOD handle()
    
        mail( ::to, ::subject, ::body )
    
    RETURN nil
​
ENDCLASS
​
message := Email(...):New()
// ¿Que es esto? 
message:handle()

Bien:

CLASS Email 
​
    //...
​
    METHOD send()
    
        mail( ::to, ::subject, ::body)
​
    RETURN nil
​
ENDCLASS
​
message := Email(...):New()
// Limpio y obvio
message:send()

Las funciones deben tener sólo un nivel de abstracción

Cuando tienes más de un nivel de abstracción usualmente es porque tu función está haciendo demasiado. Separarlas en funciones lleva a la reutilización.

Mal:

function ExecutorStaments( tokenizedStatement )
​
    local statement
    local statements    := hb_atokens( tokenizedStatement, "," ) 
​
    for each statement in statements
        if SQLConexion():Parse( statement )
            SQLConexion():Execute( statement )
            // ...
        end if
    next
​
return

Bien:

Lo mejor es crear una clase que tenga dentro dependencias a otras clases.

CLASS Tokenizer
​
    METHOD new()
​
    METHOD tokenizer()  inline ( hb_atokens( tokens, "," ) )
​
end CLASS
​
CLASS Conexion
​
    data oConexion 
​
    METHOD new()
​
    METHOD Parse( statement )   inline ( ::oConexion:Parse( statement ) )
​
    METHOD Execute( statement ) inline ( ::oConexion:Execute( statement ) )
​
ENDCLASS
​
CLASS ExecutorStaments
​
    DATA tokenizer
    DATA conexion
​
    METHOD new()
    
        ::tokenizer     := Tokenizer():New()
        ::conexion      := Conexion():New()
    
    RETURN ( self ) 
​
    METHOD Execute( statement )
    
        local statement
        local statements  := ::tokenizer:tokenizer( statement )
​
        for each statement in statements
            if ::conexion:Parse( statement )
                ::conexion:Execute( statement )
            end if
        next
​
    RETURN ( self ) 
​
ENDCLASS

No usar logicos como parámetros de funciones

Los valores logicos le dicen al usuario que la función hace más de una cosa. Las funciones deben hacer sólo una. Divide tus funciones si ellas siguen diferentes caminos basados en un valor booleano.

Mal:

function createFile(name, temp)
​
    if (temp) {
        fcreate( './temp/' + name)
    else 
        fcreate( name )
    end if 
​
return

Bien:

function createFile(name, temp)
​
    fcreate( './temp/' + name)
    
return 
​
function createTempFile(name)
​
    fcreate( './temp/' + name)
    
return

Evitar efectos secundarios

Una función produce un efecto secundario si hace algo más que tomar un valor y devolver otro. Un efecto secundario puede ser escribir en un archivo, modificar alguna variable global, o accidentalmente darle todo tu dinero a un extraño.

Ahora, ocasionalmente necesitaras los efectos secundarios en un programa. Como los ejemplos anteriores, necesitarás escribir en un archivo. Lo que quieres hacer en esos casos es centralizar donde realizarlos. No tengas muchas funciones y clases que escriban un archivo en particular. Ten un servicio que lo haga. Uno y sólo uno.

El punto principal es evitar trampas comunes como compartir estados entre objetos sin alguna estructura, usar tipos de datos mutables que puedan ser escritos por cualquiera, y no centralizar donde el efecto paralelo ocurre. Si puedes hacerlo, serás más feliz que la mayoría de los demás programadores.

Mal:

// Variable global referenciada por la siguiente función.
// Si tenemos otra función que use el mismo nombre, ahora será un arreglo y podría romperla.
​
memvar name 
​
function splitIntoFirstAndLastName()
​
    name = hb_atokens( name, ' ' )
​
return nil
​
name = 'Manuel Calero'
​
splitIntoFirstAndLastName()
​
? ( hb_valtoexp( name ) ) // {'Manuel', 'Calero'}

Bien:

function splitIntoFirstAndLastName( name )
​
return hb_atokens( name, ' ' )
​
name        := 'Manuel Calero'
newName     := splitIntoFirstAndLastName(name)
​
? ( hb_valtoexp( name ) )       // 'Manuel Calero'
? ( hb_valtoexp( newName ) )    // {'Manuel', 'Calero'}

No escribas funciones globales

Llenar de funciones globales es una mala práctica en muchos lenguajes porque puedes chocar con otra librería.

Mal:

function config()
​
return  { 'foo' => 'bar' }

Bien:

CLASS Configuration
​
    DATA configuration INIT {}
​
    METHOD New( configuration )
    
        ::configuration := configuration
​
    RETURN ( self )
​
    METHOD get( key )
    
        if hhaskey( ::configuration, key )
            RETURN ( hget( ::configuration, key ) )
        end if 
​
    RETURN ( nil )
​
ENDCLASS

Crea la variable configuration con una instancia de la clase Configuration

configuration := Configuration():new( {'foo' => 'bar'} )

Y ahora puedes usar una instancia de la clase Configuration en tu aplicación.

No usar el patrón Singleton

Singleton es un anti-patrón. Citando a Brian Button:

1. Son usados generalmente como una instancia global, ¿Por qué eso es malo? Porque escondes las dependencias de tu aplicación en tu código, en lugar de exponerlas. Hacer algo global para evitar pasarlo es una hediondez de código.
2. Violan el principio de la responsabilidad única]: en virtud del hecho de que ellos controlan su propia creación y ciclo de vida.
3. Inherentemente causan que el código esté estrechamente acoplado. Esto hace que muchas veces sean difíciles de probar, aunque en Harbour no tenemos bancos de pruebas.
4. Llevan su estado al ciclo de vida de la aplicación. Otro golpe a las pruebas porque puedes terminar con una situación donde las pruebas necesitan ser ordenadas lo cual es un gran no para las pruebas unitarias. ¿Por qué? Porque cada prueba unitaria debe hacerse independiente de la otra. Misko Hevery ha realizado unas reflexiones interesantes sobre el origen del problema.

Mal:

CLASS DBConnection
​
    CLASSDATA instance
​
    METHOD New( dsn )
        // ...
    RETURN ( self )
​
    METHOD getInstance()    INLINE ( if( empty( ::oInstance ), ::oInstance := ::New(), ), ::oInstance ) 
​
    // ...
ENDCLASS
​
singleton := DBConnection():getInstance()

Bien:

CLASS DBConnection
​
    METHOD New( dsn )
        // ...
    RETURN ( self )
​
ENDCLASS

Crea una instancia de la clase DBConnection y configúrala con DSN.

connection :=  DBConnection():New( dsn )

Y ahora debes usar la instancia de DBConnection en tu aplicación.

Encapsular condicionales

Mal:

if ( article:state == 'published' ) 
    // ...
end if

Bien:

if ( article:isPublished() )
    // ...
end if

Evitar condicionales negativos

Mal:

function isNodeNotPresent( node )
​
    // ...
​
​
if ( !isDOMNodeNotPresent( node ) )
​
    // ...
​

Bien:

function isNodePresent( node )
​
    // ...
​
if (isNodePresent( node ) ) 
​
    // ...
​

Evitar condicionales

Esta parece ser una tarea imposible. Al escuchar esto por primera vez, la mayoría de la gente dice, “¿cómo se supone que haré algo sin una declaración if?" La respuesta es que la mayoría de las veces puedes usar polimorfismo para lograr el mismo resultado.

La segunda pregunta usualmente es, “bien, eso es genial, ¿pero cómo puedo hacerlo?” La respuesta es un concepto de código limpio que ya hemos aprendido: una función debe hacer sólo una cosa. Cuando tienes clases y funciones que usan declaraciones if, estás diciéndole al usuario que tu función hace más de una cosa. Recuerda, hacer sólo una cosa.

Mal:

CLASS Airplane
​
    // ...
​
    METHOD getCruisingAltitude()
    
        SWITCH ::type 
            CASE '777':
                RETURN ::getMaxAltitude() - ::getPassengerCount()
            CASE 'Air Force One':
                RETURN ::getMaxAltitude()
            CASE 'Cessna':
                RETURN ::getMaxAltitude() - ::getFuelExpenditure()
        END
​
    RETURN ( 0 )
​
ENDCLASS

Bien:

CLASS Airplane
​
    // ...
​
    METHOD getCruisingAltitude() VIRTUAL
​
ENDCLASS
​
CLASS Boeing777 FROM Airplane
​
    // ...
​
    METHOD getCruisingAltitude()
    
    RETURN ::getMaxAltitude() - ::getPassengerCount()
    
ENDCLASS
​
CLASS AirForceOne implements Airplane
    
    // ...
​
    METHOD getCruisingAltitude()
    
    RETURN ::getMaxAltitude() 
    
ENDCLASS
​
CLASS Cessna implements Airplane
​
    // ...
​
    METHOD getCruisingAltitude()
    
    RETURN ::getMaxAltitude() - ::getFuelExpenditure()
    
ENDCLASS

Evitar revisión de tipo

Harbour es un lenguaje no tipado, lo que quiere decir que tus funciones pueden tener cualquier tipo de argumento. Algunas veces habrás sentido esta libertad y te habrás tentado a hacer revisión de tipo en tus funciones. Hay muchas maneras de evitar tener que hacerlo.

Mal:

METHOD travelToTexas( vehicle )
​
    if vehicle:IsDerivedFrom( 'Bicycle' ) 
        vehicle:pedalTo( Location():New( 'texas' ) )
    elseif vehicle:IsDerivedFrom( 'Car' )  
        vehicle:driveTo( Location():New( 'texas' ) )
    end if
    
RETURN ( nil )

Bien:

METHOD travelToTexas( vehicle )
​
    vehicle:travelTo( Location():New( 'texas' ) )
    
RETURN ( nil )

Quitar código muerto

El código muerto es tan malo como el código duplicado. No hay motivos para mantenerlo en tu código fuente. Si no está siendo llamado, ¡deshazte de él! Siempre estará a salvo en tu versión histórica si aún lo necesitas.

Mal:

CLASS oldRequestModule( url )
​
    // ...
​
ENDCLASS
​
CLASS newRequestModule( url )
​
    // ...
​
ENDCLASS
​
request := newRequestModule():New( requestUrl )
​
inventoryTracker('apples', request, 'www.inventory-awesome.io')
​

Bien:

CLASS RequestModule( url )
​
    // ...
​
ENDCLASS
​
request := RequestModule():New( requestUrl )
​
inventoryTracker('apples', request, 'www.inventory-awesome.io')