Koodikatselmointi

Yleistä

Perustuu aamulla 1.5.2021 ladattuun koodiin.

En saanut release 1:n .jarria toimimaan java -jar WeightandBalance-1.0-SNAPSHOT.jar komennolla. Tulostuu Javan virheilmoitus:

ClassNotFoundException: com sun.prism.es2.X11GLFactory
...

Sain sovelluksen kuitenkin toimimaan hakemalla repon zipin (aamulla 1.5.21) ja ajamalla komennon mvn package. Tämäkään ei toiminut heti, vaan antoi ilmoituksen virheellisestä luokkatiedoston nimestä wbapp/dao/acDataDao.java. Kun nimen korjasi isolla alkavaksi, niin maven suostui tekemään jarrin target hakemistoon, joka vihdoin toimi.

Lisää pienempää ongelmaa tuotti Postgressin käyttö projektissa. Projektille mielestäni ehkäpä sopisi paremmin kevyempi ja helpommin käytettävä sqlite. Yritin myös luoda taulut käyttämällä annettua schemaa, mutta se kaatui virheisiin. Annettu referenceValues.txt sisälsi kuitenkin toimivat komennot.

Yleiskatsaus koodista

Ohjelmaa on mielestäni yritetty jakaa ihan järkevästi eri luokkiin niin, että esim. käyttöliittymä ei tuntisi tietokantaan liittyvää implementaatiota. Tässä ei kuitenkaan mielestäni onnistuttu täysin, sillä tietokanta-implementaation enkapsuloivat Daot (UserDataDao ja AcDataDao) tällä hetkellä palauttavat public kysely-metodeilla suoraan kannasta saatuja ResultSettejä ja SQL-exceptioneitä. ResultSettien palauttaminen sellaisenaan vuotaa "julki" kannan toteutuksesta taulujen sarakkeet, joka siis lisää riippuvuuden tietokannan sisäisen muodon ja Daon käyttäjien kesken rikkoen enkapsulaation hyödyn. Tekemällä ResultSeteistä sopimuksen mukaiset listat (tai muun olion) ja palauttamalla ne, Dao-luokat piilottaisivat mielekkäämmin tietokannan sisäisen muodon Daon käyttäjiltä. Tämä mahdollistaisi esim. kannan muuttamista ilman, että muita luokkia tarvitsisi muuttaa. Sama myös pätee SQL-exceptioneihin, mutta ehkä vähän enemmän teoreettisella tasolla. Mitä jos tietokanta vaihdetaan vaikkapa pilvipohjaiseen tallennukseen, eikä se enää heitäkkään virhetilanteessa SQL-virheitä?

Tästä päädytään myös ongelmaan ohjelman rakenteessa: projektissa ei ole "ohjelmoitu interfaceen" eli käytettäisiin luokkia interfacen kautta, vaan toteutuksen kautta. Tälläisenään on Daon toteutuksen vaihtaminen toiseen hankalaa.

Toteutuksessa käytetään kiitettävästi riippuvuuksien injektointia ja yritetään välttää turhia riippuvuuksia luokkien kesken. En myöskään löytänyt liian pitkiä metodeja, josta kiitosta kirjoittajalle.

Luokkien toteutuksesta

wbapp/dao/UserDataDao.java:

• Arkkitehtuurikommentit liittyen Daojen toteutukseen.
• Metodien nimistä: checkName hieman hämäävä, toimisiko esim. findName tai jokin sellainen paremmin? sama checkPasswordille.

wbapp/dao/acDataDao.java:

• Arkkitehtuurikommentit liittyen Daojen toteutukseen.
• Tiedoston nimi tulisi alkaa isolla kirjaimella.
• On hieman hankalempaa sanoa nimistä mitään, kun en niin ohjelman tarkoituksesta ymmärrä. Ehkäpä getCount:n voisi nimetä kuvaamaan tarkoitusta paremmin. Olisiko depend-loppuisille metodeille myös jokin parempi nimi? Liittyykö näiden nimien ongelmaan yleisemmin AircraftDatan tiedon jäsentelyn ongelmat?

wbapp/domain/Aircraft.java:

• Metodien ja muuttujien nimistä voisi tiputtaa sanan "ac" pois. Minkä muun kuin lentokoneen id:tä haetaan, kun lentokone olion getId()-metodia kutsutaan?

wbapp/domain/AircraftData.java:

• Johonkin tiettyyn lentokoneeseen liittyvä luokka AircraftData on täysin irrallaan Aircrafistä? Eikö jokaisella lentokoneella voisi olettaa olevan referenssi omaan Dataansa?
• Luokassa on turhia metodeja, joita ei kutsuta ollenkaan. Esim. getmaxWeight(), joka myös rikkoo camelCase nimeämistä.
• Metodi getCount2() ei ole kovinkaan kuvaava tehtävästä työstä? Mitä tarkoittaa count2?
• Luokalla on 3 2-ulotteista taulukkoa oliomuuttujina, joiden nimet ovat dataList, dataList2 ja dataList3. Nämä nimet ovat todella sekavia ulkoiselle lukijalle. Mitä ne tarkoittavat ja mitä niillä tehdään? Samoin kyseisiä listoja käytetään vain kylmästi numeerisilla indekseillä. Esim. mitä tarkoittaa dataList[1][3]? On helppo ymmärtää, kun ohjelmaa kirjoittaa ja kaikki on tuoreellaan mielessä, mutta entä pari kuukautta myöhemmin?
• Muuttujien nimeämisessä metodien sisällä on ongelmia: esim createFullItemList():n muuttujat p, k, i ja l eivät ole yhtään kuvaavia siitä mihin niitä käytetään.

wbapp/domain/AircraftList.java:

• Kuinka mielekästä on tehdä riippuvuus sovelluslogiikan koodiin yksittäisen graafisen käyttöliittymän tietorakenteisiin (JavaFX:n collectionit)? Entä jos käyttöliittymäkirjastoa muutetaan?

wbapp/domain/User.java:

• Kannattaisiko käsitteet käyttäjästä (User) ja käyttäjän hallinnasta (login yms) eriyttää?

wbapp/domain/WBCalculator.java:

• Samoja nimeämisongelmia kuin aiemmin. Index ja count?
• Ilmeisemmin indexin ja countin ei tarvitse olla olion muuttujia. Myöskään dataa ei erikseen missään uudelleenkäytetä.

wbapp/ui/Ui.java:

• Eri näkymistä voisi tehdä luokkia, jonka ansioista UI-luokan kokoa saisi pienemmäksi. Mitä tapahtuu luettavuudelle, jos ohjelman käyttöliittymä tästä vielä monimutkaistuu?

Testit

Esim. UserTestissä voisi myös testata uuden käyttäjän luontia, joka vaatisi, että testitaulut tyhjennetään alussa. Huomasin myös, että testit eivät voi epäonnistua, sillä assertEquals(false, false); tyyliset lauseet ovat käsittääkseni aina totta. Ohjelman testikattavuus ei siis kerro mitään.