Koodikatselmointi 1

[cobrelli]

paketti ladattu 4.1 klo 14:23

Aihemäärittely antoi hyvän yleiskuvan aiheesta, myös projekti vastaa aihemäärittelyä. Projekti ja aihe vaikuttaa mielestäni todella mielenkiintoselta ja olikin hauskaa lukea wikipedia artikkeli aiheesta ja testailla peliä eri rooleilla.

Vaikka tähän mennessä olleissa deadlineissa ei ole vaadittukaan niin kommentointi voisi helpottaa metodien toiminnan ja rakenteen ymmärtämistä. Metodit ovat kuitenkin selkeitä, eivät liian pitkiä ja helposti luettavia, joten koodia pystyikin lukemaan mielestäni todella hyvin sekä se noudatti clean code periaatteita.

Koodissa itsessään huomasin vain yhden virheen, joka sekin sijaitsi tekstikäyttöliittymässä, josta oli erikseen ilmoitettu että se on vain väliaikainen ratkaisu ennen graafista. TekstiPelinLuoja luokassa rivillä 84 sijaitsevassa metodissa. String nimi alustetaan "", sitten sille annetaan kahdesti lukija.nextline() arvo, jonka jälkeen luodaan nimellä varustettu pelaaja. Nimi saa tällöin kaksi syötettä, mutta vain jälkimmäinen on merkityksellinen ja lisätään pelaajan nimeksi. Itse pelissä tällöin hahmon nimen antaminen on hieman sekavaa, koska se pyytää nimen ja tämän jälkeen jää odottamaan seuraavaa ilmoittamatta erikseen. Aika pieni ja helposti korjattava bugi kuitenkin. Muuten koodi oli mielestäni todella hyvää ja selkeää. Nimentä, pituus ja metodien tarkoitus oli mielestäni selkeää.

Sovelluslogiikka pakkauksessa on nyt aika paljon eri luokkia, luokkia voisi varmaan jakaa selkeyden vuoksi omiin pakkauksiinsa, kuten vaihe, rooli..

Luokkakaavio oli erinomainen, selkeytti todella paljon ohjelman rakennetta ja toimintaa. Huomattavasti parempi kuin mitä omani ja aiheuttikin välitöntä oman luokkakaavion parantelua. :) Tekstikäyttöliittymissä oli välillä hieman sekavia ja osa roolien erikoistoiminnoista meni hieman osi. Graafinen käyttöliittymä varmasti selkeyttää tätä tulevaisuudessa. Tekstikäyttöliittymän kautta oli mielestäni kivaa päästä tutustumaan peliin ja kokeilla miten eri toiminnot toimivat. En onnistunut saamaan peliä myöskään kaatumaan yrittämisestä huolimatta.

Testejä alkaa olemaan mielestäni hyvin, vaikkakaan ei vielä kaikilla luokilla ja testit vaikuttival järkeviltä ja tarkoituksenmukaisilta. Yksi testi failasi, mutta vain koska testiluokalla ei ollut vielä testejä.

Kaikenkaikkiaan mielestäni todella hyvä ja mielenkiintoinen aihe ja hyvin toteutettu, jäi hyvä kuva projektista.

[artokaik]

Tuon kommentoinnin tärkeyden tosiaan huomaa vasta kun lukee toisen koodia :) Pitää seuraavaan dedikseen yrittää parantaa.

Luokkien jakaminen useampiin pakkauksiin on varmasti hyvä idea, varsinkin kun noita rooleja tulee vielä lisää.

Siinä tekstikäyttiksessä on kaksi lukija.nextlinea, koska ensimmäistä pelaajaa syötettäessä se jostain syystä tallensi sen automaattisesti ilman nimeä. Muuta testausta varten viritin tuollaisen purkkaratkaisun, kun en nopeasti keksinyt mistä se alkuperäinen bugi johtui. Muutenkin tekstikäyttis on tehty lähinnä sovelluslogiikan testaamista varten sekä varmistamaan, että sovelluslogiikan ja käyttöliittymän eriyttäminen onnistuu.

https://www.lucidchart.com on aika hyvä työkalu noiden kaavioiden tekoon, varsinkin jos niitä pitää muokata jälkikäteen. Ilmasessa versiossa on 60 itemin rajoitus per kaavio, mutta yli 60 itemin kaavio alkaa olla jo aika sekava.

[cobrelli]

Joo arvelinkin että jotain tälläistä oli tuolla lukija.nextlinessa. Dokumentoinnissa olikin huomioitavissa juttua juuri tuosta tekstikäyttöliittymän väliaikaisuudesta.

Hei kiitos, täytyypä tutustua tarkemmin tuohon lucidcharttiin. :)

[Kari-R]

Tällä projektilla onkin ilmeisesti sitten kaksi katselmoijaa. En tiedä keksinkö välttämättä paljoa uutta sanottavaa.

Pidin koodissa varsinkin siitä, että siinä käytetään kuvaavia tunnistenimiä, mikä tekee siitä itsedokumentoivaa. Luokkien toiminnasta saa nopeallakin vilkaisulla selvää, vaikka niitä ei vielä olekaan erikseen selitetty kommenteilla tai javadocilla. Pakkauksetkin on toteutettu asianmukaisesti niin varsinaiselle ohjelmakoodille kuin testeillekin, ja käyttöliittymä ja sovelluslogiikka on erotettu toisistaan kuten kuuluukin. Olet myös saanut hyvin pidettyä metodit pienikokoisina, vaikka joissain kohti onkin muutamia vähän pidempiä metodeja, kuten esimerkiksi TekstiPelinLuoja-luokassa. Yksi hyvä tapa hoitaa niiden jakaminen olisi esimerkiksi tehdä niissä oleville silmukoille omat metodinsa. Eiväthän nuo pidemmätkään metodit mitenkään hirveän isoja ole, mutta ainakin itse yritän pitäytyä varmuuden vuoksi siinä "pituus korkeintaan 10 riviä"-periaatteessa.

Projektissa näkyy monessa kohtaa hyvän oliosuunnittelun periaatteet. Se vaikuttaisi noudattavan tunnollisesti Single Responsibility Principleä esimerkiksi siinä kuinka yön ja päivän toiminnot on eroteltu omiksi luokikseen. Pelaajien ja Roolien suhdekin on ratkaistu taitavasti "favor composition over inheritance"-periaatetta noudattaen, mikä lisää ohjelman muokkautumiskykyä, jos rooleja tahdottaisiinkin vaikka myöhemmin lisätä. Olet myös valinnut käyttää rajapintoja abstraktien luokkien asemesta, mitä myös pidetään hyvänä ratkaisuna Java-koodeissa. Mietin, että roolien toiminnot voisi ehkä toteuttaa kokonaan niiden omissa toimi()-metodeissaan, ja TekstiRoolienToiminnot-luokka voitaisiin sitten muuttaa joksikin KysyKayttäjänSyote-luokaksi. En tiedä olisiko siitä sen suurempaa hyötyä, mutta tuntuisi ainakin minusta selkeämmältä, kun koodissa ei pompattaisi erikseen vielä toiseen luokkaan.

Testejä olisi mielestäni saanut olla vähän enemmän, mutta ei kai vielä tässä vaiheessa mitään hirveää testikattavuutta vaadittukaan. Itseltäni jäi testien vähäisyyden takia omassa projektissani eräästä luokasta muutama vakava suunnitteluvirhe huomaamatta, ja olin jo ehtinyt tehdä useamman luokan jotka nojasivat sen toimivuuden varaan.

Katselmointi on tehty 4.1.2013 klo 20:00 ladattujen lähdekoodien pohjalta. En saanut jostain syystä NetBeanssillani ajettua ohjelmaa tai testejä, mutta koodiahan tässä arvioidaankin.