Código limpio: Funciones

En esta entrada vamos a ver qué características tienen o no tienen que tener nuestras funciones para que puedan considerase código limpio. Para ello vamos a analizar el componente Lock de Symfony por ser un componente bastante sencillo de entender. Este componente crea y administra bloqueos, un mecanismo para proporcionar acceso exclusivo a un recurso compartido al más puro estilo de los semáforos en el lenguaje de programación c.

Hacer una única cosa

Una función solo debería hacer una única cosa, y además debería de ser ser capaz de hacerla bien. Este concepto a la vez que es sencillo de entender es bastante complejo de explicar, así que vamos a definir algunas características que pueden indicarte que una de tus funciones hacen más de una cosa.

Demasiado larga

Si una función tiene demasiadas líneas de código, es muy probable que esté haciendo varias cosas a la vez. Deberías tratar de encontrar bloques dentro de esa misma función que hagan una cosa concreta e ir extrayendolas en funciones más pequeñas.

El límite exacto para considerarse demasiado larga es un poco difuso, pero por encima de 20 líneas deberías al menos plantearte si tu función es demasiado larga.

Secciones

Como continuación a lo anterior, si una función puede dividirse en secciones, nos encontramos ante un caso claro de que esa función hace más de una cosa.

A continuación vemos la función Lock::acquire

/**
 * {@inheritdoc}
 */
public function acquire(bool $blocking = false): bool
{
    try {
        if ($blocking) {
            if (!$this->store instanceof BlockingStoreInterface) {
                throw new NotSupportedException(sprintf('The store "%s" does not support blocking locks.', get_debug_type($this->store)));
            }
            $this->store->waitAndSave($this->key);
        } else {
            $this->store->save($this->key);
        }

        $this->dirty = true;
        $this->logger->info('Successfully acquired the "{resource}" lock.', ['resource' => $this->key]);

        if ($this->ttl) {
            $this->refresh();
        }

        if ($this->key->isExpired()) {
            try {
                $this->release();
            } catch (\Exception $e) {
                // swallow exception to not hide the original issue
            }
            throw new LockExpiredException(sprintf('Failed to store the "%s" lock.', $this->key));
        }

        return true;
    } catch (LockConflictedException $e) {
        $this->dirty = false;
        $this->logger->notice('Failed to acquire the "{resource}" lock. Someone else already acquired the lock.', ['resource' => $this->key]);

        if ($blocking) {
            throw $e;
        }

        return false;
    } catch (\Exception $e) {
        $this->logger->notice('Failed to acquire the "{resource}" lock.', ['resource' => $this->key, 'exception' => $e]);
        throw new LockAcquiringException(sprintf('Failed to acquire the "%s" lock.', $this->key), 0, $e);
    }
}

Podemos observar 3 secciones principales. En la primera trata de adquirir el bloqueo, en la segunda comprueba si el bloqueo debería haber expirado y en el último bloque realiza el manejo de errores. Podríamos facilitarle mucho la vida al lector si extrajeramos esos bloques dejándolos en algo similar a esto.

/**
 * {@inheritdoc}
 */
public function acquire(bool $blocking = false): bool
{
    try {
        $this->tryAcquire($blocking);
        $this->releaseIfExpired();

        return true;
    } catch (\Exception $e) {
        return $this->handleException($e);
    }
}

switch

Un bloque switch es probablemente un indicador de que una función hace varias cosas. La forma de solucionar esto es extraer ese bloque switch para que la única cosa que haga nuestra función es decidir qué hacer en función del valor que le pasemos a switch.

A continuación vemos un caso válido de un bloque switch dentro de la clase Store\StoreFactory, ya que aunque tiene un elemento switch la función hace una única cosa, en este caso devolver la instancia de Store\PersistingStoreInterface correcta en función del tipo de conexión que estemos utilizando.

/**
     * @param \Redis|\RedisArray|...
     *
     * @return PersistingStoreInterface
     */
    public static function createStore($connection)
    {
        if (!\is_string($connection) && !\is_object($connection)) {
            throw new \TypeError(sprintf('Argument 1 passed to "%s()" must be [...], "%s" given.');
        }

        switch (true) {
            case $connection instanceof \Redis:
            case $connection instanceof \RedisArray:
            case $connection instanceof \RedisCluster:
            case $connection instanceof \Predis\ClientInterface:
            case $connection instanceof RedisProxy:
            case $connection instanceof RedisClusterProxy:
                return new RedisStore($connection);
            // en total hay 32 casos diferentes.
        }

        throw new InvalidArgumentException(sprintf('Unsupported Connection: "%s".', $connection));
    }

Un sólo nivel de abstracción

Dentro de una misma función debemos mantenernos siempre dentro del mismo nivel de abstracción.

Por ejemplo si tuviéramos una capa de dominio de negocio y una capa de persistencia de datos, dentro de una misma función no deberíamos pasar de un nivel a otro, por lo cual tendríamos funciones con lógica de negocio y funciones con lógica de persistencia de datos.

Procesamiento de errores

Empezaremos indicando que el procesamiento de errores es mucho más sencillo a través de excepciones que de códigos de error, pero esto lo veremos mucho más en profundidad en el capítulo de procesamiento de errores.

El problema que tienen los bloques try / catch es que dificultan mucho la lectura.Si tenemos una función donde además de la lógica de la función tiene uno o más bloques try / catch, probablemente deberíamos extraer la parte de la lógica de negocio y dejar esa función sólamente con los bloques try catch tal y cómo hicimos con la función Lock::acquire en el ejemplo anterior.