style="display:inline-block;width:728px;height:90px"
data-ad-client="ca-pub-5164839828746352"
data-ad-slot="7563230308">

Refactoring para entender mejor

TD;DR; Ver el antes y después hasta abajo

Ya tenía tiempo sin postear así que ahí va un ejemplo de refactoring y como se puede usar para entender mejor código legado.

No importa mucho lo que el código hace ( guarda recetas de cocina ), el objetivo es mostrar como un método que hace una sola cosa a la vez es más fácil de entender, que uno que hace varias a la vez.

Para leer mejor este post hay que buscar solamente el código que esta comentado ( que se esta eliminando o moviendo ) y el comentario que dice //<-- aqui que es el que lo reemplaza.

Espero les sea de ayuda

Inicio ( no lo lean, nomas véanlo )

        public int guardaRecetas() {

            if ( validaDatosRequeridos() ) {
                DataSet recetasDs = new DataSet();
                DataTable recetasDt = new DataTable();
                int usuarioLogeado;
                int tipoId;
                recetasDs.getColumns().add("id_usuario", Integer.TYPE );
                recetasDs.getColumns().add("desc_bloqueo", String.class );
                SomeUtility.changeMaping(recetasDs.getColumns(), SomeUtility.SOME_FLAG);
                recetasDs.getTables().add( recetasDt );

                for( SomeEntity receta : recetas ) {
                    DataRow dr = recetasDt.newRow();
                    dr.put( "id_usuario", receta.getIdUsuario() );
                    dr.put( "desc_receta", receta.getDescReceta() );
                    recetasDt.getRows().add( dr );
                }
                usuarioLogeado = getUser().getIdUsuario();
                tipoId = getUser().getTipoReceta();

                Peticion peticion = new Peticion(
                        this.claveUsuario,
                        this.pagina,
                        this.idCocina,
                        recetasDs.getXml(),
                        this.usuarioLogeado,
                        this.tipoId,
                        "Constante 1",
                        "Constante 2",
                        "Etc.",
                        MAGIC_NUMBER,
                        ETC,
                        ETC
                );
                int dummy = servicio.guardaReceta( peticion );
                return dummy;
                   
            } else {
                SomeStaticMessageClass.showMessage( Globals.DATOS_REQUERIDOS_MESSAGE,
                                                    Globals.MESSAGE_1_2_3_4_ME,
                                                    MessageType.OK,
                                                    MessageImage.INFO );                        
            }
            return 0;
        }

No esta nada mal, son 40 lineas aprox. y no luce muy complejo. Sin embargo lo primero que me salta es el if/else

¿Por que no mejor lo ponemos al principio?

        public int guardaRecetas() {

            if ( !/*<-- aqui*/ validaDatosRequeridos() ) {
                SomeStaticMessageClass.showMessage( Globals.DATOS_REQUERIDOS_MESSAGE,
                                                    Globals.MESSAGE_1_2_3_4_ME,
                                                    MessageType.OK,
                                                    MessageImage.INFO );                        

                return 0;
            }

            DataSet recetasDs = new DataSet();
            DataTable recetasDt = new DataTable();
            int usuarioLogeado;
            int tipoId;
            recetasDt.getColumns().add("id_usuario", Integer.TYPE );
            recetasDt.getColumns().add("desc_bloqueo", String.class );
            SomeUtility.changeMaping(recetasDs.getColumns(), SomeUtility.SOME_FLAG);
            recetasDs.getTables().add( recetasDt );

            for( SomeEntity receta : recetas ) {
                DataRow dr = recetasDt.newRow();
                dr.put( "id_usuario", receta.getIdUsuario() );
                dr.put( "desc_receta", receta.getDescReceta() );
                recetasDt.getRows().add( dr );
            }
            usuarioLogeado = getUser().getIdUsuario();
            tipoId = getUser().getTipoReceta();

            Peticion peticion = new Peticion(
                    this.claveUsuario,
                    this.pagina,
                    this.idCocina,
                    recetasDs.getXml(),
                    this.usuarioLogeado,
                    this.tipoId,
                    "Constante 1",
                    "Constante 2",
                    "Etc.",
                    MAGIC_NUMBER,
                    ETC,
                    ETC
            );
            int dummy = servicio.guardaReceta( peticion );
            return dummy;
            //} else { //<-- aqui
            //    SomeStaticMessageClass.showMessage( Globals.DATOS_REQUERIDOS_MESSAGE,  Globals.MESSAGE_1_2_3_4_ME, MessageType.OK, MessageImage.INFO );                        
            //}
            //return 0;
           
        }

Y así me concentro en lo que sí hace y no estoy pensando en lo que no hace ( el else )

Lo siguiente que me hace ruido es ese recetasDs, no conozco la clase ni sé para que sirve, pero sí sé que puedo crearla a parte, porque no se usa después.

        public int guardaRecetas() {

            if ( !validaDatosRequeridos() ) {
                SomeStaticMessageClass.showMessage( Globals.DATOS_REQUERIDOS_MESSAGE,
                                                    Globals.MESSAGE_1_2_3_4_ME,
                                                    MessageType.OK,
                                                    MessageImage.INFO );                        

                return 0;
            }

            DataSet recetasDs = new DataSet();
            int usuarioLogeado;
            int tipoId;
            DataTable recetasDt = createRecetasDataTable();//<-- aqui
            //recetasDt.getColumns().add("id_usuario", Integer.TYPE );
            //recetasDt.getColumns().add("desc_bloqueo", String.class );
            //SomeUtility.changeMaping(recetasDs.getColumns(), SomeUtility.SOME_FLAG);

            recetasDs.getTables().add( recetasDt );

            for( SomeEntity receta : recetas ) {
                DataRow dr = recetasDt.newRow();
                dr.put( "id_usuario", receta.getIdUsuario() );
                dr.put( "desc_receta", receta.getDescReceta() );
                recetasDt.getRows().add( dr );
            }
            usuarioLogeado = getUser().getIdUsuario();
            tipoId         = getUser().getTipoReceta();

            Peticion peticion = new Peticion(
                    this.claveUsuario,
                    this.pagina,
                    this.idCocina,
                    recetasDs.getXml(),
                    this.usuarioLogeado,
                    this.tipoId,
                    "Constante 1",
                    "Constante 2",
                    "Etc.",
                    MAGIC_NUMBER,
                    ETC,
                    ETC
            );
            int dummy = servicio.guardaReceta( peticion );
            return dummy;
        }
         // <-- aqui nuevo método
        private DataSet createRecetasDataTable() {
            DataTable recetasDataTable; = new DataTable();
            recetasDataTable;.getColumns().add("id_usuario", Integer.TYPE );
            recetasDataTable;.getColumns().add("desc_bloqueo", String.class );
            SomeUtility.changeMaping(recetasDataTable.getColumns(), SomeUtility.SOME_FLAG);
            return recetasDataTable;
        }

Al hacerlo también me doy cuenta que dos variables se declaran pero no se usan sino hasta más tarde y pueden declararse ahí:

        public int guardaRecetas() {

            if ( !validaDatosRequeridos() ) {
                SomeStaticMessageClass.showMessage( Globals.DATOS_REQUERIDOS_MESSAGE,
                                                    Globals.MESSAGE_1_2_3_4_ME,
                                                    MessageType.OK,
                                                    MessageImage.INFO );                        

                return 0;
            }

            DataSet recetasDs = new DataSet();
            //int usuarioLogeado;
            //int tipoId;
            DataTable recetasDt = createRecetasDataTable();
            recetasDs.getTables().add( recetasDt );

            for( SomeEntity receta : recetas ) {
                DataRow dr = recetasDt.newRow();
                dr.put( "id_usuario", receta.getIdUsuario() );
                dr.put( "desc_receta", receta.getDescReceta() );
                recetasDt.getRows().add( dr );
            }
            int usuarioLogeado = getUser().getIdUsuario(); //<-- aqui
            int tipoId         = getUser().getTipoReceta(); //<-- aqui

            Peticion peticion = new Peticion(
                    this.claveUsuario,
                    this.pagina,
                    this.idCocina,
                    recetasDs.getXml(),
                    this.usuarioLogeado,
                    this.tipoId,
                    "Constante 1",
                    "Constante 2",
                    "Etc.",
                    MAGIC_NUMBER,
                    ETC,
                    ETC
            );
            int dummy = servicio.guardaReceta( peticion );
            return dummy;
        }
        ..

Luego lo que quiero sacar es el for, por que siento que el método hace dos cosas a la vez.

¿Que tal si le creo su propio método?

        public int guardaRecetas() {

            if ( !validaDatosRequeridos() ) {
                SomeStaticMessageClass.showMessage( Globals.DATOS_REQUERIDOS_MESSAGE,
                                                    Globals.MESSAGE_1_2_3_4_ME,
                                                    MessageType.OK,
                                                    MessageImage.INFO );                        

                return 0;
            }

            DataTable recetasDt = createRecetasDataTable();

            DataSet recetasDs = new DataSet();
            recetasDs.getTables().add( recetasDs );

            addRecetasTo( recetasDt );//<-- aqui
            //for( SomeEntity receta : recetas ) {
            //    DataRow dr = recetasDt.newRow();
            //    dr.put( "id_usuario", receta.getIdUsuario() );
            //    dr.put( "desc_receta", receta.getDescReceta() );
            //    recetasDt.getRows().add( dr );
            //}
            int usuarioLogeado = getUser().getIdUsuario();
            int tipoId         = getUser().getTipoReceta();

            Peticion peticion = new Peticion(
                    this.claveUsuario,
                    this.pagina,
                    this.idCocina,
                    recetasDs.getXml(),
                    this.usuarioLogeado,
                    this.tipoId,
                    "Constante 1",
                    "Constante 2",
                    "Etc.",
                    MAGIC_NUMBER,
                    ETC,
                    ETC
            );
            int dummy = servicio.guardaReceta( peticion );
            return dummy;
        }
        ...
         // <-- aqui nuevo método
        private void addRecetasTo( DataTable dataTable ) {
       
            for( SomeEntity receta : recetas ) {
                DataRow dr = dataTable.newRow();
                dr.put( "id_usuario", receta.getIdUsuario() );
                dr.put( "desc_receta", receta.getDescReceta() );
                dataTable.getRows().add( dr );
            }
        }

Mhh aquí veo que el DataSet y el DataTable intervienen poco... y el data set solo se requiere para obtener el XML... lo dejo así por ahora y continuo con algo más evidente: el dummy. No hace nada así que lo ponemos "enlinea"

        public int guardaRecetas() {

            if ( !validaDatosRequeridos() ) {
                SomeStaticMessageClass.showMessage( Globals.DATOS_REQUERIDOS_MESSAGE,
                                                    Globals.MESSAGE_1_2_3_4_ME,
                                                    MessageType.OK,
                                                    MessageImage.INFO );                        

                return 0;
            }

            DataTable recetasDt = createRecetasDataTable();

            DataSet recetasDs = new DataSet();
            recetasDs.getTables().add( recetasDs );

            addRecetasTo( recetasDt );
            int usuarioLogeado = getUser().getIdUsuario();
            int tipoId         = getUser().getTipoReceta();

            Peticion peticion = new Peticion(
                    this.claveUsuario,
                    this.pagina,
                    this.idCocina,
                    recetasDs.getXml(),
                    this.usuarioLogeado,
                    this.tipoId,
                    "Constante 1",
                    "Constante 2",
                    "Etc.",
                    MAGIC_NUMBER,
                    ETC,
                    ETC
            );
            return servicio.guardaReceta( peticion );
            //int dummy = servicio.guardaReceta( peticion );//<-- aqui
            //return dummy;//<-- aqui
        }
        ...

Lo cual me lleva a darme cuenta de otra cosa, la petición no se usa para nada, entonces ponemos también "enlinea"

   
        public int guardaRecetas() {

            if ( !validaDatosRequeridos() ) {
                SomeStaticMessageClass.showMessage( Globals.DATOS_REQUERIDOS_MESSAGE,
                                                    Globals.MESSAGE_1_2_3_4_ME,
                                                    MessageType.OK,
                                                    MessageImage.INFO );                        

                return 0;
            }

            DataTable recetasDt = createRecetasDataTable();

            DataSet recetasDs = new DataSet();
            recetasDs.getTables().add( recetasDs );

            addRecetasTo( recetasDt );
            int usuarioLogeado = getUser().getIdUsuario();
            int tipoId         = getUser().getTipoReceta();

            return servicio.guardaReceta( new Peticion( //<-- aqui
                    this.claveUsuario,
                    this.pagina,
                    this.idCocina,
                    recetasDs.getXml(),
                    this.usuarioLogeado,
                    this.tipoId,
                    "Constante 1",
                    "Constante 2",
                    "Etc.",
                    MAGIC_NUMBER,
                    ETC,
                    ETC
            ));
            //return servicio.guardaReceta( peticion );//<-- aqui
        }
        ...

Lo que a su vez me enfoca en otras dos variables que no se usan y se pueden poner en linea tambien:

        public int guardaRecetas() {

            if ( !validaDatosRequeridos() ) {
                SomeStaticMessageClass.showMessage( Globals.DATOS_REQUERIDOS_MESSAGE,
                                                    Globals.MESSAGE_1_2_3_4_ME,
                                                    MessageType.OK,
                                                    MessageImage.INFO );                        

                return 0;
            }

            DataTable recetasDt = createRecetasDataTable();

            DataSet recetasDs = new DataSet();
            recetasDs.getTables().add( recetasDs );

            addRecetasTo( recetasDt );
            //int usuarioLogeado = getUser().getIdUsuario();
            //int tipoId         = getUser().getTipoReceta();

            return servicio.guardaReceta( new Peticion(
                    this.claveUsuario,
                    this.pagina,
                    this.idCocina,
                    recetasDs.getXml(),
                    getUser().getIdUsuario(),  //<-- aqui
                    getUser().getTipoReceta(), //<-- aqui
                    "Constante 1",
                    "Constante 2",
                    "Etc.",
                    MAGIC_NUMBER,
                    ETC,
                    ETC
            ));
        }
        ...

Listo, mucho mejor.

Y me sigue haciendo ruido ese DataSet y DataTable; solo se necesitan para sacar el XML. Pues bien, voy a crear un método para obtener ese XML

      public int guardaRecetas() {

            if ( !validaDatosRequeridos() ) {
                SomeStaticMessageClass.showMessage( Globals.DATOS_REQUERIDOS_MESSAGE,
                                                    Globals.MESSAGE_1_2_3_4_ME,
                                                    MessageType.OK,
                                                    MessageImage.INFO );                        

                return 0;
            }

            String xmlRecetas = getXmlParaRecetas(); //<--- aqui
           
            //DataTable recetasDt = createRecetasDataTable();
            //DataSet recetasDs = new DataSet();
            //recetasDs.getTables().add( recetasDs );

            //addRecetasTo( recetasDt );

            return servicio.guardaReceta( new Peticion(
                    this.claveUsuario,
                    this.pagina,
                    this.idCocina,
                    xmlRecetas,
                    getUser().getIdUsuario(),
                    getUser().getTipoReceta(),
                    "Constante 1",
                    "Constante 2",
                    "Etc.",
                    MAGIC_NUMBER,
                    ETC,
                    ETC
            ));
        }
        ...
         // <-- aqui nuevo método
        private String getXmlParaRecetas() {

            DataTable recetasDt = createRecetasDataTable();
            DataSet recetasDs = new DataSet();
            recetasDs.getTables().add( recetasDs );
            addRecetasTo( recetasDt );
            return recetasDs.getXml();
           
        }
        ...

Y claro, ese variable XML string puede ponerse en linea:

        public int guardaRecetas() {

            if ( !validaDatosRequeridos() ) {
                SomeStaticMessageClass.showMessage( Globals.DATOS_REQUERIDOS_MESSAGE,
                                                    Globals.MESSAGE_1_2_3_4_ME,
                                                    MessageType.OK,
                                                    MessageImage.INFO );                        

                return 0;
            }

            //String xmlRecetas = getXmlParaRecetas();

            return servicio.guardaReceta( new Peticion(
                    this.claveUsuario,
                    this.pagina,
                    this.idCocina,
                    getXmlParaRecetas(), //<-- aqui
                    getUser().getIdUsuario(),
                    getUser().getTipoReceta(),
                    "Constante 1",
                    "Constante 2",
                    "Etc.",
                    MAGIC_NUMBER,
                    ETC,
                    ETC
            ));
        }

Chachaaaan!

Ahora puedo ver de un solo vistazo, que hace mi método. Parece trivial ( y de hecho lo es ) y esa es la idea, que sea fácil de entenderlo.

El punto es qué el método solo haga una cosa ( guardar recetas ) y si para hacer esa cosa necesita hacer algunos otros pasos, dejárselos a otros métodos.

El riesgo es terminar con muchos métodos que a su vez hacen ruido en toda la clase, pero esto generalmente no es el caso ( a menos que se abuse ), siempre se puede desaparecer el método si es demasiado trivial.

Ejemplo ( que ya no puse ) el método sonDatosInvalidos() que era:

     if( sonDatosInvalidos() ) {
        ...
     }
     ...
     public void sonDatosInvalidos() {
         return !validaDatosRequeridos();
     }

Así que no lo puse.

¿Y esto para que?
El resultado no es un ( necesariamente ) código más chico, a veces incluso es más largo. Tampoco un código que sea más eficiente, si había algun comportamiento raro, lo sigue habiendo, no se esta cambiando lo que el código hace, sino su estructura. Esta es la importancia del Refactoring; ahora es un código más fácil de entender. Hay muchas menos variables que nomas hacen ruido y es más fácil de llegar y saber que hace en poco tiempo.

Antes/Después

Comparesé

Antes:

        public int guardaRecetas() {

            if ( validaDatosRequeridos() ) {
                DataSet recetasDs = new DataSet();
                DataTable recetasDt = new DataTable();
                int usuarioLogeado;
                int tipoId;
                recetasDs.getColumns().add("id_usuario", Integer.TYPE );
                recetasDs.getColumns().add("desc_bloqueo", String.class );
                SomeUtility.changeMaping(recetasDs.getColumns(), SomeUtility.SOME_FLAG);
                recetasDs.getTables().add( recetasDt );

                for( SomeEntity receta : recetas ) {
                    DataRow dr = recetasDt.newRow();
                    dr.put( "id_usuario", receta.getIdUsuario() );
                    dr.put( "desc_receta", receta.getDescReceta() );
                    recetasDt.getRows().add( dr );
                }
                usuarioLogeado = getUser().getIdUsuario();
                tipoId = getUser().getTipoReceta();

                Peticion peticion = new Peticion(
                        this.claveUsuario,
                        this.pagina,
                        this.idCocina,
                        recetasDs.getXml(),
                        this.usuarioLogeado,
                        this.tipoId,
                        "Constante 1",
                        "Constante 2",
                        "Etc.",
                        MAGIC_NUMBER,
                        ETC,
                        ETC
                );
                int dummy = servicio.guardaReceta( peticion );
                return dummy;
                   
            } else {
                SomeStaticMessageClass.showMessage( Globals.DATOS_REQUERIDOS_MESSAGE,
                                                    Globals.MESSAGE_1_2_3_4_ME,
                                                    MessageType.OK,
                                                    MessageImage.INFO );                        

            }
            return 0;
        }

Después:

        public int guardaRecetas() {

            if ( !validaDatosRequeridos() ) {
                SomeStaticMessageClass.showMessage( Globals.DATOS_REQUERIDOS_MESSAGE,
                                                    Globals.MESSAGE_1_2_3_4_ME,
                                                    MessageType.OK,
                                                    MessageImage.INFO );                        
                return 0;
            }

            return servicio.guardaReceta( new Peticion(
                    this.claveUsuario,
                    this.pagina,
                    this.idCocina,
                    getXmlParaRecetas(),
                    getUser().getIdUsuario(),
                    getUser().getTipoReceta(),
                    "Constante 1",
                    "Constante 2",
                    "Etc.",
                    MAGIC_NUMBER,
                    ETC,
                    ETC
            ));
        }
        private String getXmlParaRecetas() {

            DataTable recetasDt = createRecetasDataTable();
            DataSet recetasDs = new DataSet();
            recetasDs.getTables().add( recetasDs );
            addRecetasTo( recetasDt );
            return recetasDs.getXml();
           
        }
        private DataSet createRecetasDataTable() {
            DataTable recetasDataTable; = new DataTable();
            recetasDataTable;.getColumns().add("id_usuario", Integer.TYPE );
            recetasDataTable;.getColumns().add("desc_bloqueo", String.class );
            SomeUtility.changeMaping(recetasDataTable.getColumns(), SomeUtility.SOME_FLAG);
            return recetasDataTable;
        }

        private void addRecetasTo( DataTable dataTable ) {
       
            for( SomeEntity receta : recetas ) {
                DataRow dr = dataTable.newRow();
                dr.put( "id_usuario", receta.getIdUsuario() );
                dr.put( "desc_receta", receta.getDescReceta() );
                dataTable.getRows().add( dr );
            }
        }

Conclusión

Como se puede ver, hay tres nuevos métodos, pero incluso estos tres métodos son mucho muy especializados y fáciles de leer.

Lo que le hace falta a este ejemplo y algo forzoso!!! ( y peligroso si no se tiene ) es un set de pruebas automatizadas que corra antes de que empecemos a hacer el cambio y que siga corriendo después de haberlo hecho. Hacer esto sin pruebas es altamente riesgoso, tanto que mejor sería no hacerlo, porque se corre el riesgo de romper algo que funcionaba bien.

Espero que esto les sirva y les dejo una cita de Martin Fowler de su libro Refactoring, improving the desing of existing code


Any fool can write code that a computer can understand.
Good programmers write code that humans can understand. ~Martin Fowler

Ahhh verdad!!! :) :)

Saludos.

Comentarios

Opciones de visualización de comentarios

Seleccione la forma que prefiera para mostrar los comentarios y haga clic en «Guardar las opciones» para activar los cambios.
Imagen de ezamudio

mmmmm

Es que depende de qué buscas con el refactoring, supongo... por ejemplo, se supone que es mala práctica poner condiciones negadas, y lo primero que hiciste fue convertir tu if (algo) en if (!algo). Y luego, metiste un return en ese bloque, lo cual incrementa la complejidad ciclomática de tu método.

Lo de la complejidad ciclomática es una herramienta de medición y puede que te importe o no (a ti y a tus compañeros de equipo que lean ese código). Puede que les sea más claro pensar "si no son datos válidos entonces va a ocurrir esto" y ver el return y saber que eso es lo único que ocurre, y si no pues no se entra al bloque y ya. Habemos quienes nos parece más claro pensar "si los datos son válidos, pasará esto" y luego ver un else y saber que eso va a ocurrir en el caso contrario (los datos no fueron válidos).

Todo lo demás me parece muy bueno: partir el método grande en varios métodos más pequeños, para hacer más entendible esas partes de código y facilitar su modificación en el futuro. Solamente dejaría yo el if con la condición positiva. No soy un nazi de la complejidad ciclomática así que podría vivir con los dos returns, aunque se supone que la manera de atacar eso es tener una variable de retorno a la que le asignas 0 desde el inicio y en el if la modificas con el resultado de servicio.guardaReceta(). Algo así pues:

public int guardaRecetas() {
  int rval = 0;
  if ( validaDatosRequeridos()) {
    rval = servicio.guardaReceta(blabla);
  } else {
    showMessage(blabla);
  }
  return rval;
}

Para los que no sepan de qué carajos estoy hablando, la complejidad ciclomática es una medición inventada en los 70's que cuenta el número de rutas independientes de un código. Cuando mides las complejidad de un método, empiezas con una calificación de 1. Cada if en el código incrementa la complejidad en 1; cada return intermedio también la incrementa en 1. Los ciclos while y for también incrementan la complejidad. Esta medida es útil para saber cuántas pruebas hay que hacerle a un método; en el caso de guardaRecetas hay que hacer 2 pruebas, una donde se cumple la condición de validación y una donde no se cumple (se supone que habría que hacer 3 pruebas porque el return adicional incrementa la complejidad y la medida final es 3, pero ese bloque está muy simple; de todas maneras estoy contando mal porque hay que tomar en cuenta que se invocan los otros métodos uno de los cuales tiene un if etc).

Hay herramientas que calculan la complejidad ciclomática del código; plugins para Eclipse como checkstyle, findbugs, etc y hay para distintos lenguajes e IDE's.

Wow

¡¡¿No mams cómo le haces para saber tantos conceptos complejidad ciclomatica?!! Que razón tuvo Socrates Sólo sé que no sé nada

Buen post

Imagen de ezamudio

estudiando

En esta carrera no puedes dejar de estudiar y aprender nunca.

Me queda claro

Me queda claro, solo que cada que leo tus comentarios me doy cuenta que me hace falta mucho, muchisimos mas conocimientos.

Que chido por ti, siento envidia de la buena.

\m/

Claro que nos importa la

Claro que nos importa la complejidad ciclomática, pero también hay que tomarla con criterio y es aquí donde la experiencia gana.

Estoy deacuerdo con lo de los returns, en especial cuando están en medio del código:

// difícil de entender...
int  algo() {
     blah();
     blah();
     blah();
     blah();
     if ( cond ) {
         return 1;
     }
     blop();
     blop();
     blop();
     return 0;
}    

Pero si tenerlo al principio ( como GuardClause ) simplifica el entendimiento del código me parece que es más beneficioso. El cálculo de la complejidad no toma en cuenta este criterio ( hasta donde recuerdo )

Por lo que lo siguiente sigue siendo sencillo de entender:

int algo() {
    if ( invalido ) {
        return -1;
    }
    ahoraSi();
    ahoraSi();
    ahoraSi();
    return 0;
}

Relacionado: Remplazar condiciones anidadas con clausulas de guarda

Sobre las condiciones negadas también estoy de acuerdo, en especial cuando niegan una negación ( una doble negación es una afirmación )

// nap...
if ( !  esInvalido )  {
}
// yeap
if ( esValido ) {
}

Pero de nuevo, negar una afirmación es bastante natural. En el cálculo de la complejidad no podría evaluar si se esta negando una negación o una afirmación.

Es por esto que el concepto de los masitros artesanos el gana a los procesos.

:)

Imagen de ezamudio

negaciones

La complejidad ciclomática no se afecta si un if evalúa una negación, solamente dije que eso es considerado mala práctica por algunos individuos (o en algunos casos, etc etc).

Imagen de bferro

Reglas de refactoring

Algunos comentarios.
Extraer un fragmento de código de un método y encapsularlo en otro método (La regla que Fowler le llama Extract Method y más vieja que Matusalén) es una buena regla cuando realmente se trata de un fragmento de código. Yo no crearía un método para simplemente encapsular el   for( SomeEntity receta : recetas ) { ... } , pues realmente el for statement es "autocontenido" y IMHO no constituye un fragmento.
Con respecto a comenzar el código de la forma que propone Oscar, lo haría si estoy seguro que esa porción de código representa una precondición necesaria para la ejecución normal del método.
También cuestionaría lo de no usar variables para almacenar resultados temporales que más adelante van a ser usados. En muchas ocasiones, usarlas conduce a un código más legible e igualmente productivo.

Imagen de Sr. Negativo

TDD y el Refactoring

@OscarRyz
El TDD y el Refactoring si que son difíciles, a veces por cumplir con el tiempo de entrega de un proyecto no se llega a usar :(

Lo haces parecer fácil.

@ezamudio
No sabía nada de la complejidad ciclomática ... me falta estudiar :O !!!

Imagen de JavaMan

Recordando los inicios...

Este POST me hace recorda hace algo de mas de 10 anios atras cuando aprendi me iniciaba a programar asi a la mala, recuerdo que mi profesor no enniaba en dicho ciclo solo temas basicos de programacion pero a modod estructural osea todo en caida, recuerdo mi primer proyecto academico fue de esa manera y las lineas de codigo se recontra repetian y eran infinitas pero ahi uno nota el cambio ya que en proyectos academicos posteriores ya permitian el manejo de metodos void, con return y ahi es cuando uno se da cuenta de la impotancia de la reutilizacion y la refactorizacion que hace que un codigo sea mas entendible, mantenible, reutilizable, etc ..y con miles de lineas de codigo menos ..!

Saludos.

Imagen de paranoid_android

Tema muy bueno.

Seguramente existen muchas formas de hacer un refactoring y muchos criterios que se aplican con experiencia.

Sin duda es muy bueno hacer limpieza de código sobre todo desde el inicio de la aplicación hacerlo en mantenimiento es una labor titánica.

Mi recomendación es que no te olvides de la traza metas líneas de logger antes y dentro de los ifs. Además muy importante en el catch de las excepciones como todas las demás opiniones con criterio para no pegarle al rendimiento.

En mi opinión tampoco me gusta la recursividad suele ser compleja.

        public int guardaRecetas() {
            logger.info("Ejecutando guardaRecetas() ");
            if ( !validaDatosRequeridos() ) {
               logger.info("guardaRecetas() datos no validos");
                SomeStaticMessageClass.showMessage( Globals.DATOS_REQUERIDOS_MESSAGE,
                                                    Globals.MESSAGE_1_2_3_4_ME,
                                                    MessageType.OK,
                                                    MessageImage.INFO );                        
                return 0;
            }

Excepto que luego vas a tener

Excepto que luego vas a tener un archivo de log que dice muchísimas cosas y ninguna de ellas interesante.

Por ejemplo luego esos logs terminan viendose así:

Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 
Ejecutando guardarRecetas() 

No ví que alguien mencionara la recursividad.. :-?

Imagen de paranoid_android

Pensar en reversa

Es una mera recomendación o sugerencia solo es una aportación. (Estamos chupando tranquilos jajaja)

La siguiente línea puede que interpretara mal parece un tipo de recursividad, puede no serlo desde luego.

return servicio.guardaReceta( new Peticion(

Si la única funcionalidad de tu sistema es Guardar Recetas en efecto tu log se verá como lo dices.
Depende de las necesidades de cada sistema como lo mencioné antes usando el criterio.

Después de liberar el aplicativo si tu no desarrollaste. ¿Cómo haces para descubrir un problema sin ver el log?

Sé que hay muchas necesidades distintas dependiendo el sector, hay quienes están acostumbrados a solo desarrollar y nunca necesitan dar mantenimientos o crear nuevas funcionalidades.

Hay otros que tenemos aplicativos con miles de líneas donde muchos desarrolladores tocan el código a lo largo de muchos años.

En esos casos el proceso es al revés comienza uno por el log y termina en el código.
Las partes más feas son las que no tienen traza normalmente donde se pierden los valores. Y hay que lidiar con la complejidad ciclomática donde después que modificas muchas veces el código no entra en la condición esperada.

Imagen de paranoid_android

Leyendas urbanas de la programación

Más Refactoring y menos llanto

Me contaron que el primo del tío de un amigo ( jajaja ) tenía que lidiar con código lleno de

Ifs ¿Cuantas condiciones puede haber?

if (cond1 && cond2 ){
}else if (cond3 || cond4 ){
}else if ((cond3 || cond4) && cond 5){
... multiplicalo por el numero que pensaste
}

Logica aglutinante sin separación de capas beans + daos + logica de controles + hilos + nuevas funcionalidades

Logica transversal en listeners universales llenos de switch case, ifs (Si el usuario selecciona cualquier acción)

Por si querian regresar a la funcionalidad anterior
if (true) o if (false)

Codigo muerto
/*
if (aaaa){
linea x 100
}
*/

variables raras
xyz10

Atinale al try catch
try{
...
try{
...
try{
...
etc..

Errores sileciosos
try{
}catch( Exception e){}

Falsas pistas en el log:
Exception error en:
Pila
...

Semántica capciosa
Bajo la condición 1 este campo en base de datos guarda X dato pero bajo la condición 2 este dato es otra cosa.

A pesar de tanta belleza el código funciona y cumple con lo que se pidió.

Así podemos seguir con un montón de prácticas,
El instinto dice "podría expresarse de otra forma" pero la voz de la conciencia dice "si funciona no lo toques". jajaja

style="display:inline-block;width:728px;height:90px"
data-ad-client="ca-pub-5164839828746352"
data-ad-slot="7563230308">