No, no estoy hablando de mi (grillos.mp3).
A veces creamos complejidad innecesaria al programar solo por tratar de ser ingeniosos. A veces una "simple" decisión crea un efecto dominó que se refleja en la lógica de nuestro código.
Sé que mucha gente que no programa en PHP o que lo hizo en el año 2000 A.C., dice que el lenguaje es horrible, que solo programadores inexpertos lo utilizan y no sé qué. Admito que el lenguaje tiene muchas fallas en su diseño al incorporar elementos de Perl, Java, C..., que es fácil iniciarse con él y eso es su gran ventaja y desventaja a la vez, pero en su defensa, me atrevo a decir que el lenguaje no hace al programador. Si bien, hay lenguajes que inculcan buenas prácticas, se puede ser malo programando en cualquier lenguaje.
En este post voy a mostrar un ejemplo de código que me topé por ahí en un proyecto. Desde mi punto de vista ilustra bastante bien de lo que va este rant: complejidad innecesaria.
He creado un repositorio en Github con las clases del código, clases para las pruebas de unidad, archivos de configuración para PHPUnit, PHP CodeSniffer y Composer , así como archivos de texto con el resultado esperado de algunas de las pruebas (fixtures).
La clase que mostraré a continuación tiene un solo propósito: recibir una lista de plugins que se utilizarán en el lado del cliente, codificarla en JSON y opcionalmente escapar los caracteres HTML. ¿Sencillo, no? Bueno, veamos como se encarga de ello nuestra clase:
src/PluginHelperOriginal.php
<?php
namespace App;
class PluginHelperOriginal
{
public function encodePluginSettings()
{
$attributeValue = '';
$pluginList = func_get_args();
if (isset($pluginList[0])) {
if ($pluginList[0] === false) {
array_shift($pluginList);
$attributeValue = json_encode($pluginList);
} elseif ($pluginList[0] === true) {
array_shift($pluginList);
$attributeValue = htmlentities(json_encode($pluginList));
} else {
$attributeValue = htmlentities(json_encode($pluginList));
}
}
return $attributeValue;
}
}
Tenemos un método llamado encodePluginSettings() pero vemos que no tiene ningún parámetro definido. Entonces, ¿cómo y donde recibe los parámetros? Buena pregunta. Y es aquí donde comienza la confusión.
¿Por qué digo que aquí comienza la confusión? Primero que nada, tenemos que indagar un poco en el código para darnos cuenta que este método utiliza una función de PHP llamada func_get_args() que devuelve un arreglo con los parámetros enviados a un método. Pero no hay parámetros definidos, ¿cómo es eso posible? En PHP no es posible invocar a un método con MENOS parámetros de los que se definieron porque ocurre un error, pero sí es posible invocar un método con MÁS parámetros de los que se definieron. En este caso no se define ninguno para encodePluginSettings(), por lo que se puede invocar con 0 o más parámetros. Esto es un "truco" utilizado en PHP para utilizar funciones variádicas. Por lo menos se hacía de esta manera hasta la versión 5.6 donde ya se implementaron como parte del lenguaje.
El método se puede invocar con 0 o más parámetros, pero se supone que debe recibir algo para transformarlo a JSON, ¿no? Cierto. Entonces hay que validar que se haya enviado por lo menos un parámetro. Esto es precisamente lo que verifica la primer condición; y si no se pasó algún parámetro, lo que devuelve es... el valor inicial de $attributeValue. No pues chingón, si se invoca con uno o más parámetros se procede a codificar y si no, se devuelve un string vacío, que todos sabemos es JSON perfectamente válido (NOT).
Dejando por un momento de lado esta inconsistencia, veamos qué sucede si enviamos por lo menos un parámetro. Vemos que inmediatamente después hay otra condición, pero esta se encarga de verificar si el valor de este primer parámetro equivale a false, de ser así lo remueve del array usando la función array_shift(). Después procede a codificar a formato JSON los valores restantes con json_encode().
Luego tenemos otra condición que hace algo similar, pero esta revisa si el valor del primer parámetro equivale a true. Si la condición se cumple, también remueve el primer parámetro del array y codifica a JSON, pero adicionalmente escapa los caracteres HTML mediante htmlentities() . Ok, parece ser que el primer parámetro es el que se utiliza para indicar si deseamos que se escapen los caracteres HTML. Pero no queremos que ese primer parámetro se codifique junto al resto, es por eso que se elimina del array.
Hay una condición else que se ejecutará cuando exista el primer parámetro pero su valor no sea true ni false. De cumplirse este else se codifican los parámetros en formato JSON y también se escapan los caracteres HTML. ¡Un momento!, esto es muy parecido a lo que sucede con la condición anterior, solo que no se elimina el primer elemento del array. Incluso también es muy similar a la condición que verifica si el valor del primer parámetro es false.
Recapitulando tenemos que:
Si existe al menos un parámetro y tiene un valor true entonces hay que codificar en formato JSON y escapar caracteres HTML; si el parámetro tiene un valor false solo hay que codificar en formato JSON y no escapar caracteres HTML. Pero si existe por lo menos un parámetro pero no contiene un valor tipo boolean entonces el comportamiento default es codificar a JSON y escapar caracteres HTML.
No sé, muchas condiciones y lógica duplicada. Sin mencionar que existen algunas inconsistencias. Por ejemplo, ¿qué pasa si se envía solamente un parámetro y es de tipo boolean? El método devolverá un string vacío. Esto no está bien, el método debería fallar si no recibe lo mínimo que necesita para funcionar correctamente.
¿Cómo podemos mejorar el código para que sea más entendible, o por lo menos no tan complejo? No se pierda nuestro próximo epis... no es cierto, veamos cómo. Pero antes de hacer cualquier modificación vamos a crear algunas pruebas de unidad en base a los resultados que arroja el código que tenemos. Haremos esto para asegurarnos que nuestros cambios no afecten los resultados.
Voy a utilizar Composer para aprovechar su autoloader compatible con PSR-4 y también para instalar algunas librerías como PHPUnit y PHP CodeSniffer (este último no es necesario, pero acostumbro utilizar el estándar PSR2 en mi código y esta herramienta valida que el código cumpla con este u otro estándar que le indiquemos). Ya lo mencioné antes, pero subí el código terminado y demás archivos a un repositorio de Github por si lo quieren revisar.
Las pruebas de unidad verifican el resultado arrojado por el método encodePluginSettings() con diferentes combinaciones de parámetros. Por ejemplo, es posible pasar uno o más strings que representen el nombre de plugins a utilizar, pero también es posible que cada plugin indique opciones de configuración, en cuyo caso dicho parámetro deberá ser un array.
Por ejemplo, supongamos que deseamos incluir un plugin llamado ScrollSpy y no queremos que se escapen caracteres HTML; lo haríamos de la siguiente manera:
$pluginHelper = new App\PluginHelperOriginal;
// imprime ["scrollSpy"]
var_dump($pluginHelper->encodePluginSettings(false, 'scrollSpy')); Noten que pasamos como primer parámetro un valor false para indicar que no se escapen caracteres HTML. Todo lo que venga después del parámetro boolean se considera como la lista de plugins y se convierte a JSON.
En base al comportamiento actual de la clase, crearé un par de pruebas de unidad para verificar lo que arroja actualmente y a partir de ahí hacer las modificaciones que desee y asegurarme que no se alteren los resultados:
tests/EncodeOldTest.php
<?php
require __DIR__ . '/../vendor/autoload.php';
use PHPUnit\Framework\TestCase;
use App\PluginHelperOriginal as PluginHelper;
class EncodeOldTest extends TestCase
{
protected $pluginHelper;
protected function setUp()
{
$this->pluginHelper = new PluginHelper;
}
protected function getFixtureContent($fixtureName)
{
return file_get_contents(__DIR__ . '/fixtures/' . $fixtureName . '.txt');
}
public function testCanEncodeASinglePlugin()
{
$this->assertEquals(
$this->getFixtureContent('fixture1'),
$this->pluginHelper->encodePluginSettings('ScrollSpy')
);
$this->assertEquals(
'["ScrollSpy"]',
$this->pluginHelper->encodePluginSettings(false, 'ScrollSpy')
);
$this->assertEquals(
$this->getFixtureContent('fixture2'),
$this->pluginHelper->encodePluginSettings(
['ScrollSpy', ['delay' => 500, 'color' => '#ff00ff']]
)
);
$this->assertEquals(
$this->getFixtureContent('fixture3'),
$this->pluginHelper->encodePluginSettings(
false,
['ScrollSpy', ['delay' => 500, 'color' => '#ff00ff']]
)
);
}
public function testCanEncodeMultiplePlugins()
{
$this->assertEquals(
$this->getFixtureContent('fixture4'),
$this->pluginHelper->encodePluginSettings('ScrollSpy', 'TabbedNavigation')
);
$this->assertEquals(
$this->getFixtureContent('fixture5'),
$this->pluginHelper->encodePluginSettings(
['ScrollSpy', ['delay' => 500, 'color' => '#ff00ff']],
['TabbedNavigation', ['responsive' => true, 'color' => '#ff0000']]
)
);
$this->assertEquals(
'["ScrollSpy","TabbedNavigation"]',
$this->pluginHelper->encodePluginSettings(false, 'ScrollSpy', 'TabbedNavigation')
);
$this->assertEquals(
$this->getFixtureContent('fixture6'),
$this->pluginHelper->encodePluginSettings(
false,
['ScrollSpy', ['delay' => 500, 'color' => '#ff00ff']],
['TabbedNavigation',['responsive' => true,'color' => '#ff0000']]
)
);
}
}Estas pruebas simplemente comparan lo que arroja el método encodePluginSettings() con distintos parámetros; pasando un solo nombre de plugin, varios plugins, y esto mismo pero escapando y no escapando caracteres HTML.
Lo que vale la pena resaltar tal vez es el método setUp(). Este es un método de PHPUnit que sirve para ejecutar código de forma automática para cada una de las pruebas definidas; esto es útil para no tener que repetir código en cada una de ellas y para inicializar objetos y así evitar que se recicle el mismo objeto y se obtenga algún resultado inesperado.
También se ha definido un método llamado getFixtureContent(), el cual simplemente es un método auxiliar que definí para leer contenido desde un archivo de texto. Esto debido a que algunas llamadas a encodePluginSettings() devuelven un string algo largo como para ponerlo directamente en el código.
Para ejecutar las pruebas de unidad y verificar que pasan, ejecutamos PHPUnit para este nuevo archivo desde la carpeta vendor ya que se ha instalado mediante Composer. Si ya tienen PHPUnit instalado globalmente omitan el path antes de phpunit:
./vendor/bin/phpunit tests/EncodeOldTest.php
PHPUnit 5.5.4 by Sebastian Bergmann and contributors.
.. 2 / 2 (100%)
Time: 71 ms, Memory: 3.50MB
OK (2 tests, 8 assertions)Ok, las pruebas pasan, pero ahora hay que pensar cómo simplificar el código. Lo primero que se me ocurre es que, definitivamente, quiero deshacerme de los parámetros dinámicos. Esto podría considerarse como "trampa", ya que implica modificar los parámetros del método, y por lo tanto las pruebas de unidad. Si bien es cierto, es con el fin de mejorar el código; además, no se van a alterar los resultados de las pruebas.
Lo más sencillo tal vez es definir dos parámetros: la lista de plugins como un array y un parámetro opcional para indicar si se debe escapar los caracteres HTML. Nombraré al primero $pluginList y al segundo $escapeHTML, el cual tendrá como valor default true:
src/PluginHelper.php
<?php
namespace App;
class PluginHelper
{
public function encodePluginSettings(array $pluginList, $escapeHTML = true)
{
}
}El archivo de pruebas de la versión nueva de la clase espera los mismos valores, solo cambia la manera en que se mandan los parámetros. Por ejemplo, ahora se manda un array con la lista de plugins en lugar de mandar cada uno por separado. Si el plugin contiene opciones de configuración, entonces deberá ser a su vez un array, en el que el primer elemento del array es el nombre del plugin y el segundo elemento un array de opciones de configuración. Ya sé, muchos arrays en esta nueva forma de invocación, pero desde mi punto de vista es más claro dónde empieza y termina cada plugin. Es más explícito.
tests/EncodeTest.php
<?php
require __DIR__ . '/../vendor/autoload.php';
use PHPUnit\Framework\TestCase;
use App\PluginHelper;
class EncodeTest extends TestCase
{
protected $pluginHelper;
protected function setUp()
{
$this->pluginHelper = new PluginHelper;
}
protected function getFixtureContent($fixtureName)
{
return file_get_contents(__DIR__ . '/fixtures/' . $fixtureName . '.txt');
}
public function testCanEncodeASinglePlugin()
{
$this->assertEquals(
$this->getFixtureContent('fixture1'),
$this->pluginHelper->encodePluginSettings(['ScrollSpy'])
);
$this->assertEquals(
'["ScrollSpy"]',
$this->pluginHelper->encodePluginSettings(['ScrollSpy'], false)
);
$this->assertEquals(
$this->getFixtureContent('fixture2'),
$this->pluginHelper->encodePluginSettings(
[['ScrollSpy', ['delay' => 500, 'color' => '#ff00ff']]]
)
);
$this->assertEquals(
$this->getFixtureContent('fixture3'),
$this->pluginHelper->encodePluginSettings(
[['ScrollSpy', ['delay' => 500, 'color' => '#ff00ff']]],
false
)
);
}
public function testCanEncodeMultiplePlugins()
{
$this->assertEquals(
$this->getFixtureContent('fixture4'),
$this->pluginHelper->encodePluginSettings(['ScrollSpy', 'TabbedNavigation'])
);
$this->assertEquals(
$this->getFixtureContent('fixture5'),
$this->pluginHelper->encodePluginSettings(
[
[ 'ScrollSpy', ['delay' => 500, 'color' => '#ff00ff']],
['TabbedNavigation', ['responsive' => true, 'color' => '#ff0000']]
]
)
);
$this->assertEquals(
'["ScrollSpy","TabbedNavigation"]',
$this->pluginHelper->encodePluginSettings(['ScrollSpy', 'TabbedNavigation'], false)
);
$this->assertEquals(
$this->getFixtureContent('fixture6'),
$this->pluginHelper->encodePluginSettings(
[
['ScrollSpy', ['delay' => 500, 'color' => '#ff00ff']],
['TabbedNavigation',['responsive' => true,'color' => '#ff0000']]
],
false
)
);
}
}Si ejecutamos PHPUnit para el archivo EncodeTest.php las pruebas van a fallar porque el método encodePluginSettings() no contiene código. ¿Qué deberá llevar entonces? Algo como lo siguiente:
src/PluginHelper.php
<?php
namespace App;
class PluginHelper
{
public function encodePluginSettings(array $pluginList, $escapeHTML = true)
{
$attributeValue = json_encode($pluginList);
if ($escapeHTML) {
$attributeValue = htmlentities($attributeValue);
}
return $attributeValue;
}
}Veamos las diferencias. Simplemente con definir los parámetros de manera explícita ya no es necesario verificar si existe el primer parámetro y qué tipo de valor contiene para saber cómo responder. El parser de PHP se encargará de lanzar un error si no se pasan por lo menos estos dos parámetros que hemos especificado.
Al definir $escapeHTML con un valor por default, no tuvimos que utilizar aquella condición else a manera de fallback en caso de que no se haya especificado un valor.
Ahora el propósito del código es más claro: se codifica a JSON la lista de plugins y sus opciones de configuración y se guarda el resultado en una variable. Si se ha pasado un valor para el parámetro $escapeHTML y el valor es true entonces hay que escapar los caracteres HTML de los datos que fueron codificados en JSON. Si no se especifica un valor para $escapeHTML entonces simplemente tomará el valor por default (true).
¡Pero no tan rápido! ¿Cómo sabemos que esta nueva implentación funciona? Bueno, no me miren a mi, veamos que dice PHPUnit. Para ello hay que ejecutar phpunit con el archivo de pruebas correspondiente a la nueva implementación, es decir, el archivo EncodeTest.php:
./vendor/bin/phpunit tests/EncodeTest.php
PHPUnit 5.5.4 by Sebastian Bergmann and contributors.
.. 2 / 2 (100%)
Time: 67 ms, Memory: 3.50MB
OK (2 tests, 8 assertions)Las pruebas funcionan, así que la nueva implementación no afecta el resultado esperado. Lo hemos logrado.
Como ven, a veces es fácil complicarse para solucionar algo muy sencillo. O simplemente queremos buscar la forma más corta o ingeniosa de programar, pero olvidamos que no es coincidencia que los lenguajes modernos se asemejen a un idioma usado por las personas (en este caso, y en su mayoría, inglés). El código debe ser comprensible para los desarrolladores, no para las computadoras. De lo contrario todos programaríamos en lenguaje binario.
Enlaces:
programación php phpunit tdd psr-2 psr-4