Cum s-ar putea reduce complexitatea ciclomatică?

Mi s-a întâmplat de multe ori să am câteva blocuri decizionale (fie în if, fie în blocuri switch/case) ce se întindeau pe mai multe ramuri (5-6, poate chiar mai multe). Și asta și în PHP și în JS.

Vorbind cu @andySF, am ajuns la concluzia că și el are aceeași problemă:

public static string MergeRegims(string newEntry, string entry)
{
    if (String.IsNullOrEmpty(newEntry) & String.IsNullOrEmpty(entry))
        return null;
    if (String.IsNullOrEmpty(newEntry))
        return entry;
    if (String.IsNullOrEmpty(entry))
        return newEntry;
    if (newEntry == entry)
        return newEntry;
    if (newEntry.ToUpper().Contains(RegimConventions.MDR) | entry.ToUpper().Contains(RegimConventions.MDR))
        return RegimConventions.MDR;
    if (newEntry.ToUpper().Contains(RegimConventions.INDV) | entry.ToUpper().Contains(RegimConventions.INDV))
        return RegimConventions.INDV;
    if (newEntry.ToUpper().Contains(RegimConventions.RII) | entry.ToUpper().Contains(RegimConventions.RII))
        return RegimConventions.RII;

 
    return null;
}

Cum s-ar putea proceda în acest caz?

1 Like
  1. Primele 4 conditii le-as abstractiza intr-o functie returnExisting, care sa iti intoarca prima valoare ne-nula || empty din parametrii invocati:
    if (String.IsNullOrEmpty(newEntry) & String.IsNullOrEmpty(entry))
        return null;
    if (String.IsNullOrEmpty(newEntry))
        return entry;
    if (String.IsNullOrEmpty(entry))
        return newEntry;
    if (newEntry == entry)
        return newEntry;
  1. Pentru urmatoarele 4 conditii se poate observa un pattern: daca oricare din cele 2 string-uri transformate cu uppercase contine un ‘regim’, intoarce acel regim. Functia ar primi 3 parametrii: primii 2 sunt cele 2 string-uri, al 3-lea parametru sa fie regim-ul.

Asta as abstractiza-o intr-o functie: containsRegim.

  1. In functia originala as apela functiile de la 1) && 2).
2 Likes

Cred că containsRegim nu trebuie să aibe trei parametri. E suficient return ReturnExisting(newEntry, entry) ?? ContainsRegim(newEntry, entry);. Dar asta să fie soluția pentru complexitatea blocurilor decizionale? Să le abstractizăm în funcții cu denumire mai clară?

1 Like

Eu simt o mica confuzie aici. Vorbim de complexitatea ciclomatica a cui? A metodei sau a clasei?

Daca logica impune evaluarea tuturor conditiilor la nivel de clasa, in afara de sugestiile lui @alessioalex se pot face putine. Si de fapt, nici sugestiile lui nu reduc complexitatea ciclomatica a clasei per-se. Mai degraba le comaseaza, dar deciziile luate tot atatea sunt.

Putem in schimb sa vorbim mult mai usor de complexitatea ciclomatica a metodei MergeRegims. Caz in care solutia clasica este extragerea cailor decizionale in cate o metoda. Asta nu pare a avea mare efect, dar hai sa analizam problema.

Sa testezi metoda MergeRegims asa cum este ea enuntata este in cel mai fericit caz dificila. Ajuta un pic faptul ca din fiecare IF se face un return, asta mai reduce numarul cazurilor si a complexitatii ciclomatice. Un exemplu mai elocvent ar fi cand nu se face nici un return, decat la final. O astfel de metoda ar avea complexitate maxima.

Acum, daca extragem fiecare decizie in cate o metoda, vom avea vreo 7-8 metode. Fiecare va avea complexitatea 2. Ar fi nevoie de doar 2 teste pentru fiecare metoda, adica 16 in total. Metoda MergeRegims va avea zero decizii de luat, deci va avea complexitatea 1. Nu va face altceva decat delegare. Un simplu test ca se cheama metodele corecte va fi suficient. Complexitate 17.

In cazul curent, complexitatea ciclomatica a metodei curente, adica numarul de cai pe care o poate lua, se poate calcula prin adunarea tuturor conditiilor ce trebuie luate sa iasa metoda pe fiecare din return-uri in parte. Nu fac calculul acum, dar se vede cu ochiul de sticla ca e mare numarul. Ceva de genul 1 + 2 + 3 + 4 + 5 + 6 + 7 = 28 (posibil sa mai fie si + 8). Oricum am redus de la 28 la 16 + 1 = 17.

Daca nu se face return din fiecare if, complexitatea creste exponential. Nu il mai calculez acum, dar va dati seama la ce ma refer.

2 Likes

În acest caz? A metodei.

Asta a fost și ideea mea, doar că nu știu ar trebui procedat mai departe. Adică dacă extragem fiecare condiție într-o metodă, din metoda MergeRegims cum ar trebui apelată fiecare metodă în parte?

public static string MergeRegims(string newEntry, string entry)
{
    // <<<<<<<<<<<<<<<<<<<<<<<
    // ???????????????????????
    // <<<<<<<<<<<<<<<<<<<<<<<
}

public static string invalidParams(string newEntry, string entry)
{
  if (String.IsNullOrEmpty(newEntry) & String.IsNullOrEmpty(entry))
      return null;
}

public static string invalidEntry(string newEntry, string entry)
{
  if (String.IsNullOrEmpty(newEntry))
        return entry;
}

In primul rand … dupa primele 4 conditii functia returneaza ceva obligat si nu vad cum ar putea ajunge la conditia 5 dar …

Eu as face asa in JS de ex:

function MergeRegims(string newEntry, string entry) {
   
    //partea 1
    for (i in args) {
        if (String.IsNullOrEmpty(args[i])) {
           return args[i];
       }
    }

    return null;

   //partea 2
   var compare_var = {
       newEntry : newEntry.ToUpper(),
       entry : entry.ToUpper()
   };

   var compare_val = {
      MDR : RegimConventions.MDR,
      INDV: RegimConventions.INDV,
      RII: RegimConventions.RII
   };

   for (i in compare_var) {
       for (j in compare_val) {
           if (compare_var[i].Contains(RegimConventions.compare_val[j])) {
               return RegimConventions.compare_val[j];
           }
       }
   }

  return null;

}

Mie unul imi place sa mapez conditiile in obiecte / array-uri pentru a le extinde usor mai tarziu, probabil ca si voua :slight_smile:

1 Like

Nu știu cum văd alții codul tău, dar mie mi se pare mai greu de citit un nested loop cu o condiție in interior comparativ cu cinci sau chiar zece condiții distincte…

Loop-ul ala il scrii o singura data si poti sa-i adaugi si un comentariu unde explici ce se intampla, singurul lucru pe care il schimbi in viitor daca e nevoie sa mai adaugi un termen de comparatie este obiectul compare_val … de obicei folosesc coparatie pe cheie si nu pe valoare cum am facut mai sus, dar pentru ca se foloseste o functie “Contains” nu am avut de ales. Pe cheie ar fi fost mult mai usor pentru ca atunci puteai verifica doar daca exista compare_val[compare_var] … asta e o idee, poate ca altcineva are una mai buna?

Poti explica pe scurt ce face metoda si care sunt inputurile?

MergeRegims presupune merge-ul a 2 valori, daca valorile nu exista sau sunt egale nu ai de ce sa rulezi functia.

Primele 4 conditii le-as scoate intr-o alta functie CheckInput, si daca CheckInput e valid rulez MergeRegims. Validarea inputului nu este intuitiv intr-o functie MergeSomething.

merge("", "") => null         <- what?!
merge(null, "b") => "b"
merge("a", null) => "a"

// intuitiv ar fi
merge(null, "b") => throw error!

@tachyean: în locul unui loop de genul sugerat de tine aș alege fără să stau prea mult pe gânduri un switch/case. Treaba cu comentariile este foarte înșelătoare. Va trebui să ai grijă de: teste, cod ȘI comentarii. De fiecare dată când am avut comentarii în cod au rămas în urmă după câteva iterații peste acel cod…


@navaru: nu este vorba de această metodă în special, ci de cum s-ar proceda în general. Dacă vrei, iată alt exemplu:

public function foo() {
  if( a == 1 )
    return a
  if( a == 2 )
    return 5
  if( a == 3 )
    return null
  if( a == 4 )
    return 'bar'
  if( a == 5 )
    return new Date()
}

Nu prea ai ce să mai faci aici.
După cum ai zis mai sus, poate un switch e mai readable, dar nu va avea performațe mai bune.

Ce ai putea să faci, pe cazuri concrete, e să vezi statistic care sunt cele mai des întâlnite cazuri, astfel încât să le returnezi pe acelea primele.

Dacă ai asemenea metode cu multe cazuri, problema e posibil să fie mai sus la nivel de model. De ce ai avea atâtea cazuri specifice? Nu s-ar putea elimina anumite valori în alt loc?

1 Like

cu un map intre a si un functor, poate? si corpul functiei cu multe conditii cauta in map si returneaza functor()?

2 Likes

Nu pot sa raspund la intrebarea originala. Pare un factory method, e si static si nu pare genul de candidat la refactor decat daca vreau sa fac codul testabil. E doar parerea mea de moment.

In schimb ca sa reduci matematic complexitatea in cod ai putea sa folosesti approach-ul din acest video de 15 min: http://adamwathan.me/2016/05/19/refactoring-with-collections-and-null-objects/
Zic doar matematic pentru ca abstractizarea are costurile ei. E posibil ca nu toti sa inteleaga codul dupa aia.

1 Like

Da. Clean Code explica mai detaliat procedeul de numire a metodelor extrase din cea mare. Ideea de baza pe caream retinut-o in acest caz este ca lectura metodei originale sa fie facila si sa indice (inclusiv prin numele metodelor extrase) logica pe care o executa.

1 Like

Din cate am inteles eu e vorba doar de structura, ca matematic n-ai ce optimiza… (cel putin nu fara sa pierzi din precizie sau o conditie care nu e asa importanta)
Depinde daca este necesara abstractizare pentru adaugarea cat mai simpla a conditiilor sau nu. Eu ma multumesc si cu if-uri daca nu imi trebuie functia pentru inca alte 50 de functii care posibil trebuie extinse in viitor.
Acum programatorul trebuie sa stie daca ce se intampla recursiv e ok sau nu daca foloseste alte functii in conditii care au propriile conditii.

Solutia cea mai simpla de abstractizare e crearea de map-uri si parcurgerea totala a acestora. (sa nu uitam ca JS ne mai da posibilitatea sa rulam functii unde avem noi chef, respectiv pentru optimizare parcurgem un map doar pana intr-un anumit punct daca stim ca verificarile s-au efectuat deja altundeva sau se vor efectua altundeva)

Altceva interesant : https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set
Putem crea niste multimi frumusele in JS si deja am cateva moduri de abstractizare imaginate. (dar nu se renteaza decat daca sunt cateva sute de conditii)