name: Week 7 - Code review
about: Code review on a classmate
title: Code review
labels: 'week-7, week-7-code-review'

Code review

Name

Nina Pajonk

Class

Tech-1

Feedback

Ik vind de repository erg netjes en overzichtelijk ingedeeld!
Er staat ook al veel genoteerd in de wiki.

Feedback specifiek op criterium 1 Project Tech

Ja, heet team maakt gebruik van Git en Github om versie beheer van de code te doen. Ik zie verschillende branches, maar alleen nog maar van 2 personen i.p.v. 3.

Feedback specifiek op criterium 2 Project Tech

Er zijn gezamenlijke afspraken gemaakt over de code standaarden, want het ziet er consequent uit. Overal dubbele: “ bijvoorbeeld. Dit is ook netjes beschreven in de wiki.

Feedback specifiek op criterium 3 Project Tech

De readme.md van de team repo, is nog leeg. Op de individuele repo zag ik wel een duidelijke readme beschreven. Dus dit komt vast goed.

Feedback specifiek op criterium 5 Project Tech

Dit ziet er al aardig goed/ werkend uit.

Feedback over andere dingen, zoals voor backend of frontend

• Nette comments.
• Alle bestanden zijn netjes en er staat geen onnodige code in comments.

name: Week 7 - Code review
about: Code review on a classmate
title: Code review
labels: 'week-7, week-7-code-review'

Code review

Name

Ralf

Class

tech-1

Feedback

Installatie ging heel soepel.
Na de .env file content gevraagd te hebben, werkt het in de localhost:3000 goed. Alles is klikbaar en er zit in een logische structuur in het gebruik van de app. Je kan nu alleen nog kiezen uit een aantal profielen en daarmee kan je door naar de app (liken/matchen). Ik neem aan dat dat kiezen van een profiel wordt vervangen door inloggen en registreren van een account. Maar dat is natuurlijk nu nog niet aan de orde. Al met al, een fijne en clear interface van de app👌.

Wat betreft de code:
Server (index.js).
Bij elk stuk code staat in comments waar dat stuk code over gaat. Ook is er een 'line' aangebracht in de comments waardoor het overzichtelijk wordt. Wat ik zelf niet gewend ben, maar wat Karin wel doet, is dat de 'app.' van alle onderwerpen (static folder, session en handle routes) niet bij elkaar staan. Ik vind het persoonlijk overzichtelijker als alle 'app.' onder elkaar staan. Maar omdat Karin ze heeft 'gelabeld' met waar elk stuk code over gaat, snap ik dat wel.
Bovendien vind ik het nice dat onder de handle routes alle functions genest zijn uitgewerkt. Dit zorgt voor nog steeds duidelijke overzichtelijkheid van de code. Ook hier is in comments gezet wat bepaalde stukken code doen.

package.json.
Clean file met alleen de broodnodige dependencies erin. Ik mis hier wel een linter...ESlint zou ik aanraden, omdat die goed configureerbaar is.

.ejs files.
Goed gebruik gemaakt van loops en de .ejs 'includes' (<% , <%- , <%=). .ejs files zijn mooi genest (Beautify denk ik?) en dus overzichtelijk.

script files.
De client-side JavaScript bestanden zijn netjes opgebouwd. Eerst declaration variables, dan functions en als laatst eventlisteners. Hier heb ik geen tips voor, alleen maar tops. Code is bovendien netjes geschreven. Echter weet ik niet of je meerder .js files mag hebben. Voor hetzelfde geldt voor meerdere .css bestanden.

Kortom, nette feature die overzichtelijke code bevat en hier en daar wat betreft linting nog iets dieper op in mag gaan.

Feedback specifiek op criterium 1 Project Tech

Git en Github wordt zeker goed gebruikt voor versiebeheer. Ook zijn er meerdere branches aangemaakt, wat het werken in Git gemakkelijker maakt.

Feedback specifiek op criterium 2 Project Tech

Code standaarden zoals linting moet nog even research naar gedaan worden.

Feedback specifiek op criterium 3 Project Tech

README van Karin is erg efficient. Er staat kort uitgelegd wat de feature inhoudt. Een korte weergave van de UI en dan de installatie. Ook staat de structuur van de database er in en een hele uitgebreide bronnenlijst. Super!

Feedback specifiek op criterium 5 Project Tech

Er moet nog wel het registreren en inloggen verwerkt worden. Maar voor zover het er nu uitziet, gaat dat zeker goed komen.

Goed bezig!

Issue location

https://github.com/KarinMeijvogel/datingapp/blob/c31f76869747c91c73864b79705fd36415b8e18d/index.js#L114-L155

Issue topics

• Code readability

Issue description

Currently, this function logic is entirely wrapped in an if statement. This causes more indentation, which decreases code readability.

How to solve

Turn the check around and return early.
Instead of checking if the userid exists, check if the userid does not exist. if it doesn't exist, redirect to login. After the redirect call, put return. This makes sure the rest of the function logic won't be executed.

Before

function likedUsers(req, res, next) {
  if (userid !== null) {
    // ... entire function logic
  } else {
    res.redirect("/login");
  }
}

After

function likedUsers(req, res, next) {
  // or, if (!userid) {   (falsey values)
  if (userid === null) { 
    res.redirect("/login");
    return;
  }
  // ... entire function logic
}

Benefits

Not only did you lose a level of indentation which improves code readability already, people can now immediately see what happens when there is no userid.

General rule:

• if the if part is really big and the else part really small, turn the check around
• If the if statement is the top level of the function and there is no code following the if else statement, put a small if statement that returns the function early.

Disclaimer

Please think about every opened issue and decide wether or not you want to do anything with it. It's not important to do it my way, it's important to be able to explain what you did or didn't do with it and why.

Issue location

https://github.com/KarinMeijvogel/datingapp/blob/9dc7daa8ae5093d4f052ff9b013a2d7cb355a8ed/index.js#L23

https://github.com/KarinMeijvogel/datingapp/blob/9dc7daa8ae5093d4f052ff9b013a2d7cb355a8ed/index.js#L38-L44

Issue topics

• Error prevention

Issue description

You are initializing session but not doing anything with it. Also, you are saving the user globally, which is considered a bad pattern (If the server restarts, the user is gone).

How to solve

Remove global userid variable
In login function, rewrite userid = req.body.user; to req.session.user = req.body.user.
Then write all references to userid to req.session.user

Benefits

• No loss of data on server reset
• No global variable that can be overwritten by accident

Disclaimer

Please think about every opened issue and decide wether or not you want to do anything with it. It's not important to do it my way, it's important to be able to explain what you did or didn't do with it and why.

Noticed that it's impossible to delete someone you've liked, once you matched. As long as the person is 'pending' you can delete them and this gets registered in the database, but as soon as you're matched, your delete doesn't get registered in the database anymore.