vihaton / kiilto Goto Github PK

Yleistä

Ladattu: 15:34 - 19.02.2016

• Hyvin paketoitu. Tosin aputiedostot kannattaa siirtää lähdekoodille suunnatun kansion ulkopuolelle.
• Hyvin sisennetty, helposti luettavaa (joitain luokkia lukuunottamatta).
• Testit erittäin kattavia.
• Bonuspisteet toStringin toteuttamisesta hyvin.
• Luokkien järjestykset loogisia: muttujat, konstruktorit, metodit.

logiikka.*

Pelaaja

- Yleisesti, jos luokka toteuttaa rajapinnan, tulisi sen tyypiksi asettaa rajapinta, eikä implementaatio. (kts. [tämä](https://en.wikipedia.org/wiki/Liskov_substitution_principle) ja [tämä](https://en.wikipedia.org/wiki/Dependency_inversion_principle)) - Vaikkei luokkamuuttujan luominen konstruktorin ulkopuolella vaikuta mihinkään, olisi silti järkevintä joko luoda kaikki konstruktorin sisällä tai sen ulkopuolella. - Nimeämiset ovat selkeitä ja koodi luettavaa. - `liikaaKarkkeja` voitaisiin tiivistää yhteen riviin: `return karkit.getKarkkienMaara() > 10;` Sama metodiin `voittaja`. - Joissain metodikutsuissa metodin nimi trivialisoituu muuttujan tyypin vuoksi. esim. `omaisuus.getOmaisuudestaTulevatBonusKarkit();`: tässä tapauksessa `omaisuus.getBonusKarkit()` olisi ehkä kuvaavampi nimi. - `getHintaOmaisuustulotHuomioituna`: äärettömän helposti luettavaa, nimeämiset toteutettu erittäin hyvin. - `onkoVaraa`: vaikeasti luettavaa. Muuttujat tulisi nimetä siten, ettei metodi tarvitse rivien väliin apukommentteja. - Metodi `getVaraus`: täydellinen metodi [Optional](https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html)-luokan käyttämiseen.

Pelivelho

- `TUI` = ? (TextUI? edit: confirmed, suosittelen uudelleennimeämistä) - `this`-viittaukset vain 1) konstruktorissa, 2) jos parametri tai lähimuuttuja saman niminen kun luokkamuuttuja tai 3) viitataan itse luokkaan (esim. parametreissä). - Oletettavasti tekstin tulostus on `TUI`n tehtävä, joten metodin `pelaaKierros`/`pelaaVuoro` tulisi delegoida printtaukset sinne. - Sisennykset osittain kateissa, mutta luokka onkin selvästi testikäytössä.

Poyta

- `"/home/xvixvi/kiilto/kiilto/src/main/java/aputiedostoja/omistustentiedot.csv"` on huono tapa viitata ulkoiseen tiedostoon. Se toimisi vain itselläsi, mutta oletan että koodi on väliaikaisratkaisu. - `luoOmistukset` on melko kryptinen. Mikä on `j`?

Valineluokat

- `return;` Kasakokoelman konstruktorissa. - Satunnaiset `this` viittaukset: joskus viittaat oikein, joskus et ollenkaan (esim. Merkkihenkilon konstruktori). - `this("homo", ov, 3);` + 1 - Sisennys ja nimeämiset edelleen hyvää. Koodi on ymmärrettävää ja siistiä.

Omaisuusluokat

- Suunnittelussa outo epäsäännöllisyys: joissain metodeissa testaat, että parametri on raja-arvojen sisällä, kun taas toisissa teet suoria kutsuja ilman tarkistuksia. Ajattelen, että joko tarkistat raja-arvoja turhaan sillä koodi ilmeisesti ei koskaan anna vääriä syötteitä, tai tiettyihin metodeihin tarkistukset on unohdettu laittaa. - `siirraOmistus` ja `poistaOmistus` - `getNakyvaOmistus`: [Optional](https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html)

ui.*

- `selvitaPelaajienMaara`: älä käytä exceptioneita näin. Kutsu vaikka `continue` ja aseta `pm` rajojen tarkistusten jälkeen. - `tarkistaKarrkienNostamisSyote` kannattaa uudelleenmiettiä. Saisit esim. tiivistettyä kaiken `throw new`-copypasten yhteen. - Kaikenkaikkiaan projekti ei sisällä mitään vakavia virheitä tai mystiikkaa, lähinnä mikroskooppisia outouksia ja muita häröyksiä. Suunnitteludokumentista oli avuksi ja yleinen dokumentointi oli jokseenkin kattavaa, vaikkakin jossain metodeissa sitä olisi kaivattu.

Ladattu: 5.2.2016 klo 17.16

Yleisesti:
-Siistiä koodia jokaisessa luokassa.
-Ohjelma jaettu loogisiin paketteihin ja paketit nimetty selkeästi.
-Luokat huolehtivat kukin omasta asiastaan ilman päällekkäisyyksiä, myös luokat nimetty selkeästi.
-Metoditkin on ohjeidenmukaisesti nimetty, ja jokainen tekee vain yhden asian.

Kasakokoelma:
-Kommentointi selkeytti hyvin toimintoja.

Testit
-Kaikki testit menivät läpi. (Pöytätestejä varten piti luonnollisesti hieman muokata aputiedoston osoitetta pöytä luokassa.)

Muuta
-En väkisinkään löydä mitään parannettavaa/kritisoitavaa koodista.