Koodikatselmointi 1: to 23.8. n. klo 19:19

Katselmoinnin aloitus ja repositorion lataus omalle koneelle to 23.8. n. klo 19:19

Yleinen positiivinen palaute:

Kaikki testit menevät läpi ja ohjelman ydintoiminnallisuus eli kartan generointi toimii
Uusia karttoja generoidaan onnistuneesti, tämän huomaa siten että avaa kartan sisältävä tekstitiedoston ja sitten ajaa ohjelman uudelleen ja huomaa että levyllä tiedosto on muuttunut (reload näyttää muutokset).
Parametrien muutokset näkyvät: roomAttempts lisää huoneita, height lisää korkeutta jne
Muuttujat, metodit ja luokat nimetty hyvin ja ymmärrettävästi
Koodi on selkeää (vaikka ihan kaikkeen en ehdikään paneutua syvällisesti tässä katselmoinnissa) eli työn antaminen toiselle jatkokehitettäväksi olisi melko mutkatonta.

Parannusehdotuksia ja (sivu)huomautuksia:

HomemadeRandom luokka

HomemadeRandom luokassa nextInt() metodissa käytetään jakolaskua kun jakajana (divider) on kakkosen potenssi eli 65536 = 2^16. Kuten TiTossa opittiin, luvun bittien siirto yhden vasemmalle kertoo luvun kahdella ja bittien siirto oikealla jakaa luvun kahdella. Näin ollen jakolaskun pystynee korvaamaan ekvivalentilla bittimanipuloinnilla eli bittien siirrolla oikealle eli:

(int) (seed >> 16)

Tämän pitäisi antaa sama tulos ei-negatiivisilla luvuilla (eli positiivisilla luvuilla ja nollalla) mutta tietokoneelle paljon nopeammalla bittioperaatiolla. Koska näitä pseudosatunnaislukuja tuotetaan joka iteraatiolla esimerkiksi luokan DungeonMap addRooms() metodissa, niin tehokkuushyöty voi olla merkittävä (mahdollisesti). Toki en tiedä osaako compiler automaattisesti tunnistaa tällaiset optimointimahdollisuudet jolloin oma säätö olisi turhaa.

Linkkejä aiheesta: https://docs.oracle.com/javase/tutorial/java/nutsandbolts/op3.html https://en.wikipedia.org/wiki/Bitwise_operation#Shifts_in_Java

DISCLAIMER: Kannattaa vielä tarkistaa, että tämä toimii myös rajatapauksissa ennen kuin tekee muutokset ja kiinnittää huomiota että merkki säilyy jne.

Lisäksi

Luvut joiden on tarkoitus säilyä muuttumattomina (kuten vaikka divider) voi laittaa lopulliseksi: final long divider jne.

HomemadeRandom luokassa voisi konstruktorissa olla lyhyt selitys valittujen numeroiden logiikalle vaikka linkki lähteeseen onkin luokan alussa. Kuitenkin osa noista numeroista on hyvinkin tarkalla perusteella valittu ja niitä ei mahdollisen toisen kehittäjän parane muuttaa, jos ei tiedä mitä tekee.

RoomBuilder

RoomBuilder luokassa voinee lisätä @Override kaikkiin interface metodeihin (NetBeans huomauttaa).

RoomBuilderTest luokassa testataan useampia taulukoita pelkällä assertEquals-metodilla, toisaalta myöhemmin on käytetty assertArrayEquals-metodia. En tiedä onko tälle vertailutavalle joku erikoinen syy, testit toki menevät läpi mutta NetBeanskin selkeästi huomauttaa tästä.

DungeonMap

Karttaa tallennettaessa try-with-resources on vaihtoehto jossa resurssi suljetaan automaattisesti (myös virhetilanteiden sattuessa) ja ehkä myös käyttäjälle jonkinlaisen virheviestin antaminen poikkeuksen heittämisen lisäksi. IOExceptionin voisi esimerkiksi ottaa kiinni mainissa ja kertoa käytäjälle mikä meni pieleen.

NetBeans tekee automaattisesti muunnoksen painamalla alt+enter pw muuttujan kohdalla ja valitsemlla try-with-resources:

public void saveMap() throws IOException {
    FileWriter fw = new FileWriter("generated_map.txt");
    try (PrintWriter pw = new PrintWriter(fw)) {
        pw.print(toString());
    }
}

Sivuhuomatus testiluokista

Sivuhuomautuksena: Ainakin viimeistään loppupalautuksessa kannattaa poistaa testiluokista turhat käyttämättömät setUpClass(), tearDownClass() ja niihin liittyvät importit BeforeClass, AfterClass jne. Toki voi olla että näin on aiottukin tehdä ja ne ovat siellä koska autogeneroitu ja ei tarkalleen tiedetä tarvitaanko niitä.

Loppusanat

Tsemppiä erityisesti HashMapin ja ArrayListin korvaajien tekoon!

Granigan / dungeongenerator