Code review jonkun toisen koodille

[EficodeRjpalt]

Ohjeet: https://ohjelmistotekniikka-hy.github.io/python/viikko6#koodikatselmointi

[EficodeRjpalt]

Katselmoinnista jaetaan 0-2 pistettä. Vähintään 6 laadukasta ja rakentavaa palautekommenttia riittää 1.5 pisteeseen. Täysiin pisteisiin edellytetään myös vähintään yksi käyttökelpoinen parannusehdotus.

[EficodeRjpalt]

• Koodiin voisin lisätä vinkit tyypityksestä. https://docs.python.org/3/library/typing.html (Olisi kannattanut olla jo ennen docstringejä, muttaah)
• muuttuja board.py:66 -> free: koko muuttujaa ei tarvita. Kaikki if lausekkeet palauttavat sen arvolla False => jokainen palautus voi olla suoraan False ja lopussa sitten True.
• Jään miettimään, että olisiko kuitenkin parempi, jos ruudukon rakenne olisi kaksiulotteinen matriisi. Pelin skaalaaminen isommalle laudalle olisi helpompaa.
• For-loop board:py105 -> ymmärrän ajatuksen, mutta voisi olla kommentti kummankin loopattavan numerosarjan päällä, koska indeksi-looppien lukeminen ja päättely on aika raskasta puuhaa.
• Voisi olla myös testi epäkorrektille siirrolle humanplayer_test.py:ssä.
• Jos testattava asia palauttaa boolean arvon, kannattaa oikeastaan käyttää assertTrue/assertFalse -metodia.
• Invokeen kannattaa iskeä autopep8 mukaan käskyyn, niin linttauksen yhteydessä eliminoituu automaattisesti virheet kuten trailing whitespaces
• open without encoding => lisäät openiin encoding='UTF-8' (esmes._: https://www.learnbyexample.org/python-open-function/)

[EficodeRjpalt]

Projekti ladattu 12.12.2022 klo 16:50

Morjens! Projekti toimii aivan kuten README.md lupailee ja pelaaminen onnistuu. Testit menevät läpi ja lintteri antaa koodille arvion n. 8.50. Sitten palautteeseen.

1. Itse iskin invoken lint käskyyn mukaan autopep8-käskyn, joka varmistaa, että ns. 'itsestään selvät' linttausvirheet korjataan joka kerta juuri ennen linttausta. Itse toteutin näin.
2. Muutaman lintterin herjaaman open-funktion käytön voisi korjata nopeasti lisäämällä open-funktion parametreihin tiedon käytetystä enkoodauksesta (encoding='UTF-8 riittänee). Ks. täältä
3. Jos haluaa tuoda helposti mukaan projektin ulkoisen kirjaston käyttöön, niin autopep8:n voi suorittaa myös formatterilla kuten Black => Samalla tulee plakattua ulkoinen kirjasto. Black ja formatointi vs. linttaus voi toki herättää tunteita tarkastajissa. Olen nähnyt aiheesta n. 2h tiukan dialogin kahden senioridevaajan välillä.
4. Koodissa voisi olla mukana vinkit funktioiden parametrien ja palautusarvojen tyypityksistä. Vinkit tyypeistä auttavat koodia tuntematonta lukijaa paremmin ymmärtämään, että mitä tapahtuu.
5. Muuttuja free (board.py: rivi 66) on oikeastaan tarpeeton. Kaikki funktiossa olevat return-lausekkeet voivat palauttaa suoraan boolean-tyyppisen arvon False tai True. Sama pätee rivillä 104 määriteltyyn muuttujaan win; sen alta voidaan funktiossa aina palauttaa suoraan boolean-arvo ilman, että se laitetaan talteen muuttujaan.
6. humanplayer_test.py'ssä voisi olla testi epäkorrekti siirrolle.
7. Jos jokin testattava metodi palauttaa boolean-tyyppisen arvon kuten esim. täällä, on suoraviivaisempaa käyttää unittestin metodia assertTrue tai assertFalse. Dokumentit täällä.

Luin myös läpi vaatimusmäärittelyn ja jäin miettimään muutamaa listalla olevaa asiaa.

1. Jos toteutat älykkäämmän koneen, joudut varmaan tekemään vain koneen, joka pyrkii yksinkertaisesti blokkaamaan aina ihmispelaajan aikeet, ellei koneelle anneta myös mahdollisuutta olla 'hyökkäävä' osapuoli, eli pelin aloittaja. Silloin logiikka olis aika yksinkertaisesti tarkistaa, että onko ihmisellä johonkin suuntaan rukseja jonossa jne. Jos taas kone saa aloittaa, niin siinä saakin sitten lisätä enemmän tekoälyä mukaan.
2. Jos aiot toteuttaa toiminnallisuuden, jossa pelaaja saa valita pelilaudan koon (varmaankin yhden sivun pituuden), joudut ainakin refaktoroimaan isomman kautta funktion, joka tarkastaa onko kukaan voittanut. Harkitsisin myös tietorakennetta, jolla lautaa mallinnetaan. Tällä hetkellä se on merkkijono, mikä on sinällään ihan legitiimi, mutta kuinka hyvin se skaalautuu, kun lauta on n kokoinen ja skaalautumisessa ajatellaan myös testattavuutta?

Joskus aikanaan tuli itse toteutettua vastaavanlainen sovellus ohjelmoinnin jatkokurssille Javalla, ja silloin ratkaisin ongelman mallintamalla laudan kaksiulotteisena matriisina, jossa jokainen rivi oli oma listansa.Tässä lähestymistavassa on se hyvä puoli, että tarkastamista on helpompi särkeä pienempiin apufunktioihin ja ratkaisu skaalautuu paremmin mihin tahansa lautakokoon.

Tähän liittyen vielä palaute: borad.py:105 alkaa for-silmukka, jossa iteroidaan indeksin avulla. Indeksin/indeksien avulla iteroivat silmukat ovat miltei yhtä vaativia ymmärtää auki jälkeenpäin kuin niitä kirjoittaessa - tai ehkä jopa vaativampia. Niiden kohdalla henk.koht. tekisin poikkeuksen ja kirjottaisin risuaidalla varustettuja kommentteja silmukan sisään, jotta pysyy paremmin perillä mitä tapahtuu ja miksi.