FORCÉN

Refactoring: utiliza los métodos adecuados

Written by

Forcén

in

El método que tenemos hoy en la mesa de autopsias pretende asegurar que siempre haya una opción seleccionada en un grupo de checkboxes, en caso de que el usuario decida deseleccionar todas. La razón de forzar que haya siempre una opción marcada es evitar que al código que usará la selección del usuario no le de un pasmo porque no comprueba (aunque debería) dicha selección.

 

Check_Selection: function () {
    var checks = $(“#group1 input");
    var checked = [];
    var nonechecked = true;
    
    for (var i = 0; i < checks.length; i++)
    {
        if (checks.eq(i).attr("checked"))
        {
            checked.push(checks.eq(i));
            nonechecked = false;
        }
    }
    if (nonechecked)
    {
        checks.eq(0).attr("checked", true).checkboxradio("refresh");
    }
}

Primero, como maniático que se es, un par de cambios “estéticos”. El método, efectívamente, comprueba si hay una selección, pero por aquello de que un método haga sólo una cosa y su nombre describa lo que hace, es más apropiado llamarlo forceOneSelection.

“Ajunto” las variables al inicio del método y cambio los nombres según la norma de estilo de la casa (que se ha escrito, como todos sabemos, para ser ignorada). Revisando las variables vemos que se crea un array, checked, en el que se añaden elementos. Pero luego no se usa. Un despiste lo tiene cualquiera, así que variable fuera.

El WTF comienza más adelante. Se comprueba si un checkbox está seleccionado usando el atributo “checked”. Usar attr() para estas cosas en jQuery es la manera de NO hacerlo desde la versión 1.6. Y este código usa la versión 1.7. La forma correcta sería usar prop(). Pero hay una forma aún más correcta. Preguntar por el pseudo-selector :checked.

Ya que estamos rodeados de jQuery, cambio el for por un $.each. Eso ya es manía de cada uno. El método quedaría así.

 

forceOneSelection: function () {
    var $Checks = $("#group1 input"),
        boolNoneChecked = true;
    
    $Checks.each(function (i, objCheck) {
        if ($(objCheck).is(':checked')) {
            boolNoneChecked = false;
            return false;
        }
    });
    if (boolNoneChecked) {
        $Checks.eq(0).prop('checked', true).checkboxradio('refresh');
    }
}

 

Por último, una vez el código funciona (y nunca antes), se optimiza. En lugar de pedir todos los inputs y preguntar uno a uno si están seleccionados, le pedimos amablemente a jQuery que nos devuelva sólo aquellos que están seleccionados.

 

forceOneSelection: function () {
    var $Checks = $("#group1 input:checked");

    if (!$Checks.length) {
        $Checks.eq(0).prop('checked', true).checkboxradio('refresh');
    }
}

More posts