Refactoring condiciones

[Carlos Admirador]

 Cómo manejar más eficientemente condiciones en ForEach, LINQ....   ?

Tengo este código legacy:

        var target = new List<string>();

        var prefix = adManager.EnvironmentPrefix;
        var groupsAD = adManager.GetADGroups(username);
        groupsAD.ForEach(g => {
            Console.WriteLine(g.DistinguishedName);
            var listaDN = adManager.GetDistinguishedNameItems(g.DistinguishedName);
            var isCN = listaDN[0].Name == "CN";

            var isTargetGroupVPN = listaDN[0].Value == prefix + "VPN_Usuarios"
                && listaDN[1].Name == "CN" && listaDN[1].Value == "Users";

            var isTargetGroupComunicados = listaDN[0].Value.StartsWith(prefix + "Usuarios COMUNICADOS ")
                && listaDN[1].Name == "CN" && listaDN[1].Value == "Users";

            var isTargetGroupCampañas = listaDN[0].Value.StartsWith(prefix + "Campaña ")
                && (listaDN[1].Name == "OU" && listaDN[1].Value == "CAMPAÑAS"
                    || listaDN[2].Name == "OU" && listaDN[2].Value == "CAMPAÑAS");

            var valid = isCN && (isTargetGroupVPN || isTargetGroupComunicados || isTargetGroupCampañas);
            if (!valid) return;
            target.Add(g.Name);
        });

        target.ForEach(grupo =>
        {
            adManager.DeleteUserFromGroup(username, grupo);
        });

[Carlos Peix]

Hola Carlos,

Yo veo muchas oportunidades de refactoring en ese código, entendiendo refactoring como hacer el código más fácil de entender para un humano.

Por otro lado, preguntás como hacerlo más eficiente, entonces no estoy seguro de entender el pedido. Más eficiente en que sentido?

Por último, en cualquiera de los dos casos, tenés pruebas sobre ese código que te permitan saber inmediatamente si los cambios que hagas introducen errores? Si no tenés pruebas, lo primero que haría es escribir suficientes pruebas sobre el código como está.

Saludos

----------------------------------
Carlos Peix

Móvil/Whatsapp: +54 911 4406 7571

--
Has recibido este mensaje porque estás suscrito al grupo "AltNet-Hispano" de Grupos de Google.
Para cancelar la suscripción a este grupo y dejar de recibir sus mensajes, envía un correo electrónico a altnet-hispan...@googlegroups.com.
Para ver esta conversación en el sitio web, visita https://groups.google.com/d/msgid/altnet-hispano/09a0f955-f494-4bad-9b3c-a74d0e539875n%40googlegroups.com.

[Carlos Admirador]

Gracias, me interesa  refactoring  para hacer el código más legible y mantenible.  Eficiente (quería decir, performance) en mi caso ahora es despreciable.

A nivel refactoring, se pueden mejorar la pate de escribir condiciones y que sea más legible?

[Carlos Peix]

Bien,

Asumiendo que tenés pruebas que cubren este código (me imagino entre 10 y 50 pruebas), entonces comenzaría por el refactoring Extract Method en cuatro casos, al menos. Por ejemplo:

var isTargetGroupVPN = listaDN[0].Value == prefix + "VPN_Usuarios"
                && listaDN[1].Name == "CN" && listaDN[1].Value == "Users";

lo cambiaría a:

var isTargetGroupVPN = IsTargetGroupVPN(listaDN, prefix);

Agregando el método

private bool IsTargetGroupVPN(??? listaDN, string prefix) {

    return listaDN[0].Value == prefix + "VPN_Usuarios"

                && listaDN[1].Name == "CN" && listaDN[1].Value == "Users";

}

Luego de algunos pasos, te quedaría:

var valid = IsCN(listaDN) && (IsTargetGroupVPN(listaDN, prefix) || isTargetGroupComunicados(listaDN, prefix) || isTargetGroupCampañas(listaDN, prefix));

Que a su vez podrías extraer como método. En ese caso y haciendo mucha futurología, quizás te queda algo así:

groupsAD.ForEach(g => {

    if (Valid(adManager.GetDistinguishedNameItems(g.DistinguishedName), prefix))

        target.Add(g.Name);

});

No conozco el dominio para sugerir nombres, pero me suena que Valid() podría tener un mejor nombre.

Si llegás a ese punto, quizás podrías reemplazar el ForEach por una operación de filtro, por ejemplo

target = groupsAD.Where(g => Valid(adManager.GetDistinguishedNameItems(g.DistinguishedName), prefix));

Es difícil avanzar mucho en un refactoring sin ver como va quedando el código y sin tener datos de contexto (por ejemplo, de que estructura mayor forma parte este bloque de código) y, sobre todo, sin tener el feedback del entorno de desarrollo y de la ejecución de las pruebas.

El resultado de los pasos que mencioné, sería, más o menos (buscaría un mejor nombre para el método Valid()):

groupsAD.Where(g => Valid(adManager.GetDistinguishedNameItems(g.DistinguishedName),  adManager.EnvironmentPrefix )).ForEach(grupo =>

    {
        adManager.DeleteUserFromGroup(username, grupo);

    });

Un saludo

----------------------------------
Carlos Peix

Móvil/Whatsapp: +54 911 4406 7571

On Mon, Sep 12, 2022 at 3:23 AM Carlos Admirador <admirado...@gmail.com> wrote:

Gracias, me interesa  refactoring  para hacer el código más legible y mantenible.  Eficiente (quería decir, performance) en mi caso ahora es despreciable.

A nivel refactoring, se pueden mejorar la pate de escribir condiciones y que sea más legible?

--

Has recibido este mensaje porque estás suscrito al grupo "AltNet-Hispano" de Grupos de Google.
Para cancelar la suscripción a este grupo y dejar de recibir sus mensajes, envía un correo electrónico a altnet-hispan...@googlegroups.com.

Para ver esta conversación en el sitio web, visita https://groups.google.com/d/msgid/altnet-hispano/2e7b0c8f-41bc-4335-bb2d-cedd2a9bf016n%40googlegroups.com.

[Carlos Admirador]

Muchas gracias Carlos, queda más claro así.
El contexto: se obtiene una lista de grupos de un usuario del directorio activo, y se quieren filtrar algunos grupos consultando su "DistinguishedName".

Veo que el refactoring es muy amplio:
en qué clase pongo una constante?
una clase de 500 líneas indica un codesmell, no tiene responsabilidad única ?

un método de más de 20 líneas  indica un codesmell, no tiene responsabilidad única ?
pasar como parámetro a un método el objeto completo, o las 3 propiedades del  objeto que utilizo?

Alguna página con ejemplos reales de Refactoring?

O sino este foro (stackoverflow) de "code reviews"
https://codereview.stackexchange.com/?tags=c%23

[Carlos Peix]

On Tue, Sep 13, 2022 at 4:54 AM Carlos Admirador <admirado...@gmail.com> wrote:

Veo que el refactoring es muy amplio:

Si, lo es. Creo que empezando a leer y, sobre todo, a hacer refactoring, encontrarás las respuestas a las preguntas que dejás abajo.

 

en qué clase pongo una constante?
una clase de 500 líneas indica un codesmell, no tiene responsabilidad única ?

un método de más de 20 líneas  indica un codesmell, no tiene responsabilidad única ?
pasar como parámetro a un método el objeto completo, o las 3 propiedades del  objeto que utilizo?

Alguna página con ejemplos reales de Refactoring?

O sino este foro (stackoverflow) de "code reviews"
https://codereview.stackexchange.com/?tags=c%23

--
Has recibido este mensaje porque estás suscrito al grupo "AltNet-Hispano" de Grupos de Google.
Para cancelar la suscripción a este grupo y dejar de recibir sus mensajes, envía un correo electrónico a altnet-hispan...@googlegroups.com.

Para ver esta conversación en el sitio web, visita https://groups.google.com/d/msgid/altnet-hispano/150b3e82-db78-4788-a236-7c9b3afce59fn%40googlegroups.com.