Verataisarviointi - Githubissues

Toimiva ja rakenteeltaan selkeä softa.

Toimivuus

Sovelluksen perusominaisuudet eli käyttäjän luonti, kirjautuminen, keskustelut ja profiilit toimivat hyvin. Sovellus on myös suojattu hyvin, eikä siihen ole löytynyt ilmeisiä haavoittuvuuksia. Sovellus on myös suhteellisen helppo käyttää ja navigoida. Skins ominaisuus ei taida vielä olla implementoitu. Lisäksi profiilien ja käyttäjien yhteys on hieman hämärä eikä sovelluksen tarkoitus ole tyäsin selvä ellei ole lukenut readmeta. Etusivulla voisi olla enemmän tietoa siitä, mitä sovellus tekee ja miten sitä käytetään.

Koodi

Koodi on pääosin selkeää ja helppolukuista. Koodi on myös jaettu selkeästi eri tiedostoihin ja funktioihin, mikä helpottaa ylläpitoa ja kehitystä. Koodi ei ole mitenkään erityisesti dokumentoitu, mutta se ei mielestäni ole tarpeellista, koska muuttujat ja funktiot on nimetty niin hyvin että koodi selittää itsensä. Alla muutamia huomioita koodista:

Readmessa voisi erillisten kirjastojen asentamisen siijaan ohjeistaa ajamaan vain pip install -r requirements.txt, näin varmistuisi että ladattavien kirjastojen versiot ovat oikeat.

Sessioita käsitellään hieman oudolla tavalla. Flask kirjaston sessio assiosioidaan käyttäjään csrf tokenin avulla jonka kirjasto luo ja lähettää automaattisesti headereiden välityksellä. Kuitenkin koodissasi olet luonut uuden csrf tokenin, jota lähetetään formien mukana. Kannattaa luottaa kirjaston implementaatioon ja poistaa oma csrf tokeni. Tällöin tämän

    if csrf_token != session["csrf_token"]:
        abort(403)

Voisi korvata tällä:

    if  username not in session:
        abort(403)

koska username on asetettu vain jos käyttäjä on kirjautunut sisään. Lisäksi formeista voi poistaa piilotetut inputit joiden kautta lähetät csrf tokenin ja usernamen. Ne voi ottaa suoraan session dictistä. Kannattaa lukea uudestaan kurssimateriaalien osio kirjutumisesta, ja tarkastella miten se on siellä toteutettu.

Lisäksi jos haluaa vielä enemmän siistiä koodia, voi lisätä before_request funktioon tarkistuksen, joka tarkistaa onko käyttäjä kirjautunut sisään.:

WHITELISTED_PATHS = ['/login', '/static']

@app.before_request
def check_user_logged_in():
    if "username" not in session and request.path not in WHITELISTED_PATHS:
        return redirect('/login')

Tämä varmistaa, että käyttäjä on kirjautunut ladatessa mitä tahansa sivua, paitsi muuttujan WHITELISTED_PATHS polkuja.

Monessa kohtaa koodia toistuu rakenne, missä on paljon sisäkkäisiä if lausekkeita joiden else haara on yksi rivi, jossa annetaan error ilmoitus. Tämä johtaa siihen, että koodi on todella sisennettyä ja täten vaikealukuista. Tätä voisi parantaa kääntämällä if lausekkeen negaatioksi ja poistamalla else haara kokonaan, koska koodi pysähtyy errorin kohdalla. Esim:

def signup(username, password):
    if _validate_username(username):
        if _validate_password(password):
            if not _username_taken(username):
                passhash = generate_password_hash(password)
                query = text("INSERT INTO Users (created, username, passhash, is_admin) " \
                            "VALUES (NOW(), :username, :passhash, TRUE)")
                db.session.execute(query, {"username":username, "passhash":passhash})
                db.session.commit()
            else:
                raise ValueError("Username taken")
        else:
            raise ValueError("Password must be between 8 and 50 characters")
    else:
        raise ValueError("Username must be between 3 and 20 characters")

Voisi muuttaa muotoon:

def signup(username, password):
    if not _validate_username(username):
        raise ValueError("Username must be between 3 and 20 characters")

    if not _validate_password(password):
        raise ValueError("Password must be between 8 and 50 characters")

    if _username_taken(username):
        raise ValueError("Username taken")

    passhash = generate_password_hash(password)
    query = text("INSERT INTO Users (created, username, passhash, is_admin) " \
                "VALUES (NOW(), :username, :passhash, TRUE)")
    db.session.execute(query, {"username":username, "passhash":passhash})
    db.session.commit()

Tämä tekee koodista helpommin luettavaa ja vähentää sisennystä.

Tietokanta

Tietokannan rakenne selkeä ja toimiva, eikä siinä vaikuta olevan paljon huomautettettavaa. Muutama huomio, jotka tekevät tietokannasta ehkä hieman käytettävämmän:

created timestampit voisi olla DEFAULT NOW() jolloin niitä ei tarvitsisi muistaa asettaa erikseen
Taulujen nimet voisi kirjoittaa kokonaan pienellä, kuten on tapana
messages taulussa on kentät creator ja creator_id kentät. creator pitää ilmeisesti sisällään käyttäjänimen, joka on jo tallennettu users tauluun. Tämä on hieman turhaa, koska creator_id kenttä riittää. Luojan käyttäjänimen hakeminen onnistuu JOIN lausekkeella. Kaikkien viestien hakeminen on tietysti tehokkaampaa ja helpompaa nykyisellä rakenteella, mutta käyttäjänimen vaihtuessa joutuisi päivittämään kaikki viestit.

Tyyliseikkoja

Tässä vielä muutama tyyliseikka, jotka ovat ehkä enemmän makuasioita kuin mitään kunnon palautetta.

Useassaa tiedostossa toistuu kaava:

if  condition:
     return True
return False

Tämän voisi kirjoittaa vain ```return condition````

Commit messaget ovat selkeitä ja informatiivisia, mutta hyvien tapojen mukaan niiden pitäsi olla imperatiivisessa muodossa. Esim. "Added login page" -> "Add login page". Commitit ovat myös suurikokoisia, joten niitä voisi jakaa pienempiin osiin.

ellapaella / skinhaven

Verataisarviointi #1

Toimivuus

Koodi

Tietokanta

Tyyliseikkoja