Code reviews - una din cele mai subestimate activitati din programare

Majoritatea echipelor in care am lucrat pana acum par sa trateze crearea unui code review ca ceva marunt, care de obicei nici nu e mentionat la activitatile pe care le-ai facut. Personal mi se pare una din cele mai subestimate activitati. Un code review bun trebuie sa porneasca de la a intelege ce trebuia facut. Asta poate fi in sine un lucru complicat. De exemplu daca taskul era implementarea unei functii matematice complexe, reviewerul trebuie intai sa inteleaga matematica necesara, inainte sa citeasca o singura linie de cod. Apoi urmeaza partea de analiza de cod, care poate presupune a vedea ce fac sute de linii de cod si cum sa integreaza in contextul programului mai larg.

In experienta proprie deseori cand am facut code review serios a fost foarte demotivant. In primul rand deseori persoana careia i se face code review are reactii de respingere, desi am folosit mereu un limbaj politicos si am argumentat clar motivele. Dar majoritatea programatorilor par sa vada code review ca o critica, in loc de a-l vedea ca o ocazia de a invata. In al doilea rand, la daily meeting cand spui ca n-ai progresat mult pe propriile sarcini pentru ca in ziua dinainte ai folosit cateva ore pentru code review, iar se simte o oarecare dezaprobare. De asta cred ca firmele ar trebui sa aiba o cultura care sa promoveze in mod activ activitatea de code review si sa-i dea importanta cuvenita. Altfel e clar ca  decat sa  argumentezi la greu cu persoana care a scris codul si managementul apoi sa strambe din nas ca de ce nu s-a bifat mai repede ca taskul e gata, e mult mai simplu sa te uiti super rapid pe cod si daca nu vezi nimic evident gresit, dai approve si doamne ajuta...

Pai daca dai approve, iti asumi in caz ca este gresit, taskuri din astea doar sa fie merg la bugetari, nu in firme private. Eu refuz in mod constant sa fac asa ceva, imi aloci suficient timp sau nu fac, noi pontam in PlanView si tinem o evidenta destul de stricta a orelor lucrate.

Si ai dreptate, nu se priveste cu destula seriozitate code review-ul, utilizarea principiilor de programare, modul in care se comenteaza codul...Dar tine si de noi sa impunem niste principii.

Conteaza in ce domeniu este folosit SW-ul. Daca este Safety, sa vezi ce se tine cont de code review. Si sa vezi ce se tine cont de naming convention, de code comments, reguli Misra si asa mai departe.

Edited by qew, 21 March 2022 - 17:10.

Code review-ul a murit de cind s-a inventat agile. Acuma se merge pe development rapid si hopefully testat si daca totusi pusca ceva trintesti un hotfix. Exista chestii care verifica sintaxa daca exista asa ceva impus, nu tre sa-i spui omului sa formateze intr-un anumit fel. Si acolo unde se face code review, nimeni nu are timp sa stea cu orele.  E bine asa? Evident ca nu, dar nimanui nu-i pasa, este mai important sa iasa lucrurile rapid si daca cumva mai exista programul peste ani si ani da-i in mortii ma-sii pe aia de-atunci sa-si bata capul si sa se minuneze de timpeniile din prezent.

true, code review=se ocupa clientul de asta=>zeci de hotfix-uri iar cand se aduna vreo 100 de carpeli, se tranteste o nou versiune de aplicatie si se reia ciclul.

Edited by aeon, 21 March 2022 - 20:47.

confundati code review-ul cu testarea (quality assurance)

code review-ul este prima linie de testare, e drept, dar scopul principal este mai degraba sa asiguri cod bun si mentenabil, nu sa elimini bugurile. clientii au probleme din cauza ca aplicatia n-a fost testata, nu pentru ca nu s-a facut code review. poti testa o aplicatie foarte bine (pana la nivelul la care clientii nu raporteaza nici un bug) fara sa faci code review, si poti avea buguri pe banda rulanta desi code review-ul este obligatoriu (pentru ca uneori e mai greu sa observi efectele problematice in cod decat la testare).

Quote

Pentru majoritatea aplicatiilor, un cod bun si mentenabil e mai important decat existenta unor bug-uri. Bug-urile intr-un cod bun sunt niste accidente. Bug-urile intr-un cod prost scris sunt un mod de viata. Chiar daca pentru moment testarea descopera bug-urile dintr-un cod prost scris, asta e ca bolovanul lui Sisif - crezi ca ai reusit ceva, dar la urmatorul release ai din nou o munca uriasa de a repara o gramada de bug-uri.

Esenta programarii a fost mereu calitatea codului care rezulta din cat de bun si motivat e programatorul. E drept ca lucrurile astea sunt foarte greu de masurat, dar raman baza. Niciun Agile nu va face din rahat bici. Iar code review-ul e un instrument esential ca sa ridice aceasta calitate si in acelasi timp o oportunitate (mai ales pentru cei mai putin experimentati, dar nu numai) sa invete meserie.

Shinji, on 21 martie 2022 - 15:27, said:

Nici incepatorii care se mai abat pe aici nu accepta/ solicita code-review.
Dupa cum spune Mosotti, la case mai mari nici nu incape discutie ... "ship it".

_Smiley_, on 21 martie 2022 - 21:09, said:

in primul caz este code review, in celalalt caz este code overview.

Edited by MarianG, 21 March 2022 - 23:50.

MarianG, on 21 martie 2022 - 22:43, said:

Sunt sigur ca mentalitatea asta ii ajuta mult sa progreseze...

La firma pentru care lucrez (si e o casa "mare") code review-urile sunt obligatorii, nu poti sa dai merge la codul tau pana nu primesti approve de la 1-2 colegi. Astazi ii faceam code review unui coleg, mid-developer C++ caruia a trebuit sa-i explic de ce nu e bine sa dereferentiezi un pointer null. Asta in timp ce reiesea din atitudinea sa ca il agaseaza faptul ca trebuie sa aiba de a face cu un reviewer care nu da approve lejer. Dupa ce i-am explicat in review, mi-a dat si telefon sa facem screen sharing sa lamurim (printre altele) aceasta mare dilema cu pointerii null. Si cand in sfarsit l-am convins, si-a editat propriul comentariu din review in care intreba care e problema cu pointerul acela, lasand raspunsul meu sa pluteasca in gol. Culmea ca omul nu e deloc prost, deci cred ca era mai degraba un efort de a justifica cumva propria sa neglijenta, decat o incapacitate de a intelege.

Dupa ce ai mai multe experiente similare ajungi sa te intrebi daca merita sa iti toci timpul si nervii in felul asta. Si aici ar trebui sa intervina cultura organizationala a firmei. Pentru ca e usor sa zici "code review e obligatoriu", dar apoi sa nu aloci in mod clar timp pentru aceasta activitate. Ca deh, dupa 3-4 ore de review in care intai trebuie sa intelegi ce a facut cineva si apoi te izbesti de lipsa de chef a lui de a isi repara problemele pe care i le-ai semnalat, deseori cuplata cu iritare ca ii critici codul minunat, esti fresh sa tragi tare pe taskul tau.

Sunt de acord cu ce spune initiatorul topicului insa intreb si eu exista firme de genul ca eu nu le cunosc?

In cazul ca exista sa se numeasca si le cunoastem.

Eu am intalnit doar...

Mosotti, on 21 martie 2022 - 18:18, said:

Edited by iulian_1976, 22 March 2022 - 01:27.

Well, argumentele pentru code review par bune, daca traiesti intr-o lume ideala, plina pina la refuz de eroi legendari care nu pot dormi noaptea daca nu scriu codul perfect. In realitate existenta unui tool de code review si obligativitatea de a primi approve de la 5000 de seniori nu inseamna nimic, pentru ca oricine poate da approve la orice. Daca ai o chestie serioasa, nu un rahat de bug fix de 3 linii, este imposibil sa te uiti la cod si sa intelegi ce-i acolo fara sa petreci cel putin la fel de mult timp ca ala care a facut codul

Sigur ca poti de exemplu sa descoperi ca e un cod complet de cacat, scris ca dracu etc si sa spui sa-l rescrie pe tot, pentru ca nu e frumos, nu e mentenabil etc, desi functioneaza perfect si asta e un scenariu perfect posibil, dar este si o imensa pierdere de timp. Pentru ca daca ai in echipa oameni care fac asemenea lucruri este mult mai util sa ii verifici in timp ce lucreaza si sa-i indrumi pas cu pas decit la sfarsit sa vezi ca totul e o ciorba dubioasa. E ca si cum ai lua un mester sa-ti faca o casa si il lasi sa lucreze un an la ea si la sfirsit te uiti si vezi ca desi casa e in picioare, nu ploua in ea, apa curge la robinet etc, e perfect locuibila, materialele sint de cacat, peretii sint strambi si pe linga asta vopsiti in galben cu roz turbat. Sigur ca nu vezi ca de fapt fundatia are crapaturi decit daca te apuci sa sapi, habar n-ai ca la iarna o sa-ti inghete curul in ea pentru ca e prost izolata si nici ca daca bate vintul un pic mai tare incep sa cada tiglele, da ii spui sa revopseasca si eventual sa puna niste rigips ca sa indrepte peretii. Poate ca era mai bine sa procedezi altcumva

Shinji, on 21 martie 2022 - 23:54, said:

eu unul comentez direct in review doar greselile clare, sau sugestii de imbunatatire. orice altceva, discut intai cu respectivul.
scopul unui review e imbunatatirea codului, nu "public shaming"-ul.

_Smiley_, on 22 martie 2022 - 08:53, said:

N-a fost vreun public shaming. Dereferentia un pointer care putea fi null si i-am scris sa verifice intai ca nu e null. La care mi-a raspuns care e problema daca chiar e null pointerul. O intrebare sincer ciudata venind de la un om cu ani de experienta in C/C++. Dar de vreme ce m-a intrebat, i-am explicat, politicos si cu argumente.

Quote

Ei exact aici e problema. Poate ca nu dureaza la fel de mult timp, dar dureaza mult in orice caz daca codul si problema pe care o rezolva nu sunt simple. Si managerii vin cu cerinte gen code review si creat unit teste, care sunt lucruri bune si in multe cazuri (nu totdeauna!) ar trebui facute, dar fara sa realizeze cat de mult dureaza asta. Pentru ei e ceva de genul: sprintul are 10 zile, pai bun, aloci o zi sa faci unit teste, iar review gasesti timp printre alte chestii, nici nu are nevoie de timp alocat. Deh.

Edited by Shinji, 22 March 2022 - 09:41.

eu cred ca depinde de buget și ce vrei sa faci. de exemplu la o banca se cer unit teste, cod de review și tone de documentație au echipa de testare etc pt ca reputația contează mult dar codul de obicei tot rahat iese în schimb e testat și merge si au buget de toate chestiile astea. la un start-up sau daca faci un soft pentru tine e foarte posibil sa implementezi aceasi chestie ca cei de la banca cu o miime din bugetul lor fără cod review și echipa de testare și de vreo 10 ori mai repede. cum e mai bine? depinde de buget

Nu faci Code Review pentru nimicuri gen formatarea codului, asta se automatizeaza foarte usor.

Faci pentru a identifica in primul rand probleme majore. Poate nu e ok sa citeasca datele din sursa X chiar daca pe moment merge, sau sa introduca dependinta la o biblioteca externa de verificare daca un numar e par sau impar.

E in functie de buget si timp si mai ales cum obtii fonduri. Daca bosii de sus vor sa livreze orice la timp doar ca sa-si bifeze lista de bonusuri nici nu se mai tine cont daca e functional softul.
Pai nu-l poate folosi nimeni.
Pai nu-i nimic, se schimba numele proiectului care insa face acelasi lucru, se renuntat la ce s-a dezvoltat, si se reface acelasi soft in alta varianta imbunatatita.
Pierderea o suporta statul sau actionarul care la randul lui o pune in carca  statului.