Koodikatselmointi #1

[maarila]

Latasin tiedostot ti 21.8. klo 12:08.

Tosi mielenkiintoinen projekti ja aihevalinta! Vähän on kimuranttia katselmoida keskeneräistä työtä, ja osa kommenteista onkin varmasti luokkaa "on noin, koska ei vielä valmis", mutta kirjasin nyt kuitenkin ylös kaiken, mitä silmään osui.

• En tiedä, ovatko konventiot vain uponneet kallooni turhankin syvälle, mutta mulle tuotti ohikiitävän hetken verran vaikeuksia lukea for-luuppeja, jotka eivät käyttäneet ensimmäisenä indeksinä i:tä, vaan esimerkiksi k:ta tai j:tä. Esimerkiksi transpositiosalauksessa on sisäkkäiset for-luupit järjestyksessä j-i, minkä seurauksena mulle oli tosi epäluontevaa hahmottaa, mihin kumpikin viittasi, kun indeksejä käytettiin luuppien sisällä. Sorting-kansion metodeissa on indeksinä käytetty myös muuttujaa idx. Miettisin siis ehkä luuppien yhdenmukaistamista kautta linjan, niin koodia on sujuvampi lukea. Ei siis välttämättä tarvitse mennä (lievästi pakkomielteiseen) i-j-k-järjestykseen, mutta ainakin olisi hyvä, että systeemi olisi koko projektin läpi yhdenmukainen.

• Samantyyppinen juttu (ja tämä ehkä todistaa neuroottisuuteni) tuli vastaan helpers-kansion GreatestCommonDivisorissa: metodi ottaa vastaan parametrit a ja b, mutta sitten vertaa niitä if-lauseessa järjestyksessä b ja a. Eihän se toimintaa haittaa saati sitä miksikään muuta, mutta näin lukijana ensimmäisenä tuli mieleen, että onko siinä jokin tietty syy, miksi näin on tehty. Noudattaisin siis itse siinä parametrien annettua järjestystä.

• Tämä nyt ei ole mitenkään akuutti juttu, mutta viimeistään projektin loppusiivousvaiheessa ottaisin pois luokkien aluista "To change this licence header..." -kommentit.

• Ciphers-kansion salauksissa yhdistetään merkkijonoja luupeissa +:lla. Oletko testannut, olisiko StringBuilderista saatavissa tehokkuusetuja, vaikkeivät merkkijonot nyt kovin valtavia olekaan? (Olettaen, että StringBuilderia saa tira-työssä käyttää...)

• Muuttujien nimet ovat tosi hyviä ja kuvaavia valtaosin, mutta joitakin ehkä harkitsisin: CharIndexPairissa ovat muuttujat c ja number (samanmoinen vähän epäkuvaava c on myös IndexOfCoincidencessa). Olisivatko character ja index kuvaavampia? Miettisin myös tapauskohtaisesti, kuinka paljon nimeämisessä lyhentäminen auttaa. TranspositionAttackissa ovat mm. muuttujat combi, ciph ja quad. Keskimmäisestä käyttäisin varmaan muotoa cipher, niin se tekisi koodista luettavampaa, mutta nuo kaksi muuta ovat hankalampia, kun niiden kantasanat ovat sen verran pitkiä. Quad voisi tosin ehkä olla ihan perustellusti vaikka quads, kun kantasana on monikossa. (Kyllähän tuo kolmikko siis noinkin toimii, mutta jäin vain miettimään.)

• Toinen nimeämisjuttu löytyy testien puolelta: JUnitissa (ja laajemmaltikin yksikkötesteissä) käytäntönä lienee, että testimetodin nimi kertoo, mitä testi testaa, siis tyyliin "public void negatiivisenLuvunAntaminenPalauttaaMiinusYksi()".

• Ciphers-kansion CharIndexPairia kuvataan apuluokaksi. Kannattaisikohan se siirtää helpers-kansioon? Tai ehkä cryptanalysis-kansioon? Nyt se on vähän yksikseen salausten joukossa. Muilta osin projektin rakenne on erinomaisen hyvä ja selkeä!

• Lopuksi vielä yksi juttu vähän ehkä katselmoinnin ulkolaitamilta, mutta kun itse sain vastaavaa oppia vähän aikaa sitten, ja pidin sitä tosi hyvänä, niin ajattelin panna kiertoon... Gitin commit-viesteistä, nimittäin. "Best practicena" pidetään viestien kirjoittamista imperatiivissa. Ei siis "Lisäsin testit" tai "Lisätään testit" vaan "Lisää testit" (suositus löytyy, kuten sittemmin opin, mm. täältä commit guidelineista: https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project). Kun tätä minulle ystävällisesti "ehdotettiin" ja siihen mukauduin, niin olen omissakin projekteissani pitänyt sitä hyvinkin selvänä ja toimivana tapana (plus että se vähentää arpomista commit-viestien kirjoitusvaiheessa). Ja yhdenmukaisuutta sekin tuo.

Tsemppiä loppuprojektiin!

Mika