javageci

pure Java, what else ?

Codereview sample

Sok évvel ezelőtt került elém egy Java forráskód, amiben valahogy benne maradtak a code review comment-ek:

private final String TETS_USER_NAME = "Jakab Galiban";
//REV: Miért nem static a változó? JD
//REV: Miért kellene annak lennie? GIJ
//REV: Azért, mert így minden osztályhoz létrejön egy
//REV: külön példány, és így lassabb, több memóriát foglal.
//REV: Főleg ha hosszúak a string-ek.

Note:

ez sok évvel ezelőtti emlék. Akkor olvastam ezt egy forráskódban. Nem tudom kik írták bele, a név kezdőbetűkre sem emlékszem, ezért szerepel is G.I. Joe és John Doe. És a pontos megfogalmazásra sem emlékszem, az igazat megvallva arra sem, hogy magyarul volt-e a komment, vagy angolul.

Szóval a saját kommentjeim:

Az ilyen változót valóban static kulcsszóval szoktuk jelölni. Egyébként nem mindig, mert van, hogy olyan helyre írjuk, ahol magától is static lesz, anélkül, hogy ezt kiírnánk. Tehát a komment jó.

Utána jött egy kérdés az ismeretlen junior fejlesztőtől, hogy miért is kellene ennek static-nak lennie. Jogos a kérdés. Miért jogos? Mert nem tudja! Meg kell kérdezni. Teljesen korrekt.

Innen kezdve viszont elkezdődik az a játék, amelyet oly sokszor, és oly sokan játszunk, mert nem látjuk, hogy a másik kezében sem fénykard van, hanem ott is csak egy fehér bot fityeg: vak vezet világtalant. Mert valami persze van. Tényleg több memóriát fog foglalni a nem statikus verzió, és tényleg kicsinyét lassabb lesz (kivéve, ha az osztály szingleton, ugye?). De ez kit érdekel. Amikor arról van szó, hogy a pulton kitöltött féldeci pálinka csak 4 cent, akkor sem állunk neki pampogni, mert sokkal fontosabb, hogy remegő kézzel hamarabb lehörpintsük, mintsem, hogy azt a maradék centet is megkapjuk. Mennyivel lesz gyorsabb a program futása? Fél óra helyett már 29 perc, 59 másodperc és 500 milliszekundum alatt is lefut? Még a perui lámákat sem érdekli! Nekem elhihetitek.

Hogy több memóriát foglal? Hahhhh! Mennyibe kerül a memória? (Híjjnye, minnyárt kapok egy mémes pofont.) Pedig a bentlakásos iskolát nem is említettem, úgyis, mint internátus. (Gy.k: INTERN-átus)

De akkor miért? Azért, mert sok időt elvesz. De nem az olcsó processzor időből, hanem a drága fejlesztő időből, amikor valaki olvassa a kódot, és megakad rajta: Miért nem statik? Hm… Lehet, hogy a menyét elhozta a menyét? És csak gondolkodik, és gondolkodik, mert nem akarja elhamarkodottan azt a következtetést levonni, hogy azért statik, mert aki írta junior volt, mint a fű az első tavaszi vágás után.

Pedig kár gondolkodni. Át kell írni statik-ra, és lefuttatni az összes unit tesztet. Azoknak a lefedettsége 100%, így ha valahol, valami gondot okoz, hogy statik lett a változó, akkor az kiderül igen hamar. Ez csak természetes.

22 responses to “Codereview sample

  1. rlegendi február 28, 2013 1:10 du.

    Good point. Amúgy a `TETS_USER_NAME` nem `TEST_…` akart lenni? 🙂

  2. tvik február 28, 2013 1:16 du.

    Meg azt a magas labdát is lecsapnám hogy ez nem változó hanem field. 🙂

    • v február 28, 2013 3:54 du.

      Idéznék a Java nyelvi definícióból: “The variables of a class type are introduced by field declarations.” A szabvány felváltva használja a “field” valamint a “class variable” és “instance variable” terminológiákat.

  3. Gábor Lipták február 28, 2013 1:17 du.

    Ha tudom, hogy tudja mi a különbség a static és a nem static között, és valószínűleg csak elírás, akkor javítom, és kész. Ha nem, akkor a review teljesen jogos, és a junior fejlesztő tanul belőle. A review szerintem fele arányban a jobb szoftver miatt van, fele arányban pedig tudásmegosztás. Ha ilyen esetben vakon javítunk, csak hogy időt spóroljunk, akkor nincs lehetőség egymástól tanulni.

  4. Ketler Iván február 28, 2013 1:33 du.

    Nekem az a megdöbbentő, ha emberek forráskódon keresztül leveleznek egymással, ahelyett hogy személyesen (legrosszabb esetben telefonon) beszélgetnének.

    • Gábor Lipták február 28, 2013 1:39 du.

      Igen. Kell egy kis mozgás. Én inkább felállok, odamegyek, és megbeszélem 🙂

      • v február 28, 2013 3:56 du.

        Ha több csapat van, mondjuk New York-ban, Zurich-ben, Budapesten és Dnepropetrovszk-ban akkor ez mind távolságban, mind időben nehezen kivitelezhető. Már a Debrecen – Budapest is necces séta, de az még elképzelhető.

        • Ketler Iván február 28, 2013 4:43 du.

          Bocs de ez nem lehet probléma. Distributed project esetén is szokott olyan lenni, hogy “találkozási idősáv”, amikor elméletileg minden érintett elérhető (és gépközelben is van). Ha telefonon, akkor azzal, Ha Office Communicator-ral, akkor azzal. Ha skype-pal, akkor azzal. Lényeg az, hogy “találkoznak”. Itt a Földön bármely ország legfeljebb 12 órával lehet odébb (előre vagy hátra), nem túl nagy erőfeszítés találni egy közös órát, amikor hetente egyszer beszélhetnek.

          • Molnár Miklós február 28, 2013 5:31 du.

            Ezzel csak az lehet a probléma, hogy nagyon kevés időre sok topik szokott jutni és elsikkad esetleg a nagy igyekezetben (az ilyen heti egy megbeszélésnél, nagy hajtás közepette).
            Szerintem az aszinkron kommunikáció jobb és hatékonyabb nagyon sok esetben, míg a szinkron kommunikációt érdemesebb komplexebb tényleg végigtárgyalandó topikokra tartogatni.
            Más kérdés, hogy aszinkron kommunikációt forráskódban kell-e megejteni. Szerintem nem. Most poéntól függetlenül is.

        • S.E. március 10, 2013 10:43 de.

          Hallottam egy érdekes, új találmányról: emailnek hívják. Ha nem extrém sürgős a dolog, akkor ennek a problémának a megoldására is használható.
          😀

    • Tamás Barabás március 1, 2013 10:59 de.

      Nalunk nem forrasba kerul, hanem celszoftverrel keszul, es nem az idozonak miatt, hanem mert a szo elszall, de a hip-hop örök

  5. tamasrev február 28, 2013 2:17 du.

    Jó cikk, én is így gondolom: elsősorban a fejelsztő idejére kell optimalizálni (lásd még itt: http://www.dilbert.com/strips/comic/2013-02-24/) . Sebességre/memóriára meg csak akkor, amikor számít.

    Viszont a záró gondolatok elég idealisták. A 100% valószínűleg lefedettség azt jelenti, hogy profik írták a kódot, hiszen nagyon jól tesztelhető. Valószínűleg TDD-vel írták. A projektek maradék 98%-ában ez sajnos nem így megy. Megesik az is, hogy magas a lefedettség, de az assert-ek száma a 0-hoz konvergál.

    Persze egy ilyen pimf dolgot a rossz teszetk is megfognak. A komolyabb dolgokhoz meg jó unit teszteket, jó funkcionális teszteket és jó kódot kell írni.

  6. Mark március 1, 2013 1:16 de.

    Egy sor kód, 5 sor komment és órákat lehetne beszélgetni a dologról. Best post ever.

    1) Igen, staticnak illik lennie, így szokás
    2) “TEST_…” – tökmindegy, hogy static-e vagy sem, hisz úgyis csak valami teszt adat lesz, tán egy unit tesztben van mindez, stb.
    3) Kommentben “levelezni”? Igen, személytelennek tűnik, de ami fontosabb ennél, naplózva, dokumentálva van. Ha szociális skilleket akarunk fejleszteni, simán a reviewfolyamat lezárta után igyunk meg vele egy kávét. Ne a review késztessen bennünket gyaloglásra 😉
    3.a) (volt már olyan munkahelyenm, ahol a policy szerint két reviewra összerendelt fejlesztő a review lezártáig nem beszélhette meg egymással a dolgot – csak a kód alapján volt szabad ítélni)

    De ha már a performancia is szóba jött… Ha “final”, akkor az osztályban ezen a referencián már csak ez a memóriaterület lesz elérhető. Ha “String”, akkor a string intern pool egy immutable objektuma. De ha “final String”, akkor akár static is lehetne*. Adódik a kérdés: kioptimalizálja a fordító? – Megtehetné.
    *: ha nem final, vagy nem String az objetum, akkor ez nem teljesül**
    **: ezzel is lehetne vitatkozni; ha nem String, de immutable/singleton/poolban van, akkor mi a helyzet.*** Bár azt nem nézem ki egyik fordítóból sem, hogy egy immutable osztályt felismerjen (a primitívekig végigjárja és finalként verifikálja azokat a composition-chainben)
    ***: nos, ezért mondtam, hogy órákat lehetne beszélgetni róla. 🙂

Hozzászólás