Koodikatselmointi 1

[artokaik]

Haettu githubista pe 4.1. klo 20.30

Yleistä:

Hyvää:

• Ei ylipitkiä metodeja missään luokassa
• Suurin osa metodeiden ja muuttujien nimistä oli kuvaavia (poikkeuksena xor(boolean a, boolean b)).
• Luokat on jaettu pakkauksiin loogisesti
• Kaikkia luokkia on testattu ainakin jonkin verran
• Kaikki testit menevät läpi

Kehitettävää:

• Muutamassa kohdassa olisi ehkä kaivannut selventäviä kommentteja

Alueet:

En ihan ymmärtänyt miten TetrisPeliAlue luodaan, eli missä kohtaa on sijainti 0.0. Ilmeisesti kuitenkin ylhäällä keskellä, koska uudet tetriminot lähtevät sieltä?

En myöskään ymmärtänyt miksi on erikseen Alue ja sen perivä TetrisPeliAlue. Mihin tarvitaan Alue-luokkaa?

Palikka, Sijainti:

Selkeät, hyvin nimetyt ja kompaktit luokat.

Tetriminot:

En ihan täysin tajunnut tuon Tetriminopalikkakokoelma-luokan tarkoitusta. Tetriminohan on periaatteessa pelkkä käänneltävä ja liikuteltava palikkakokoelma. Ilmeisesti tetriminon metodeita käytetään pääasiassa kun tetriminoa liikutetaan ja –palikkakokoelman metodeja kun tetriminoa luodaan? Vai onko niiden eriyttämiseen jokin muu syy? Onko koko palikkakokoelma-luokka tarpeellinen, vai voisiko sen korvata ArrayListillä ja Sijainnilla Tetriminossa?

Tetriminonrakentaja:

Metodit ovat lyhyitä ja nimistä on pyritty tekemään kuvaavia. Koodin luettavuutta ehkä vähän hankaloittaa se, että sisäkkäisiä metodikutsuja on paljon ja toisaalta metodien nimet ovat hyvin samantyyppisiä. Metodit voisivat myös olla koodissa siinä järjestyksessä kun ne suoritetaan.

Omasta mielestäni koodi on helppolukuisempaa jos sisäkkäisiä metodeja on vähemmän, vaikka se tekisikin ”päämetodista” vähän pidemmän. Vaikkapa seuraavaan tyyliin:

rakennaTetrimino(int palikoidenMaara)

  tetrimino = new Tetrimino
  tetrimino.lisaaEkaPalikka();
  for i = 2 to palikoidenMaara

   ArrayList<Sijainti> viereiset = tetrimino.etsiViereisetVapaatPaikat()
   Sijainti sijainti = arvoSijainti(viereiset)
   tetrimino.lisaa(new Palikka(sijainti))

Tällä hetkellä tuossa koodissa arvotaan mielestäni turhaan erikseen palikka, jonka viereen uusi palikka tulee, ja sen palikan viereinen sijainti.

Törmäykset:

Pikaisella katsomisella ei heti selvinnyt mitä eri törmäykset ovat. Tämän olisi ehkä voinut kommentoida luokan alkuun.

[artokaik]

Hyvin nukutun yön jälkeen luin tuon katselmointi uudestaan, ja siitä jäi ehkä vähän negatiivinen kuva. Kokonaisuudessaan koodi oli erittäin siistiä ja siitä piti ihan hakemalla hakea noita kehityskohteita.

[Kari-R]

En ole vielä kirjoittanut kommentteja, koska niitä ei ole vielä edellisissä deadlineissä vaadittu. Alan kirjoittamaan niitä maanantain deadlineen.

Alue-luokka määrittää TetrisPelialueen ääriviivat. Sitä tarvitaan siihen, että voidaan katsoa onko esimerkiksi tetrimino pelialueen sisäpuolella. Pelialueella ei ole varsinaisesti mitään yksikäsitteistä (0,0)-pistettä, koska sijainnit lasketaan suhteessa sen alkupisteeseen. Olen nyt muuttanut Pelialuetta niin, että Aluetta ei enää peritä, vaan käytän kompositiota. Päivitän muutokset Gittiin maanantaina.

TetriminoPalikkakokoelman tarkoitus on olla palikkakokoelma, joka pitää huolta ns. tetriminosäännöstä. Eli että kaikki tungetut palikat ovat toistensa vieressä. En usko, että jollain yksinkertaisella ArrayList-virityksellä voisi korvata tuota luokkaa. Olen kyllä miettinyt ehdotustasi, että Tetrimino ja TetriminoPalikkakokoelma yhdistettäisiin samaksi luokaksi. Ainoa syy, miksi en ole vielä niin tehnyt, on se, että epäilen sen rikkovan jollain tapaa yhden vastuun periaatetta, koska silloinhan ko. luokalla olisi jo melkoisesti ominaisuuksia. Toisaalta sekin tuntuu vähän hölmöltä, että Tetriminossa on TetriminoPalikkakokoelma ja TetriminoPalikkakokoelmassa on Tetrimino. Täytyy miettiä.

Itsekään en oikein pidä tästä sisäkkäisten funktiokutsujen tekemisestä, mutta teen nyt näin, kun koodin laatuvaatimuksissa niin tahdottiin. Metodien esittelyjärjestyksen muuttaminen oikeaan järjestykseen kuulostaa hyvältä idealta.

Tuo tetriminot rakentava koodi ei tosiaan ole kovin optimoitua. En jaksanut miettiä sitä sen pidemmälle, koska palikoiden määrä pysyy kuitenkin koko ajan todella pienissä lukemissa (Tetriksessä enintään kolmessa, kun on enää yksi laittamatta), ja koska tämä ei ole TiRa-labra niin siitä ei varmaan tipu pisteet. Jos aikaa jää, otan käyttöön tuon ehdottamasi ratkaisun.

Kiitos palautteesta.