jova486
commented
2 years ago Kiitos! Todella hyvää ja hyödyllistä palautetta. Jotkut kohdat ovat juuri työn alla ja tässä on hyviä neuvoja miten edetä mutta myös hyviä neuvoja asioista joita en ollut huomannu. Minulla on ilmeisesti sinun shakugenerator arvioitavana. Laitoin viestiä tg:n kautta. Toivottavasti oikeaan osoitteeseen. Olisi kysyttävää siihen liittyen.
Projekti vaikuttaa mielestäni vallan mielenkiintoiselta ja olevan melko hyvässä vaiheessa. Kirjoitan alle myöhemmin itse koodin rakenteesta, mutta ensimmäiseksi mainitsen että suosittelen midi2audio -moduulia, jolla voi konvertoida midi-tiedoston äänitiedostoksi. Syntaksi on hyvin yksinkertainen, esm. näin:
En ollut ihan varma mihin toimintoon viitataan viikkorraportin kohdassa: "en saanut musiikin rakennetta tuottavaa toimintoa aikaiseksi." tai niissä kohdissa, jossa vaikutit itsekriittiseltä, että ei olisi edennyt hyvin. Projektin toiminnallisuus vaikuttaa mielestäni hyvältä.
Dokumentoiva koodi -asiasta: Service-luokassa esimerkiksi olet mielestäni käyttänyt suuressa osassa variaabelien nimeämistä, jossa niistä on helppo lukea niiden tarkoitus. Esimerkiksi markov_chain.py -tiedostossa nimeäminen ei ollut yhtä selkeää. Käytännössä kannattaa laittaa hieman pidempiä variaabelinimiä, jotka ovat deskriptiivisiä niiden sisältämän tiedon suhteen.
Lisäksi "type annotations" on hyvä kikka koodatessa lisätä sekä luettavuutta, että IDE-ympäristötkin osaavat neuvoa käyttäjää usein paremmin niitä käyttäessä, koska tietävät esm mitä metodeja parametreina annetut oliot sisältävät. Joskus nimeäminen johti myös vähän harhaan esm. Service-luokan metodi "open_file" ei avaakaan mitään vaan vain tallentaa tiedostonimet, jotka tullaan avaamaan vasta myöhemmin. Lisää spesifiyttä vaan ja elääntymistä lukijan rooliin, joka ei tiedä funktion merkitystä ennalta. esm "set_midi_filename"
Luokkametodeja ja attribuutteja kannattaa myös muuttaa piilotetuiksi jos niitä ei tarvita luokan ulkopuolella vaan ovat luokan sisäisiä apumetodeja. Tämä on keino kertoa itselle ja muiille ohjelmoijille, että näitä metodeja ei ole tarkoitus kutsua luokan ulkopuolelta. Lisää yllättävän hyvin koodin luettavuutta. Koodin enkapsuloinnista ja funktioiden ja luokkien tehtävistä: Koodissa mm. Service-luokka sisältää paljon erilaisia tehtäviä ja toimintoja, jotka eivät muodosta mielestäni selkeää kokonaisuutta. Koittaisin jakaa sen velvollisuuksia loogisiin kokonaisuuksiin erillisiksi luokiksi, esimerkiksi tiedostojenkäsittely-luokka, musiikillisten detaljien käsittelyluokka, midi-datan luonnin ja käsittelyn luokka yms. Myös ohjelman voisi varmaan aloittaa kuten tyypillistä erillisestä index.py -tiedostosta, jossa voi jotakin toimintaympäristön asettamista tehdä mm.
Löysin myös joitakin tilanteita, jossa ei ollut tarvetta tallentaa variaabelia luokka-attribuutiksi, vaan olisi riittänyt lokaali variaabeli, joka annetaan apufunktiolle parametrina. Tästä esimerkkinä Trie -luokan self.output, joka voisi olla vain output ja antaa apufunktiolle dfs se parametrina. Ylipäätään hyvänä käytäntönä pidetään sitä että metodien/funktioiden/luokkakonstruktorien kutsuissa annetaan eri koodin osien käsittelemiä tekijöitä eteenpäin parametreina, jolloin niiden definitiot ovat toisistaan riippumattomia. (riippuvuuksien injektointi)
Trie-luokan insert_arrayn selkeys ja varmaankin tehokkuuskin lisäntyisi, jos sille annetun jonon lukuja ei käytäisikään läpi kahdella sisäkkäisellä luupilla, vaan otettaisiin aina markovin ketjun syyvyyden verran lukuja trieen. Kunkin noden counteria voidaan lisätä joka kerta kun insert funktio käy noden läpi. Näin pikamiettimällä ymmärtääkseni saataisiin aikaan sama lopputulos. Muutenkin Trie -luokan toiminnan ymmärtäminen oli hieman hankalaa query + dfs funktioiden toiminta ja tarkoitus ym. Tässä olisi mahdollista ehkä yksinkertaistaa niiden toimintaa sen avulla että pohtisi erilaisia mahdollisuuksia muuttaa itse trie-rakenteen muotoa niin, että operaatiovaiheet vähenisivät.
Koodissa on mielestäni paljon hyvääkin, mutta helpottaa varmasti omaakin työtä projektin kanssa jos teet perusteellista refaktorointia niin että operaatioissa ei ole ylimääräisiä vaiheita ja variaabeleita silloin kun se ei ole tarpeellista ja toisaalta yllä kirjoittamani tapaan luokkien kapselointi olisi selkeä.
Ja ennen kaikkea - kokonaisuutenahan tämä on kivasti toimiva ohjelma! Huomaat varmasti itsekin raporttien itsekriittisistä kommenteistasi huolimatta, että refaktoroidessa paljastuu että pohjatyössä on paljon hienoa.
Koska aiheemme ovat niin samanlaiset (itse teen projektissani myös musiikin generaatiota markovin ketjuja ja trie-rakennetta käyttäen), olisi kiva jos sinua kiinnostaisi jutella vaikka etämiitissä tms. aiheesta, jos toistemme oppimasta voisimme saada molemmat lisäpuhtia!