ShootingStar91
commented
4 years ago Kiitos tosi hyvästä vertaisarvioinnista. Paljon hyviä pointteja, etenkin tuo kun painotit dokumentaatiota ja testausta, koitan pitää mielessä että noista saa noin paljon pisteitä kun aiemmin niistä on minulla lähtenyt paljon pisteitä näissä harjoitustöissä. Pahoittelut myös kun jouduit lukemaan koodiani vaiheessa jossa se oli (ja on) vielä aikamoinen sotku; olin keskittynyt lähinnä huffmanin toimintaan enkä kommentointiin, tyyliin ja selkeyteen...
En ole varma mitä tarkoitat viimeisellä kappaleella, etenkin ArrayListin ja HashMapin hitaudesta. Toki nämä täytyy joka tapauksessa koodata itse mutta ajattelin kirjoittaa tästä silti:
List on vain rajapinta jonka ArrayList toteuttaa eikä oma luokkansa. Työssäni on siis tällä hetkellä pelkkää arraylistiä ja hashmappia. ArrayList ei tietääkseni ole hidas. Sen get(index) operaatio tapahtuu lukemalla suoraan taulukosta joka sen sisällä on, eli se on täsmälleen yhtä nopea kuin vaikka int muuttuja = taulukko[indeksi]. Siinä on se ero taulukkoon että sen kokoa voi muuttaa, ja jos alkiota lisätessä sen sisäistä taulukkoa pitää kasvattaa niin silloin toki tapahtuu paljon operaatioita kun vanha taulukko pitää kopioida uuteen, isompaan taulukkoon. HashMap ei myöskään ole hidas, sen get ja put operaatiot on O(1) (tosin se voi olla hitaampi joskus, monimutkaisista syistä, mutta käytännössä se on melkein aina O(1)). Toki esim. contains-metodi on hidas, mutta sekään tuskin on itse tekemällä nopeampi. :)
Jos tuossa on jotain väärää niin kerro ihmeessä. Muuten arviointisi oli erittäin hyvä, kirjoitit paljon hyviä vinkkejä jotka aion pitää mielessä.
Ensinnäkin Huffmannin ydin näyttää toimivalta. Onnittelut siitä. Tässä vaiheessa harjoitustyökurssia se on tärkeä asia. Alla olevilla kommenteilla on tarkoitus osoittaa asioita, joilla voit parantaa arvosanaasi loppuarvostelussa:
Koodin laadusta:
Koodissa on laadullisia ongelmia HuffmanCoder-luokassa. Encode- ja decode- metodit ovat aivan liian pitkiä vaikka kurssilla ei tarkkoja asetuksia checkstylelle olekaan annettu. Yli sata riviä on paljon. Yksi helppo tapa refaktoroida nuo pitkät metodit olisi tehdä jokaisen for- tai while-loopin sisällöstä oma metodinsa. Muodostuva metodi ottaa parametreina samat asiat, jotka menevät looppiin, suorittaa loopin ja palauttaa lopputuloksen. Vaihtoehtoisesti looppaus-metodi voi olla "void", jos se muuttaa ainoastaan luokkamuuttujien arvoja. Esimerkiksi Encode-metodissa on loopeille kommentitkin jo valmiiksi kirjoitettuna. Niistä saisi suoraan metodien kuvaukset.
Koodin kommentoinnnista: Jos käytät sisäisiä luokkia, olisi hyvä kommentoida myös niiden käyttötarkoitus. HuffmanCoder-luokka on vähän raskas luettava kommenttien osalta. Ensin kerrotaan, mitä HuffmanCoder tekee. Sen jälkee on sisäinen luokka Entry, jonka toimintaa ei kuvata mitenkään. Se vain on. Sitten palataankin taas Huffman-coderiin. HuffmanCoder-luokassa on myös paljon metodeja, joita ei ole kommentoitu. Tai no. Tarkkaanottaen kommentteja puuttuu monesta muustakin osasta työstä.
Muistathan, että loppuarvostelussa toteutetun ohjelman osuus on 30p/60p ja tästä 30 pisteestä 10 pistettä koostuu dokumentoinnista ja koodin selkeydestä. Toimivaan Huffman-algoritmiin verrattuna nuo ovat todella helppoja pisteitä. Niistä javadoc-kommenteista ja ylipitkien metodien siivoamisesta saa yhtä paljon pisteitä kuin siitä toimivasta algoritmista, joka työssä jo on.
Testaus:
Testauksestakin sanon heti alkuun että se on 10/30 pistettä. Yksikkötestaukseen kannattaa panostaa, sillä se on melko yksinkertaista ja mekaanista puuhaa. Vaikka testit eivät olisi kauttaaltaan "hyviä", keskikertaisistakin on luvassa iso määrä pisteitä verrattuna siihen, että testaus on jäänyt tekemättä.
Yksikkötestaus on melkoisesti alkutekijöissään. Kannattaa huomioida, että siihenkin menee jonkin verran aikaa, joten ei kannata jättää viime tippaan. Suosittelisin, että kun refaktoroit HuffmanCoder-luokkaa, teet samalla testejä. Jokaiselle loopille oma metodinsa ja jokaiselle metodille oma yksikkötestinsä. Yksikkötestaushan ei testaa algoritmin toimintaa vaan kunkin metodin toimintaa. Siinä testataan lähinnä sitä, palauttaako metodi oikean arvon. Kun decode- ja encode -metodit on purettu osiin, on paljon helpompi yksikkötestata niitä osia. Siitä kun saa hyvän rutiinin päälle, on hyvä kirjoittaa testit niille muillekin metodeille. Ja siitä voi sitten jatkaa muihin luokkiin, vaikka util-paketin sisältöön. Älä kuitenkaan testaa gettereitä tai settereitä, jos ne ovat automaattisesti generoituja.
Suorituskyvyn testauksesta sen verran, että testaat jo pakkausastetta. tämä on hyvä ja muistathan tehdä harjoitustyöhösi testausdokumentin. Olet nyt testannut asian, mutta et välttämättä saanut siitä pisteitä testausraportin puuttumisen takia. Vaikka et mitään jacoco-raportin screenshotteja tässä vaiheessa sinne suoltaisikaan, voit silti ottaa netbeansin run-tuloksista kuvakaappauksen liittyen alicen pakkautumiseen ja purkautumiseen ja laittaa sen harjoitustyön testausdokumentin osaksi.
Itseäni on askarruttanut tässä työssä se, että suorituskyky tarkoittaa yleensä ohjelman nopeutta. Itse tekisin yksinkertaisen nopeusvertailun System.currentTimeMillsin avulla. Tästä päästäänkin seuraavaan asiaan:
Harjoitustyö kaipaa itsetehtyjä HashMapia, ArrayListiä yms, koska javan valmiita tietorakenteita ei valmiissa työssä pitäisi olla. Kannattaa huomata sekin, että javan omat rakenteet ovat usein hitaita(ArrayList, köh köh) ja niiden käyttäminen hidastaa työtäsi. Tiralabran sivuilla oli maininta, että ohjelman pitää toimia tehokkaasti. itsetehtäviä rakenteita olisi nähdäkseni ainakin HashMap, ArrayList, List ja Collections. Itse lähtisin liikkeelle ehkä ArrayListin korvaamisesta, koska sillä saisi korvattua käsittääkseni myös List:in(joka on kait sallittu, mutta erittäin hidas. Ja jos teet toimivan ArrayListin joka tapauksessa, niin Listin roikottamisessa mukana ei ole mitään erityistä hyötyä.
Onnea harjoitustyön viimeisille viikoille!