ikpa / ot-harjoitustyo

0 stars 0 forks source link

Koodikatselmointi #1

Open gitjms opened 4 years ago

gitjms commented 4 years ago

Ladattu to 23.4.2020 klo 3.27

Yleistä

Ikpan pelikehitelmä vaikuttaa olevan hyvin aluillaan. Vaatimusmäärittelyn luettelemia toimintoja löytyy, eli peli on jo kivasti pelattavissa.

Sovelluksessa on yhdeksän luokkaa plus käyttöjärjestelmän rakentava luokka.

Kolmelle luokalle on olemassa testit, joita on yhteensä 38. Testit ovat hyviä ja ne menevät läpi. Testikattavuus on tosin vasta 41% ja haarautumakattavuus 46%, joten testejä on varmaasti tulossa vielä lisää.

Checkstyle-raportin mukaan tyylivirheitä ei ole, lukuunottamatta kolmea ylipitkää metodia.

README:n ohjeessä ohjelman ajamiseksi on virhe. Siinä opastetaan käyttämään komentoa

mvn compile exec:java -Dexec.mainClass=labyrintti_harjoitustyo.RealMain

mikä ei toimi, sillä pom:ssa on kirjattu mainClass:iksi labyrintti.ui.RealMain. Näin ollen komennon kuuluu olla

mvn compile exec:java -Dexec.mainClass=labyrintti.ui.RealMain

Sovelluksessa ei ole käytetty minkäänlaista DAO-mallia, joka olisi ollut kurssin tarkoitusperien mukaista.

Luokat

Luokat on jaettu pakkauksiin, joita on käyttöjärjestelmäpakkauksen ui lisäksi chars, object, levels ja highscore.

Paketti chars rakentaa pelaajan pelattavan hahmon, object luo muut oliot, kuten kerättävät itemit, vihollisina toimivat piikkipallot, seinät sekä tasojen maalit. Paketti levels huolehtii pelitasoista (joita on kaksi), ja highscore huolehtii pisteiden hallinnasta.

Luokkakaavio ei ole ihan kunnossa, sillä siinä on luokkia, joita ei ole olemassa, kuten Door, ButtonDoor, Hostile, Button, EnemyDoor ja Enemy. Otaksun, että nämä ovat suunnitteilla olevia luokkia. Kuitenkin luokkakaavion tulisi mielestäni esittää nykyistä versiota.

Tästä syystä luokkakaaviosta on hyötyä vain osaksi, luokkien Main, Level, Goal, Item ja MainChara osalta. Luokkakaavion mukaan luokka Level pääsee käsiksi eri pakkauksessa oleviin luokkiin Goal, Item ja MainChara, mutta siitä ei käy selville luokan MainChara pääsy paketin object luokkiin. Muutenkaan en oikein pysty lukemaan tätä kaaviota.

Sekvenssikaaviona esitetty peliruudun päivittäminen sen sijaan vaikuttaa toimivalta.

Esimerkkejä tyylivirheistä

Tyyliohjeiden mukaan "Käytä mahdollisimman kuvaavia nimiä kaikkialla" ja "Muuttujat, joilla on iso käyttöalue, tulee olla erittäin selkeästi nimettyjä".

Luokassa Level esimerkiksi käytetään maalin (Goal) luokkamuuttujana termiä "g", ja metodeissa "initialise" ja "update" hahmon sisäisenä muuttujana termiä "c" (kuten myös luokan Main luokkamuuttujana). Mielestäni tämä ei ole ohjeiden mukaista ja myös oikeasti hankalaa luettavaa.

Vaikka ohjeissa sanotaankin, että "Lyhyen metodin sisäisille muuttujille riittää yleensä lyhyempi nimi", ei mielestäni yksi kirjain ole riittävä nimeksi. Näissä voisi hyvin käyttää maalin nimenä "goal" ja hahmon nimenä "chara", kuten jälkimmäistä on muuallakin käytetty.

Tosin hyväksyisin kyllä esimerkiksi luokassa ScoreReader olevat lyhenteet "f" (File), "fw" (FileWriter), "s" (Scanner) yms., koska nämä eivät liity sovelluslogiikkaan. Sovelluslogiikassa on tärkeämpää olla hyvin kuvaavat termit, jotta kooditarinaa pystyy seuraamaan sujuvasti.

Myös luokan LvlConstructor luokkamuuttuja "wc" (WallConstructor) ei ole hyvä nimi. Luokan Goal metodin "inGoal" termi "sec" (Shape) ei myöskään avaudu. Mikä tämä "sec" on? Section?

Luokkien Item muuttuja "s" (Circle) ja Spike muuttuja "p" (polygon) ovat epämääräisiä, mutta luokkien lyhyyden ja muun selkeyden vuoksi nämä eivät ole isoja ongelmia.

Koodin on muuten ihan luettavaa. Tämä on kuitenkin aivan aluillaan oleva peli, joten on vaikea etsiä parannettavaa vielä. Koodi siis toimii tällaisenaan.

Käytänteet ovat kunnossa lukuunottamatta DAO-puutetta, ja laatu on kohdillaan paitsi aiemmin mainitsemaani kolmea ylipitkää metodia.

Muita ongelmia

Kun toisessa tasossa liikuttaa pelaajan hahmon aivan alas ja jatkaa yhä eteenpäin vaikka hahmoa ei enää näy, tulee hetken päästä virheilmoitus:

Exception in thread "JavaFX Application Thread" java.lang.IndexOutOfBoundsException: Index 4 out of bounds for length 4

Sovellus ei kuitenkaan kaadu: peli-ikkuna katoaa ja tilalle tulee lopetusikkuna, jossa kysytään pelaajan nimeä pistetilastoa varten.

Laatuvaatimusten mukaan "Ohjelma ei saa printata Exceptioneita (Stack tracea) komentoriville, vaikka virhe ei kaataisi ohjelmaa" ja "Tarkista, ettei ohjelmasi käsittele taulukon ulkopuolelle meneviä arvoja, tai jo poistettuja arvoja", joten tämä pitäisi korjata.

ikpa commented 4 years ago

Kiitoksia palautteesta. Korjauksia on tehty ja osa on suunnitteilla.

gitjms commented 4 years ago

Itse asiassa DAO:n puutteen mainitseminen ei ollutkaan relevantti, sillä DAO:a sovelletaan käsittääkseni lähinnä tietokantasovelluksissa eikä Ikpalla ole tietokantaa käytössä.

Muuten luulisin, ettei nimenanto (HighScoreDao) tee tästä työstä DAO-mallin mukaista, sillä käsittääkseni DAO-malliin liittyy interface-osio, jonka taakse datan käsittely piilotetaan, eikä tässä työssä ole interfacea. Mutta en ole tästä ihan varma, DAO:t ovat itsellekin vielä hämäriä.

Latasin nyt projektin uudestaan, eikä Exceptionia enää tullut. Sen sijaan pääsinkin kolmannelle tasolle... :+1:

Pelikentän kokoa ja peliobjektien sijaintia pitäisi kyllä myös parantaa, sillä nyt monet komponentit jäävät ikkunan ulkopuolelle eikä niihin pääse käsiksi.

ikpa commented 4 years ago

Olen pyrkinyt suunnittelemaan kentät siten, ettei peliruudun ulkopuolella olisi mitään objekteja olettaen, että pelaaja ei itse muuta ikkunan kokoa. Varsinaiset kentät on lisäksi suunniteltu siten, että pelihahmo ei pääse seinien ulkopuolelle; kolmas taso onkin vielä keskeneräinen. Missään tämänhetkisessä kentässä ei kuitenkaan näy minun kuvassani komponentteja, jotka ovat ruudun ulkopuolella. Mitkä komponentit jäävät sinulla ruudun ulkopuolelle?

gitjms commented 4 years ago

Kentän asemoinnissa on jotain ongelmaa. Voi olla kyse on jostain alustariippuvaisesta jutusta (käytän Windowsia).

Kuvia: kuv1

Tältä näyttää taso 2. Peli-ikkuna on ylhäällä näytön yläreunassa kiinni ja alhaalla alapalkissa kiinni. Pelaaja ei siis voi nähdä mihin hahmopallo kulkee alhaalla, kuten alla olevassa kuvassa näkyy:

kuv2

Tässä vielä peli-ikkuna maksimikoossaan täyttäen tietokonenäytön:

kuv3

Eli peliruudun alaosaa ei näy.

ikpa commented 4 years ago

Taitaa johtua alustasta. Omalla Linux-koneellani peli-ikkuna muotoutuu automaattisesti kentän pane-olion kokoiseksi. Assistenttikaan ei ole maininnut tällaisesta ongelmasta minulle arvioinnissa.