search

Imdapro / commons-rdf

Mirror of Apache CommonsRDF
Other
0 stars 1 forks source link

Manuális kód átvizsgálás #3

Closed Imdapro closed 7 years ago

Imdapro commented 7 years ago

A kód egy részét ellenőrizzük manuálisan.

TitanillaGats commented 7 years ago

A megbeszéltek alapján létrehoztam a gitlab repot, amit itt találtok: https://TitanillaGats@gitlab.com/TitanillaGats/commons-rdf.git

TitanillaGats commented 7 years ago

Végeztem az AbstractRDFParser átnézévsével. Nem értem a tagváltozó miért lett függvényhívás, nem találtam hozzá értelmes commit üzenetet sem. Nálam egyébként nem is találja se függvényként, se tagváltozóként, pedig ami nálam van RDFSyntax fájl, abban létezik és tagváltozó.

TitanillaGats commented 7 years ago

Átnéztem a LiteralImpl osztályt is, én a sok replace-t lecserélném egy replaceAll-ra, ahogy a commentben ki is fejtettem. Amit még hiányolok, az a comment a logikát is tartalmazó függvénynél... Néhol egy tagváltozó létét is 5 sorban taglalják, ahol pedig lenne is mit magyarázni, ott semmi.

TitanillaGats commented 7 years ago

Általános észrevétel: kommentek hiánya/túlhasználata, final és Impl használata mindenhol, semmi értelmes commit üzenet vagy issue hivatkozás Felesleges helyeken is final jelző használata tagváltozók, paraméterek és függvények esetében. Nem látom a mögöttes koncepciót, azt viszont igen, hogy 5 hónapja történt és szisztematikusan eléjük írták, számomra "mert miért ne" indokkal. A felesleges kommentek mellett a legrosszabb az értelmes kommentek hiánya. Az előző megszólalásomnál már kifejetettem, hogy mi is ezzel a problémám. A másik általános észrevételem CleanCode elvekkel ütközik. Egyszerűen nem illik kódolni az osztályok nevét, ha semmi plusz információt nem hordoz. Ebben az esetben még megtévesztő is. Hiszen majdnem minden osztály nevében szerepel az Impl postfix, de ez nem jelenti azt, hogy amelyiknél nincs ott, az nem implementál valamit.

TitanillaGats commented 7 years ago

Types osztállyal is végeztem. Nekem annyira nem tetszett hogy görgetni kellett a lényegi tartalomért, a statikus típus-létrehozásokat lehet hogy külön osztályba tenném vagy valahogy máshogy csinálnám. Sokat kellett görgetni a tartalomért, de legalább volt komment.

TitanillaGats commented 7 years ago

Végignéztem az IRIImpl osztályt, egyedül a konstruktora nem tetszett. Tudják, hogy érvénytelen stringnél hiba dobódik, de el sem kapják és tovább sem dobják.

Imdapro commented 7 years ago

Az issue-hoz tartozó wiki oldal

Imdapro commented 7 years ago

Átnéztem, és helyesnek találtam TitanillaGats meglátásait.

micskeiz commented 7 years ago

Rendben, alapos volt a kód átnézése. Külön plusz, hogy még a GitLab-ot is kipróbáltátok közben.

Imdapro commented 7 years ago

Az átnézés sikeresen megtörtént.