Vertaisarviointi 2 - Githubissues

No niin, mielenkiintoinen hommahan tämä ihan. Perus hommat näyttää olevan paketissa ihan kivasti. Tässä muutamia kommentteja ja muita vanhan parran jorinoita, joita ei välttämättä tarvitse ihan täysin vakvissaan ottaa; monet jutut lähinnä huomautuksia työelämän puolelta ja osa tekstistä oireita siitä minkälaista Pythonilla devaaminen yleisesti on kun on ollut alalla liian pitkään. Eli kannattaa lukasta vähän kieliposkella ja pahoittelut että tuli taas vähän romaanin mittainen teksti :)

1. Yleisiä perus huomioita

Ilmeisesti testi kattavuus on ainoastaan 63% testatunkin koodin osalta? Tähän varmaan jokin hyvä syy?

Toinen mikä tuli vastaan on se, että et ole tainnut ajaa lintteriä, kun kommenteissa on 111 merkkiä pitkiä rivejä, jne.; käsittääkseni Pythonissa pitäisi olla 80 merkkiä rajoitus rivin pituudelle. Lintterissä on aika pitkä lista kaikkea mitä voisi korjata.

Myös mainissa, eli main.py tässä tapauksessa, on oudosti importattu paljon kaikkea mitä kyseinen tiedosto ei käytä; mahtaakohan olla jokin oikea syy miksi näin on tehty, kun ei ole ihan tyypillistä tämä? Toki joillakin frameworkeilla on outoja sivuoireita.

2. Yleisiä huomioita koodista

UI ei toimi

En pysty testaamaan käyttöliittymää, koska saan tämmöisen virheviestin:

Traceback (most recent call last):
  File "src/main.py", line 18, in <module>
    main()
  File "src/main.py", line 10, in main
    window = Tk()
  File "/usr/lib/python3.8/tkinter/__init__.py", line 2270, in __init__
    self.tk = _tkinter.create(screenName, baseName, className, interactive, wantobjects, useTk, sync, use)
_tkinter.TclError: no display name and no $DISPLAY environment variable

Pohdin että johtuu varmaan siitä, että ajan headless Ubuntua WSL:ssä (tarvitsen tätä töissä siihen, että voin tehdä koneoppivia malleja GPU kiihdytettynä).

Yleisiä kommentteja luokista

Osassa luokista on "aneemista domainkieltä", eli gettereitä ja luokkia, jotka eivät tavallaan tee mitään. Esimerkiksi Private Key ja Public Key luokat ovat täysin identtiset ja täten duplikoitua koodia, joka ei edistä koodin helppolukuisuutta. Muutamat metodit ovat yhden rivin metodeja, jolloin olisi ehkä fiksumpaa vain suorittaa kyseinen rivi. Pythonissa on toki omat ongelmansa olio ohjelmoinnin suhteen, kun se ei ole oikea olio-ohjelmointi kieli (tärkeitä rakenteellisia rajoitteita jne. puuttuu) ja joitakin tärkeitä periaatteita ei aina pysty noudattamaan. Toinen asia tietysti on että kouluissa projektit usein ovat vähän kapeita, eikä niissä välttämättä huomaa tämmöisen koodaus tyylin haittoja. Käytännössä olio-ohjelmoinnin tärkein funktio työelämässä on se, että se omaa paremman kyvyn hallita kompleksisuutta koodipohjan kehittyessä, koska oliot voivat noudattaa niin kutsuttua domain kieltä, joka kuvaa sitä mitä ohjelma liiketoiminnallisesti tekee. Eric Ewansin "blue book", eli Domain Driven Design on ihan hyvää luettavaa jos aihe kiinnostaa ennemmänkin.

Abstraktio hidastaa koodin lukemista ja vaikeuttaa ymmärrystä, joten sitä ei sovi tehdä kevyin perustein. Moni toki hylkää OOP paradigman ja pakenee funktionaaliseen koodaukseen, kun näiden perus periaatteiden noudattaminen on usein vaikeaa :) Työelämässä sen ymmärtäminen mitä domain kielellä tarkoitetaan on hyvin tärkeää, mutta liian harva sitä ymmärtää enää nykyään, kun ala kasvaa niin nopeasti.

Toisaalta, liittyvätkö nämä ylimääräiset luokat ja asiat siihen, että tuolla components kansiossa osa moduulin koodista noudattaa jonkinlaista UI frameworkin sanelemaa rakennetta, joka sitten helpottaa formien / muun hydratoidun datarakenteen parsimisessa? Tämmöisiäkin on tullut vastaan ja siis jos tämä on syynä niin ihan ymmärrettävää.

Certificate Module

certificate.py tiedostossa rivi 70 on muuten liian pitkä. Tehdään liian monta asiaa yhdellä rivillä. Koodi dokumentoisi paremmin itse itseään, jos tehtävät asiat olisivat tehty atoomisemmin siten, että jokaisen rivin tulos assignataan kuvaavasti nimettyyn muuttujaan. Paras muuttujan nimi on sellainen, joka kertoo jotain siitä, miksi tämmöinen temppu, joka rivillä tehdään, on tarpeellinen. Rivillä 101 sama homma. Rivit eivät ole monimutkaisia, mutta joskus kun tekee näin ja joku toinen jatkaa niin yhtäkkiä ei olekaan enää loogista kokonaisuutta yhdellä rivillä.

Prime Generator

Sopivan simppeliltä näyttää. Riveillä 60 ja 64 on for looppeja, joissa parametria ei käytetä ja lintteri virheet vaimennetaan. Tyypillisesti Pythonissa ja monissa muissa kielissä käytetään muuttujan nimeä ‘_’, eli koitappa korvata first_round ja second_round muuttujan alaviivalla niin varmaan lintterikin hiljenee.

Rivien 62-69 if-for-else sisennykset ihmetyttää; käsittääkseni pitäisi kyllä tulla erroria, että ei näin voi ajaa, kun eihän for loop voi olla if:n jälkeen, jonka jälkeen samalla tasolla on else, joka kuitenkin ilmeisesti viittaa if:iin? Voi olla että toimii oikein, mutta näissä kannattaa olla tarkkana, koska näihin kohtiin sattuvat bugit voivat olla hankalia jäljitettäviä, koska jotkut Pythonin osat eivät välttämättä ymmärrä tämmöistä rakennetta. Ilmeisesti tuon else:n rivillä 68 pitäisi olla sisennetty?

Ja tosiaan, ymmärrän että tässä projektissa on tarkoitus tutustua algoritmiin ja halusin vain kertoa sellaisen käytännön asian, että Pythonillahan ei kannata tehdä mitään looppaavia juttuja, jos niitä ei voi ulkoistaa c-moduuleille. Pythonin loopit ovat 10-1600 kertaa hitaampia kuin monien muiden kielien vastaavat ja salauksessa performanssi on usein tärkeää; toki tässä ehkä tehdään ilmeisesti omalla koneella ajettavaa utility työkalua, jossa performanssi ei ehkä ole niin justiinsa. Todennäkisesti tämän tiesitkin, mutta halusin varmistaa, kun tämmöistä ei kuitenkaan ollut dokumentaation pohdinnoissa; en siis tiedä miten monta kierrosta noita luuppeja oikein ajetaan käytännössä, mutta jos ajetaan paljon niin sillä allkaa olemaan merkitystä. En missään nimessä ehdota, että kannattaisi tässä projektissa lähteä mitään c-kirjastoa tekemään, eli ihan vaan for your information tyylinen kommentti tämä.

RSA Key Generator

Rivi 50 on taas liian pitkä ja sitä voisi hieman palastella samaan tapaan kuin Certificate tiedostosta kirjoitin.

Testeistä

Testien nimet ovat hyvin kuvaavia ja ainakin prime_generator.py:n testaus näyttää tarkoituksenmukaiselta.

Loppu kommenttia

Ihan ehjän näköinen kokonaisuus, kiva olisi ollut päästä testaamaankin toki, mutta tällä erää ei onnistunut. Pythonille on ihan kivoja tekstipohjaisiakin käyttöliittymä paketteja, jotka eivät tarvitsisi näyttöä; jotkut ajavat linuxia lähinnä docker containerissa ja niissäkään ei välttämättä ole näytölle accessia. Kiva homma, ja hyvää jatkoa!

hzville / Tietorakenteet-ja-algoritmit-harjoitustyo-2022

Vertaisarviointi 2 #2