1

Quiero refactorizar el siguiente método y dejar un código limpio.

El método en cuestión recibe una cadena. Según qué cadena sea se ejecuta un método. Este método a ejecutar puede ser de un padre y una madre diferente al resto.

class MiClase : public QObject
{
    void metodoParaRefactorizar(QString cadena)
    {
        QJsonDocument jDoc;
        bool instructionFound = false;

        if(cadena.compare("instruccionUno") == 0)
        {
            jDoc = this->objClassOne->metodoPublico();
            instructionFound = true;
        }
        else if(cadena.compare("pepito") == 0)
        {
            jDoc = this->objeClassTwo->getList();
            instructionFound = true;
        }
        else if(cadena.compare("cadenaMolona") == 0)
        {
            jDoc = this->objeClassServer->getName();
            instructionFound = true;
        }
        else if(cadena.compare("otraCadenaDiferente") == 0)
        {
            jDoc = this->objeClassHtml->ejecutaScript();
            instructionFound = true;
        }
        .... 
        // Muchos más 'else if'
        ....
        else if(cadena.compare("ultima") == 0)
        {
            jDoc = this->objeClassLast->closeAll();
            instructionFound = true;
        }

        if(instructionFound)
        {
            sendAnswer(jDoc);
        }
    }
}

He estado buscando por internet y he visto que la solución sería el polimorfismo. Pero en este caso los métodos a ejecutar pueden ser de clases muy diferentes.

Me huele muy mal este código. Lo veo infumable y difícil de mantener. Produce mucho acoplamiento. Se que el problema es más de diseño que otra cosa. Me gustaría aprender a hacerlo mejor.

¿Qué solución le podría dar? ¿Como convierto esto en código limpio y fácil de mantener?

5
  • puedes usar switch -> case en vez de else if
    – lois6b
    Commented el 21 oct. 2016 a las 10:11
  • 1
    @lois6b Si no fuese por un detalle tonto y es que no puedes usar switch al haber un string...
    – eferion
    Commented el 21 oct. 2016 a las 10:15
  • @eferion ah si? es cosa de c++ ?
    – lois6b
    Commented el 21 oct. 2016 a las 10:17
  • Es cosa de C, de C++ y de algún otro lenguaje. Si vienes de Java o de .Net es normal que te extrañe, pero en los lenguajes de más bajo nivel hay cosas que no se pueden hacer tal cual :)
    – eferion
    Commented el 21 oct. 2016 a las 10:27
  • @eferion, haha gracias por la informacion. Si, en Java y C# nunca tuve problema y supuse que para C++ tampoco habría.
    – lois6b
    Commented el 21 oct. 2016 a las 10:29

3 respuestas 3

4

Una posible solución pasa por indizar todas las posibles opciones en un mapa. Cada posible opción va a asociada a una lambda con el código a ejecutar en cada caso.

Una vez generado el mapa, saber si una opción es válida es tan sencillo como consultar la existencia de la clave. Si la clave existe entonces ejecutamos la operación asociada a la misma.

void metodoRefactorizado(QString cadena)
{
    static std::map<QString,std::function<QJsonDocument()>> opciones
    {
      std::make_pair("instruccionUno",[&]() { return this->ObjClassOne->metodoPublico(); }).
      std::make_pair("pepito",[&]() { return this->objClassTwo->getList(); })
    };

    auto it = opciones.find(cadena);
    if( it != cadena.end() )
    {
      QJsonDocument jDoc = it->second();
      sendAnswer(jDoc);
    }
}

Por supuesto puedes mover, por legibilidad, la función de inicialización a otro método. Gracias a la sintaxis Move (disponible a partir de C++11) la pérdida de rendimiento (si te preocupa) es despreciable.

std::map<QString,std::function<QJsonDocument()>> InitMap()
{
  return std::map<QString,std::function<QJsonDocument()>>
    {
      std::make_pair("instruccionUno",[&]() { return this->ObjClassOne->metodoPublico(); }).
      std::make_pair("pepito",[&]() { return this->objClassTwo->getList(); })
    };
}

void metodoRefactorizado(QString cadena)
{
    static std::map<QString,std::function<jDoc()>> opciones = InitMap();

    auto it = opciones.find(cadena);
    if( it != cadena.end() )
    {
      sendAnswer(it->second());
    }
}

Un saludo.

9
  • A ver si me aclaro un poco. Me estás proponiendo que cree un contenedor 'map' donde asocio la cadena con un método concreto. ¿No?
    – Jonathan
    Commented el 21 oct. 2016 a las 11:01
  • Exacto. El mapa, a diferencia de una secuencia de if-else, no realiza una búsqueda lineal, luego el tiempo de búsqueda es menor. Por otro lado te estás ahorrando la secuencia interminable de if-else.
    – eferion
    Commented el 21 oct. 2016 a las 11:07
  • Ok, genial lo voy pillando. Poco a poco pero lo voy viendo.
    – Jonathan
    Commented el 21 oct. 2016 a las 11:09
  • La segunda parte que me gustaría entender un poco mejor y donde me pierdo un poco. No entiendo por qué hay 2 'return' anidados. Entiendo que cada elemento es una cadena y una función lambda que devuelve el resultado de ejecutar la instrucción correspondiente. Pero... ¿por qué lo metes dentro de este 'return'? return std::map<QString, std::function<jDoc()>> Se podría definir fuera los elementos del map, ¿no?¿Cada vez que ejecuto opciones.find(cadena) busca la cadena, ejecuta la función lambda correspondiente y... ¿la asigna a it? ¿de dónde viene it->second()? En esa parte me pierdo...
    – Jonathan
    Commented el 21 oct. 2016 a las 11:20
  • lo que hay dentro de std::make_pair es una lambda que no es más que una función embebida en el código (disponible desde C++11). La función de esa lambda es devolver un QDomDocument, por lo que necesita su propio return.
    – eferion
    Commented el 21 oct. 2016 a las 11:22
0

Una solución elegante podría ser:

  • Construyes un array con todas las cadenas a comparar
  • Construyes un array de funciones y asignas a cada elemento la función que corresponde a la cadena que tinene el mismo índice.
  • Defines una función que retorne el índice si la cadena existe.
  • Finalmente ejecutas la función según el índice devuelto.

Será mucho más fácil de mantener y entender que llenando una función de if-else.

0

Tu función metodoParaRefactorizar puede ser resumida en estas líneas:

void metodoParaRefactorizar(std::string s)
{
    auto found = callbacks.find(s);
    bool instructionFound = false;
    QJsonDocument jDoc;

    if ((instructionFound = (found != callbacks.end())))
    {
        jDoc = (this->*found->second)();
    }
}

Pero eso significa que hemos pasado la complejidad a otro lado. Tal y como ha sugerido mcNets puedes usar callbacks. Para que el código anterior funcione debes hacer algo parecido a esto1:

class MiClase
{
    struct A { QJsonDocument metodoPublico() { return {}; } };
    struct B { QJsonDocument getList() { return {}; } };
    struct C { QJsonDocument getName() { return {}; } };
    struct D { QJsonDocument closeAll() { return {}; } };

    using callback = QJsonDocument(MiClase::*)();

    A a; B b; C c; D d;

    QJsonDocument metodoPublico() { return a.metodoPublico(); }
    QJsonDocument getList()       { return b.getList(); }
    QJsonDocument getName()       { return c.getName(); }
    QJsonDocument closeAll()      { return d.closeAll(); }

    const std::map<std::string, callback> callbacks =
    {
        {"instruccionUno",      &MiClase::metodoPublico},
        {"pepito",              &MiClase::getList},
        {"cadenaMolona",        &MiClase::getName},
        {"otraCadenaDiferente", &MiClase::closeAll},
    };

    void metodoParaRefactorizar(std::string s)
    {
        auto found = callbacks.find(s);
        bool instructionFound = false;
        QJsonDocument jDoc;

        if ((instructionFound = (found != callbacks.end())))
        {
            jDoc = (this->*found->second)();
        }
    }
};

Como observarás es un poco incómodo pues debemos duplicar llamadas a las funciones de sub-objetos mediante funciones análogas en el objeto principal. Esto es necesario para homogeneizar todas las llamadas al tipo callback (puntero a función de MiClase que devuelve QJsonDocument y no recibe nada).

Sin embargo tiene la ventaja que (desde mi punto de vista) mantener las callbacks es muy limpio e intuitivo:

const std::map<std::string, callback> callbacks =
{
    {"instruccionUno",      &MiClase::metodoPublico},
    {"pepito",              &MiClase::getList},
    {"cadenaMolona",        &MiClase::getName},
    {"otraCadenaDiferente", &MiClase::closeAll},
};

Añadir o eliminar cadenas-funciones es muy sencillo en el mapa que asocia cada cadena con la función que debe ser llamada.

Tu Respuesta

By clicking “Publica tu respuesta”, you agree to our terms of service and acknowledge you have read our privacy policy.

¿No es la respuesta que buscas? Examina otras preguntas con la etiqueta o formula tu propia pregunta.