Koodikatselmointi

[aromaa]

26.4.2019 22:35

Ohjelma vaatii, että on olemassa src/resources/resources/sounds kansio, jossa on kaikki tiedostot, vaikka nämä tiedostut kuitenkin löytyvät pakatusta JAR tiedostosta, sieltä niitä ei kuitenkaan ladata.

Myös yksi huonopuoli on myös, että tietokannan pitäisi sijaita kansiossa src/main/resources/db/scores.db, tämä olisi kuiteskin hyvä vaihtaa pääkansioon, sillä kun kyseisiä kansioita ei ole, ei niitä myöskään luoda ja tietokannan luonti epäonnistuu.

Ohjelman Main luokassa määritellään kaikki mahdolliset näkymät, nämä olisi hyvä siirtää omiin luokkiinsa.

Yksi kätevä luokka on TimeUnit, jonka avulla voi muunnella eri aikatyyppejä kuten ms -> ns, joka helpoittaa koodin luettavuutta

Tämä on kyllä ihan mielipide kysymys, mutta GameView luokassa, initializeRectangleBoards methodissa on tosi paljon peräkkäisiä array accesseja, näyttäisi vähän nätimmältä kun se tehtäisiin kerran ja sitten käytettäisiin local variablea

AbstractIdObject on musta vähän hassu käsitely, ainakin abstractina, koska se lukittaa sitten tohon base clässiin, vaihtaisin ainakin itse rajapinnaksi ja käyttäisin jtn muuta nimikettä, kuten "Identifiable"

Palojen initializeCoords koodia vois vähän selventää siten, että palottelee kaikki "dimenssions" omiin methodeihin ja sen sijaan, että käyttää kolmiulotteisia arrayta niin luo tyyliin jonkun setDimension(1, int[][]), tämä ainakin selkeyttäisi paljon

Jos haluaa siitä vielä yhden askeleen enemmän, voisi muuttaa sekä dimenssion, että symbol int tyypistä Enum tyyppisiksi, jolloin saisi selkeät nimet

Jos vielä haluee optimoida enemmän, niin Piece luokan voisi muuttaa geneeriksi Piece, jonka jälkeen sitten tekisi seuraavasti:

setPieceCoordinates(Dimenssion dimenssion, int x, int y), ja sitten genericin perusteella abstract Piece pistää siihen oikean symboolin

Yksi mikä on unohtunut on, että PreparedStatement ja ResultSet täytyisi sulkea lopuksi. Molemmat luokat implemantoi AutoClosable rajapinnan, joten ne voi laittaa vaan try() blockin sisään

[kordaniel]

Kiitoksia katselmoinnista! Todella hyviä huomioita sekä kehitysehdotuksia. Osasta olin tietoinen jo ennestään, mutta suurin osa auttaa minua ihan konkreettisesti. Jos vain saan ajan riittämään, niin aion kyllä tutkia kaikkia esittelemiäsi asioita ja myös totetuuttaa niitä :) PreparedStatement sekä ResultSetistä, olen ollut siinä käsityksessä, että niitä ei tarvitse sulkea kun käytössä on tuo try with resources-lohko. Eli että ne sulkeutuisivat automaagisesti? Todennäköisesti olen väärässä ja onneksi ainakin tuo on helppo ja nopea korjata! :) Sinulle palautteena vielä, että todella hieno projekti sinulla menossa!

[aromaa]

Tosiaan, Connection, PreparedStatement, jne ei tarvitse itse käsin sulkea kunhan ne ovat resource-lohkon sulkeiden sisällä eli tämä toimii:

try(Connection connection = this.getConnection();
      PreparedStatement statement = connection.prepareStatement("SELECT 1"))
{
}

Mutta tämä sen sijaan ei toimi oikein:

try(Connection connection = this.getConnection())
{
     PreparedStatement statement = connection.prepareStatement("SELECT 1")
}

Tämähän on käytännössä compiler tason "syntax sugar" ja tulostaa vähän eri koodia pakatussa JAR tiedostossa. Eli tekee manuaalisen sulkemisen compiler tasolla.

Ja kiitoksia, itselläni on menossa myös ihan mukava projekti :)

EDIT: Pienenä lisäyksenä voisi laittaa, että resource-lokho pätee kaikkiin AutoClosable rajapinnan implementoiville luokille