Vertaisarviointi

Repositorio kloonattu: 14.6.2020 klo 19.49

Yleistä

Projektiin oli mukava tutustua, sillä se on koodattu laadulla ja tekijä on selvästi valinnut itseään kiinnostavan aihepiirin.

Projekti on jo tosi pitkällä, ja kaikki toiminnot toimivat juuri niin kuin olettaa saattaakin. Eli mielestäni oikein hyvää työtä.

Yritin kuitenkin keksiä joitain huomioita koodista. Osa näistä on varmaan lähinnä makuasioita, ja varsinaisesti ainut havaitsemani ongelma on, että jotkut kontrollerit kuuntelevat myös sellaisia pyyntöjä, joita ne eivät selvästikään oikeasti odota.

auth/views.py

auth_logout-metodi on hieman ongelmallinen, koska uloskirjautuminen onnistuu GET-pyynnöllä, jolla ei pitäisi olla mitään tilaa muuttavia sivuvaikutuksia.

auth_register-metodi hyväksyy myös GET-pyynnöt, vaikka tarkoituksena on selvästi käsitellä vain POST-pyynnöllä lähetetyt lomakkeet. Eli nyt jos joku (käyttäjä tai vaikkapa hakurobotti) navigoi polkuun /auth/register, metodin suoritus käynnistyy, vaikkei mitään lomaketta ole lähetettykään.

campaigns/models.py

Kiinnitin huomiota metodien nimeämiseen. check_account ei mielestäni oikein kerro metodin tarkoituksesta (jokin käyttäjään liittyvä tarkistus, mutta mikä?). Kommentti toki kertoo, mistä on kyse, mutta voisiko parempi nimi metodille olla esim. is_registered_to_campaign, jolloin metodi dokumentoisi itse itsensä? Samoin check_admin voisi olla selvempi muodossa is_campaign_admin.

campaigns/views.py

campaigns_index-metodi olisi mielestäni helpommin luettavissa, jos templatelle annettavat tiedot haettaisiin ensin yksi kerrallaan erillisiin muuttujiin.

Myös campaign_register-metodissa kannattaa kiinnittää huomiota käsiteltäviin HTTP-pyyntöihin. Tämäkin metodi suoritetaan myös GET-pyynnöllä eli jos navigoin polkuun /campaigns/register/1 eikä kampanjalla ole salasanaa, niin minut liitetään (huomaamattani?) kampanjaan. Ja hakurobottikin voi tehdä GET-pyynnön tuohon polkuun. Tässä ei varmaan odoteta oikeasti muita kuin POST-pyyntöjä?

campaign_view: Sisäkkäiset if-lauseet vaikeuttavat hiukan koodin lukemista. Tämä voi toki olla makuasia, mutta sama toiminnallisuus taitaisi onnistua myös tähän tyyliin:

if Campaign.check_admin(campaign_id, current_user):
    return render_template(...)

if Campaign.check_account(campaign_id, current_user):
    return render_template(...)

return redirect(...)

Kontrollerien polkuja voisi halutessaan yhtenäistää. Esim. seuraavista poluista toisessa annetaan ensin varsinainen "action" (remove) ja sitten poistettavan entiteetin id, kun taas toisessa nämä menevät toisin päin:

/campaigns//admin/remove/
/campaigns//creatures//remove/

Frontend

Fronttipuolella koodi on varsin selkeää ja sovellus on helppokäyttöinen.

Ulkoasusta tuli mieleen, että sivun varsinaisen pääsisällön (navbarin ja footerin välillä) sisältävälle containerille voisi asettaa sopivan minimikorkeuden. Nyt sivuilla, joilla on vähän sisältöä korkeussuunnassa, footer nousee keskelle sivua, vaikka minusta se saisi olla aina sivun alareunassa. Eli esim. min-height: 100vh tai min-height: calc(100vh - ??px) voisi lähteä kokeilemaan.

Lisäksi osalla sivuista (esim. campaigns-etusivu, olio- ja NPC-listaukset) ei ole yhtään otsikkoelementtiä. Otsikot ja tarvittaessa väliotsikot voisivat tehdä sivuista selkeämmän näköiset, kun eri elementtien välinen hierarkia nousisi paremmin esiin.

Olet varmaan miettinytkin jatkokehitysideoita. Minulle tuli mieleen, että olisi kiva, jos oliot ja NPC:t esitettäisiin listan sijaan jonkinlaisina kortteina – siis samaan tyyliin kuin sivu, jossa on Creatures ja NPC-otsikot ja hienot kuvat.

Oma 404-sivu oli mukava lisä!

Muita huomioita

Jos rekisteröitymislomakkeeseen syöttää tosi pitkän käyttäjänimen, annetaan validaatiovirhe "Either the account name or the password is too short". WTFormsin automaattisesti generoima virheviesti ei näy, koska templatessa renderöidään register_form.error. Saat virheen halutessasi näkyviin iteroimalla templatessa listaa register_form.register_name.errors, kuten teet muissakin templateissa.

Olioita ja NPC:tä lisätessä joihinkin kenttiin pyydetään laittamaan --, jos arvoa ei tunneta (pituus, tyyppi, yms.). Ohjeistus on selkeä, mutta jäin miettimään, olisiko kuitenkin parempi merkitä kentät valinnaisiksi, jotta käyttäjä voisi halutessaan jättää ne tyhjiksi ja lomakkeen validointi menisi silti läpi.

tire95 / Vicipaedia-Arcanum