AaaDee commented 5 years ago

Koodikatselmus 1 (ladattu 5.6. klo 20)

Projekti näyttää rakenteensa ja dokumentaation puolesta erittäin toimivalta ja hyvin edenneeltä, hieno homma!

Dokumentaatio ja käyttöliittymä:

(näistä ei toivottu erikseen kommentteja, mutta lyhyesti kuitenkin)

Dokumentaatiossa (esim. project specification) olisi hyvä olla melko alussa itse algoritmien ohessa maininta siitä, että missä muodossa ja minkälaisella käyttöliittymällä itse ohjelma tullaan toteuttamaan. Esimerkiksi pelkkä lyhyt maininta (tyyliin "toteutetttava ohjelma on suoritettava java-sovellus, joka pakkaa parametrinä annettavan tiedoston käyttäjän valitsemalla algoritmilla") olisi hyvä.

Käyttöliittymä: Ilmeisesti ohjelma toimii mainin kautta, ja erillistä käyttöliittymää ei toteuteta? Jos näin, niin olisi hyvä että käyttäjän kannalta relevantit parametrit (lähinnä varmaan tiedostosijainti ja käytettävä algoritmi) olisivat heti alussa, ja muu ohjelman käynnistys olisi ulkoistettu vaikkapa omaksi initialize-metodikseen, joka saa nämä asetukset parametreinä.

Sovelluksen käyttö

Ohjelmasta ei ainakaan tällä hetkellä ollut suoritettavaa versiota. Tämä varmasti work in progress, mutta loppuun mennessä olisi hyvä olla selvästi nähtävissä (ja dokumentoituna) että miten homma toimii.

Omalla koneellani projektin buildaaminen ja suorittaminen ainakin aiheuttivat errorin, eli myös tästä syystä suoritettava jar-tiedosto olisi kätevä.

Koodi

Ohjelman pääduuni vaikuttaisi tapahtuvan compress-luokan metodissa compressString. Metodi on 50 rivin mittaisena jo melko pitkä, ja se sisältää 4 sisäkkäistä if- tai while-lauseketta. Tämä alkaa olla jo melko raskasta, eli metodia olisi hyvä jakaa pienempiin osametodeihin.

Metodin alussa searchWindosStart-muuttuja määritellään ?- ja :-operaattoreiden avulla, kun taas seuraavaksi käytettään if/else lausekkeita. Nämä ovat käsittääkseni tässä yhteydessä ekvivalenttejä, joten käyttäisin mielellään vain toista ratkaisua. Oma preferenssini on if/else, koska ?/: on ainakin omasta mielestäni hankalampi hahmottaa.

Lisäksi myös itse luokka LZ77Compress on turhan pitkä (suositus on kuitenkin yleensä alle 100 riviä). Luettavuuden kannalta olisi hyvä refaktoroida luokkaa useampiin alaluokkiin.

Testit

UI-luokassa testissä writingDictionaryToAndReadingFromFilesWorks() on kaksi assertEquals-lauseketta. En ole ihan varma, että mitä nämä kaksi asserttia mittaavat, mutta olisi luultavasti selkeämpää, jos ne olisi eristetty omiksi testeikseen (mahdollisesti jaetulla beforeClass-metodilla).

LZ77Test-luokassa on hieman sama ongelma, eli testit näyttäisivät testaavaan useampaa asiaa yhtä aikaa eri asserteilla. Erityisen paha on compressedEntryHasCorrectProperties()-testi, joka näyttäisi testaavan ainakin kolmea eri metodia eri parametreillä. Jos tämä testi menee pieleen, niin ainakin itse en pystyisi millään sanomaan, että missä vika piilee, eli jakaisin testin useampaan, luultavasti kolmeen osaan.

Lopuksi

Kokonaisuutena näkisin kuitenkin, että ohjelman perusrakenne on hyvin kunnossa, ja edellämainitut asiat on helposti fiksattavissa ennnen loppupalautusta, eli kaiken kaikkiaan homma näyttää erittäin hyvältä. Tsemppiä siis loppukurssille, kyllä ne viimeiset kulmat vielä siitä hioutuu! -A

sainikumara commented 5 years ago

Hei kiitos paljon katselmoinnista! Kertoisitko vielä, millaista erroria buildaaminen tuotti?

AaaDee commented 5 years ago

Hei! Nyt kun kokeilin uudestaan, niin build olikin ok (oisko jotain maven-latauksia tullut taustalla), mutta run heitti seuraavaa:

exec-maven-plugin:1.2.1:exec (default-cli) @ CompressaoSemPerdas --- caught exception: /home/ad/Desktop/compressao_sem_perdas-master/CompressaoSemPerdas/resources/text Exception in thread "main" java.lang.NullPointerException at java.lang.String.(String.java:566) at compressao.IO.stringFromFile(IO.java:37) at ui.LZ77.main(LZ77.java:18)

Oma pikadiagnoosi on, että luettava tekstitiedosto ei löydy. Ja nyt kun kattelin uudestaan, niin ei löydy koko resources-kansiotakaan, kun se on gitignoressa.

Kokeilin vielä ajaa saman uudestaan poiskommentoiduilla vaihtoehdoilla, nyt virhe tulee rivillä 30 ilmeisesti taas resource-tavaran puutteesta.

-A

sainikumara / compressao-sem-perdas