Skip to content
This repository has been archived by the owner on Aug 21, 2023. It is now read-only.

Vertaisarviointi #24

Closed
henrinikku opened this issue Oct 15, 2022 · 1 comment
Closed

Vertaisarviointi #24

henrinikku opened this issue Oct 15, 2022 · 1 comment
Labels
peer review Peer review

Comments

@henrinikku
Copy link

Viimeisin commit 39fdadf

Moikka!

Projektin aihe on mielenkiintoinen ja tärkeä. Olen käyttänyt RSA-algoritmia itsekin, mutta sen toiminnan yksityiskohdat eivät olleet minulle entuudestaan tuttuja.

Dokumentaatio

Kaikki dokumentit on tehty todella huolellisesti ja niissä ohjelman toiminta on selitetty auki kattavasti.

Projektin rakenne ja koodin laatu

Projektin rakenne on järkevä ja johdonmukainen. Toiminnallisuus on paketoitu luokkien taakse, mikä helpottaa koodin lukemista. Metodit (myös yksityiset) ja luokat on kommentoitu kattavasti. Koodin tyyli ja nimeämiskäytännöt ovat johdonmukaisia.

Parannusehdotuksia:

  • Joillain luokilla (esim. ConsoeIO) ei ole lainkaan tilaa, joten mielestäni voisi olla perusteltua käyttää niiden sijaan itsenäisiä funktioita samassa moduulissa. Vähintäänkin metodeista olisi mielestäni hyvä tehdä näiden luokkien kohdalla staattisia.
  • Jonkin CLI-kirjaston käyttö (esim. Typer) voisi olla nykyistä helpompi vaihtoehto käyttöliittymän toteuttamiseen. En tosin ole varma, tukeeko Typer tai monet muut kirjastot mallia, jossa käyttäjä syöttää komentoja ajon aikana.
  • Jotkin riippuvuudet (esim. colored) on merkitty dev-riippuvuuksiksi, vaikka ohjelma ei toimi ilman niitä.

Toiminnallisuus

Ohjelman tekstikäyttöliittymä on toimiva ja helppokäyttöinen. Ainoa itseäni vaivaava asia on se, että konsoli tyhjentyy aina ennen komennon suoritusta. Ohjelmaa olisi ehkä hieman helpompi käyttää, jos näkisi selaamatta aiemmin suorittamansa komennot.

Itse algoritmi toimii oman lyhyehkön testailuni perusteella oikein ja todella nopeasti. Ei tosin ainakaan vielä tue emojeita :(

Parannusehdotuksia:

  • Konsolin tyhjennys pois tai asetuksen taakse
  • Olisi kiva, jos ohjelma voisi lukea/kirjoittaa olemassaolevat avaimet jostain konfiguraatiotiedostosta, jolloin niitä ei tarvitsisi joka kerta generoida uudelleen.
  • Ohjelman ohjelmallista käyttöä helpottaisi, jos käyttöliittymä toimisi siten, että käyttäjä antaa komennon ja saa siihen vastauksen, minkä jälkeen ohjelma sulkeutuu. Tämä tosin saattaisi vaatia aika paljon muutoksia.

Testit

Testit ovat selkeitä ja niistä käy hyvin ilmi kunkin testin tarkoitus. Kaipaisin ehkä hieman lisää yksikkötestejä Crypt ja KeyGenerator-luokille, mutta nykyisistäkin on varmasti apua ohjelman toimivuuden toteamisessa.

Parannusehdotuksia:

  • pytest-benchmark voisi olla hyvä apukirjasto suorituskykytestaukseen, etenkin kun näyttää siltä, että algoritmia on tarve ajaa monta kierrosta hyvän suorituskykyarvion saamiseksi.

Mahdollisia bugeja

  • Jos ajan esim echo 1 | poetry run invoke start ja yritän sulkea ohjelman painamalla q, suoritus näyttää jäätyvän.
  • Ohjelma kaatuu, jos yrittää tulostaa avaimen ennen sen luomista.
@rikurauhala
Copy link
Owner

Hei!

Kiitos rakentavasta palautteesta. Erotin ConsoleIO:n omaksi luokakseen jo ihan alussa Ohjelmistotekniikka-kurssin mallin mukaisesti. Ideana oli nimenomaan riippuvuuksien injektointi ja testaamisen helpottaminen myöhemmin. Ymmärrän kyllä pointtisi! CLI-kirjaston käyttäminen on myös hyvä idea, mutta käyttöliittymäkoodin kirjoittaminen uusiksi tässä kohtaa kurssia ei välttämättä ole paras idea. Kiitos kuitenkin vinkistä! Hyvä huomio myöskin riippuvuuksista, täytyykin korjata asia.

Konsolin tyhjentyminen on selvästi mielipideasia. Aiemmassa vertaisarvioinnissa esitettiinkin, että konsolin tulisi tyhjentyä. Muutin toiminnallisuutta siihen suuntaan, mutta taidan tässä asiassa kääntyä kannallesi. Tämän sovelluksen kohdalla olisi varmasti hyödyllistä, että aiemmat komennot pysyisivät näkyvissä.

Emojeita ei kyllä valitettavasti voi käyttää sovelluksessa! Sovelluksen tukemien merkkien määrä on varsin rajoittunut, sillä halusin kurssin hengessä toteuttaa oman tietorakenteeni. Pythonista toki löytyy sisäänrakennettuna ord() ja str() -komennot, joilla tuen saisi helposti koko Unicode-merkistölle.

Avainten lukeminen tiedostosta on myös varsin hyvä idea ja sen varmasti toteuttaisin, jos jatkaisin sovelluksen kehitystä. Kuitenkin kurssin pääpainon ollessa itse algoritmeissa, koin riittäväksi sen, että avaimet tallennetaan muistiin.

Kiitos hyvästä vertaisarvioinnista!

@rikurauhala rikurauhala pinned this issue Oct 16, 2022
@rikurauhala rikurauhala added the peer review Peer review label Oct 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
peer review Peer review
Projects
None yet
Development

No branches or pull requests

2 participants