Koodikatselmointi 2

[Virta]

Katselmoitava koodi: tpatja, miinat

Tutustuin koodiisi jo 7.6. 1200 alkaen.

Synkattu 9.6. 0830, jonka perusteella katselmointi tehty.

Koodisi on pääasiassa selkeää ja helposti luettavaa, paljon toiminnallisuuksia joita en ole käyttänyt enkä siksi voi ihan kaikkeen ottaa kantaa.

Luokista:

• HighScoreEntry: Kannattaisi pilkkoa pienempiin osaluokkiin; tulosten kirjoittaminen ja lukeminen on toiminnallisuutta joka tulisi clean coden mukaan olla omassa luokassaan.
• Harkitse, kannattaisiko HighScoreEntry ja siihen olennaisesti liittyvät luokat siirtää omaan pakettiinsa; nehän ovat lähinnä tallennukseen liittyviä, voisivat siksi olla erillään pelilogiikasta.
• MiineEngine: Kannattaisi pilkkoa Level-määrätyt arvot omaan luokkaansa, clean code.
• ConsoleUI: Kannattaisi pilkkoa GameState-määrätyt arvot omaan luokkaansa, clean code.
• GUI: kannattaisi pilkkoa UiSquare-luokka omaan luokkaansa, jota tämä luokka käyttää, clean code. Toistoa: GameState-määrätyt arvot, jos tämä olisi oma luokkansa, ei tulisi toistoa. Metodit initUI, updateUI voisi pilkkoa pienempiin osametodeihin, clean code. Kannattaa harkita myös toiminnallisuuden (MouseListener implementation) pilkkomista omaan luokkaansa erilleen GUI:n näyttämisestä, clean code.
• HighScoreDialog: kannattaisi pilkkoa konstruktori pienempiin metodeihin, esim 24-32, 34-49.

Testeistä:

• Testit HighScore*:lle ilmeisesti vielä kesken; levylle kirjoitusta/lukua ja poikkeusten heittämistä kannattaa testata. -Comparator ja -Adapter kannattaa myös testata.
• Muut testit hyviä, kattavuus hyvä, kaikki menee läpi.

Dokumentaatio ei kuulu katselmoinnin piiriin, mutta mainittakoon, että on aika hyvällä mallilla.

[tpatja]

(kohdat 1 ja 2): HighScoreEntry on simppeli datasäiliöluokka yhdelle high score tietueelle. Tarkoitatko HighScoreManager:ia?

• tämänhetkinen pakettijako engine & ui riittänee, kun luokkia on noin vähän
• Level ja GameState on nyt omissa tiedostoissaan
• UiSquare on GUI luokan toteutuksen sisäinen osa (simppeli säiliö), eikä sitä tarvita ulkopuolella, eli jätän sen nested luokaksi
• GUI:n ja HighScoreDialogin pitkät metodit on nyt pilkottu lyhyemmiksi
• testeistä sen verran että levy IO on tarkoituksella jätetty pois, koska yksikkötestauksessa ei käytetä levy IO:ta. Comparator luokkaa käytetään entryjen sorttaamiseen ja sille on testi HighScoreManagerTest luokassa. HighScoreManagerAdapter on interface jota ei siis suoraan voi testata, mutta sitäkin käytetään HighScoreManagerTest:ssä.

[Virta]

Kohdissa 1 & 2 viittasin aiemmin HighScoreEntryyn, mutta katsoin ilmeisesti väärin. -Manager luokassa on toki levy IO:ta jonkin verran, mutta luokassa metodit nivoutuvat kyllä yhteen nätisti. Periaatteessa siitä voisi pilkkoa levyIO:n omaan luokkaansa, mutta ei sitä loppupeleissä paljoa ole. Toki olin omassa koodissani pilkkonut levyIO:n omaksi luokakseen vaikka se sisältääkin vain kaksi varsinaista levyIO metodia.

Pakettijaolla viittasin lähinnä HighScore- luokkien eriyttämiseen Square ja Engine- luokista.

Rajapintoja ei tosiaan voi suoraan testata, my bad. Onko väärin testata levyIO:ta yksikkötesteissä?

On 09.06.2013 19:06, tpatja wrote:

(kohdat 1 ja 2): HighScoreEntry on simppeli datasäiliöluokka yhdelle high score tietueelle. Tarkoitatko HighScoreManager:ia?

*

tämänhetkinen pakettijako engine & ui riittänee, kun luokkia on
noin vähän

*

Level ja GameState on nyt omissa tiedostoissaan

*

UiSquare on GUI luokan toteutuksen sisäinen osa (simppeli säiliö),
eikä sitä tarvita ulkopuolella, eli jätän sen nested luokaksi

*

GUI:n ja HighScoreDialogin pitkät metodit on nyt pilkottu lyhyemmiksi

*

testeistä sen verran että levy IO on tarkoituksella jätetty pois,
koska yksikkötestauksessa ei käytetä levy IO:ta. Comparator
luokkaa käytetään entryjen sorttaamiseen ja sille on testi
HighScoreManagerTest luokassa. HighScoreManagerAdapter on
interface jota ei siis suoraan voi testata, mutta sitäkin
käytetään HighScoreManagerTest:ssä.

— Reply to this email directly or view it on GitHub https://github.com/tpatja/miinat/issues/2#issuecomment-19168579.

Ystävällisin terveisin, Frans Ojala

[tpatja]

Yksikkötestauksessa jokaisen testin pitäisi olla "self contained", eli toimia aina samalla tavalla riippumatta ulkoisista tekijöistä (esim käyttöjärjestelmäkohtaiset erikoisuudet, levyn ja levyajurin tila jne). Jos ulkopuolisia tekijöitä on oltava, niitä pitää pystyä hallitsemaan. Esim. dependency injection ja mock luokat mahdollistavat sen.

Jos halutaan tehdä tallennus ja lataus testattavaksi tuossa minun HighScoreManager luokassa, se onnistuisi esim näin:

• annetaan konstruktorissa OutputStream ja InputStream oliot luokalle dependecynä
• käytetään em streameja tallennukseen ja lataukseen -> nyt luokka ei enää ota kantaa siihen käyttääkö streamien toteutus ByteArray vai File IO:ta. Yksikkötestissä voi antaa ByteArray stream instanssit (ByteArrayInputStream ja ByteArrayOutputStream) ja enginestä luotaessa File stream instanssit.

Kiitos muuten palauttesta. Ilman sitä olisi jäänyt varmaan siistimiset tekemättä.

[Virta]

Pitää varmaan kattoa omat tallennuksen testit ja ehkä muokata niitä.

Mutta ole hyvä vain, tätä varten katselmointeja on =).

Virta

On 09.06.2013 21:54, tpatja wrote:

Yksikkötestauksessa jokaisen testin pitäisi olla "self contained", eli toimia aina samalla tavalla riippumatta ulkoisista tekijöistä (esim käyttöjärjestelmäkohtaiset erikoisuudet, levyn ja levyajurin tila jne). Jos ulkopuolisia tekijöitä on oltava, niitä pitää pystyä hallitsemaan. Esim. dependency injection ja mock luokat mahdollistavat sen.

Jos halutaan tehdä tallennus ja lataus testattavaksi tuossa minun HighScoreManager luokassa, se onnistuisi esim näin:

• annetaan konstruktorissa OutputStream ja InputStream oliot luokalle dependecynä
• käytetään em streameja tallennukseen ja lataukseen -> nyt luokka ei enää ota kantaa siihen käyttääkö streamien toteutus ByteArray vai File IO:ta. Yksikkötestissä voi antaa ByteArray stream instanssit (ByteArrayInputStream ja ByteArrayOutputStream) ja enginestä luotaessa File stream instanssit.

Kiitos muuten palauttesta. Ilman sitä olisi jäänyt varmaan siistimiset tekemättä.

— Reply to this email directly or view it on GitHub https://github.com/tpatja/miinat/issues/2#issuecomment-19171239.

Ystävällisin terveisin, Frans Ojala