Vertaisarviointi 2

[Halmela]

I wrote this in Finnish and only after looking at your documentation I noticed that everything is in English. Please notify, if you want this in English

Projekti kloonattu 18.6. klo 17.56

---

Asennus

Poetry valitteli, ettei löydä Tkinter-riippuvuutta, joka on pyproject.tomlissa, muttei poetry.lockissa. Koska kyseinen moduuli tulee Pythonin mukana, kommentoin riippuvuusrivin .tomlista pois ja sain toimivan asennuksen.

Käyttö

Nappulat kuvaavat hyvin, mitä ne tekevät, joskin startin ja lopun painaminen pienelle kartalle on läppärillä hieman vaikeaa. Jos loppupisteen pistää hieman väärään kohtaan (toki tässä vaiheessa ei tarvitse miettiä hirveästi onko naapurisolu oikea vai väärä), niin sen sijainnin saa korjattua vasta kun on vaihtanut aloituspisteen.

Kartta ja sille piirtyvät reitit ovat kivan näköisiä ja inforuutu kertoo vertailuun tarvittavat tiedot. Toivoisin, että reittejä pääsisi kokeilemaan isommillakin kartoilla, jotta algoritmien erot pääsisivät paremmin esille. Mikäli olen ymmärtänyt oikein, JPS on Dijkstraa nopeampi ja vaatii vähemmän solmujen avaamisia, mutta tämä ero tulee paremmin selville varmaankin vasta kartan kasvaessa.

Koodi

Projektia on jaettu hyvin eri moduuleihin käyttötarkoituksen mukaan. Sovelluksen toiminnalle tarpeelliset tiedostot ovat omassa paikassa ja tukiskriptit toisessa. Hieman voisi palastella lisää, esimerkiksi nyt src/algorithms/graph.py sisältää verkon, solmun ja kaaren määrittelyt. Nämä ovat toisiinsa nivoituneita luokkia, mutta itse tiedosto on hieman raskas lukea nyt (tosi pieni murhe, ei tarvitse huolestua).

Olet kommentoinut koodia siellä, missä selkeyttä tarvitaan eniten. JPS:n läpilukeminen oli helpompaa, koska pysyin koko ajan suunnilleen kärryillä. Koodin luettavutta voisi hieman parantaa pitkien rivien pilkkominen^1 ja pitkien funktioiden palastelu useammaksi pieneksi funktioksi.

Esimerkki[^2] : https://github.com/gitjuli94/pathfinding/blob/43a3e3ac7ccc3a5e70fa8bf8a90c74060cc92879/src/algorithms/JPS.py#L149-L257 Rivit 150-187 hakua alustetaan, Rivit 190-256 on päälooppi, Rivit 199-220 käsitellään maalisolmun löytäminen, Rivit 223-250 käsitellään solmun naapurit.

Tämän yli 100 riviä (ml. kommentit) pitkän funktion palastelu vaikkapa neljään yllänimettyyn (tai jollain muulla tavalla) funktioon selkeyttäisi koodia ja siten helpottaisi lukemista ja lopulta sen muokkaamista.

Ylemmästä esimerkistä löytyy pari hieman ei-pythonista tyylirikettä (eivät vaikuta koodin oikeuteen, ainoastaan luettavuuteen). if jpoint is None on sama kuin if not jpoint ja if path_vertex is not None on sama kuin if path_vertex.

---

Olet tehnyt suunnittelupäätöksen, että kartat luetaan ja luodaan etukäteen maptolist.py-funktiolla ja tallennetaan omiksi .py-tiedostoiksi. En usko, että tämä on kauhean raskas operaatio, joten sen voisi tehdä myös ajon aikana Load map -nappulan painamisen yhteydessä. Tällöin saisit hieman isompia karttoja kätevämmin käyttöösi, jotta algoritmit pääsisivät kunnolla loistamaan.

Viimeinen silmääni pistänyt asia oli, että muutat karttojen tiilet nolliksi ja ykkösiksi. Koska kyseessä on vain kaksi arvoa, se voisi aivan hyvin olla myös tosi/epätosi. Tämän seurauksena varmaankin kulkeutuvuuden tarkistus ja verkkojen luominen olisi ainakin siistimpää luettavaa, ja ehkä jopa jotkin osat olisivat nopeampia.

Kaiken kaikkiaan projektisi on oikein hyvässä jamassa. Reitti löytyy ja piirtyy nopeasti, eli ydintoiminnallisuus on hyvässä jamassa. Kokeile hieman palastella isoja funktioita ja moduuleita pienemmiksi, niin koodia on hieman kivempi lukea myös.

[^2]: HUOM! En ole kirjoittanut koodiasi, joten en tiedä miksi olet tehnyt ne päätökset, jotka olet tehnyt, enkä välttämättä tiedä täysin miten asiat toimivat, joten luota omaan arvostelukykyysi viime kädessä

[gitjuli94]

Moi, kiitos kattavasta ja rakentavasta palautteesta! Hyviä huomioita. :)

Voisitko vielä tarkentaa mitä tarkoitit tällä:

Viimeinen silmääni pistänyt asia oli, että muutat karttojen tiilet nolliksi ja ykkösiksi. Koska kyseessä on vain kaksi arvoa, se voisi aivan hyvin olla myös tosi/epätosi. Tämän seurauksena varmaankin kulkeutuvuuden tarkistus ja verkkojen luominen olisi ainakin siistimpää luettavaa, ja ehkä jopa jotkin osat olisivat nopeampia.

[Halmela]

Tällä hetkellä maptolist.py muuttaa karttatiedostosta löytyvät merkit 0:ksi tai 1:ksi riippuen ovatko ne kuljettavia vai eivät. Koska kyseessä on kaksi arvoa, niitä voisi olla luonteva käsitellä bool-tyyppisinä (True tai False). Tällöin ainakin https://github.com/gitjuli94/pathfinding/blob/cf2544799b241663bbe24d74d5bb4c2c59649156/src/algorithms/graph.py#L53 tarkistuksen voisi poistaa. Pythonissa 0 == False ja 1==True, mutta niiden käyttö voi olla joskus hieman epäselvää luettavaa, koska periaatteessa numerolla voisi olla muitakin arvoja niiden lisäksi, vaikka kirjoittaja tismalleen tietäisi ettei voisi olla.

Tuo kommentti koon vaikutuksesta perustui omaan väärinymmärrykseen Pythonin toiminnasta. Python tallentaa sekä kokonaisluvut että totuusarvot vaihtelevan kokoisina muistiin:

from sys import getsizeof
print(getsizeof(0))
# 24 tavua = 24 * 8 bittiä = 192 bittiä
print(getsizeof(1))
# 28 tavua = 28 * 8 bittiä = 224 bittiä
print(getsizeof(False))
# 24
print(getsizeof(True))
# 28

Etsin tästä vähän tietoa, ja standardidokumentaatiossakin sanottiin boolin olevan intin erikoistapaus.

---

Noiden tyyppien muuttamien on kuitenkin aika pieni muutos, jolla ei ole melkeinpä mitään roolia koko ohjelman toiminnan kannalta. Ehdotan kuitenkin, että yrität siirtää tuon kartan lukemisen ja muuttamisen ohjelman ajon aikana tehtäväksi toimenpiteeksi, jotta saisit helpommin uusia karttoja testattavaksi.