Als we toch bezig zijn, my 2 cents. (code review)
Probeer je code te organiseren op basis van een logische volgorde. Onderstaande functie is eigenlijk je hoofdfunctie. Een mens lees van boven naar beneden en heeft geen zin op rond te zoeken. Probeer ook op die manier je code te schrijven, zodat je van boven naar beneden in een oogopslag ziet wat je code doet.
code:
1
2
3
4
5
| function continueRegistration() {
if (formValidation()) {
window.location.href = "registreren2.html"
}
} |
Ik zie hier heel veel herhaling. Alles wat je herhaalt is generiek en zou je een keer maar hoeven schrijven.
code:
1
2
3
4
5
| function formValidation() {
let voorNaamInput = document.getElementById('voorNaamInput').value;
let achternaamInput = document.getElementById('achterNaamInput').value;
let emailadresInput = document.getElementById('emailadresInput').value;
} |
Daarnaast lijkt me dat je herhaal input eerst komt op de pagina en daarna pas je wachtwoordHerhalenInput echter verwerk je ze in omgekeerde volgorde.
Ook zou je je kunnen afvragen in hoever je een gebruiker zijn wachtwoord wilt laten herhalen. Zeker nu browsers vaak dingen automatisch invullen of een wachtwoordsuggestie geven.
Zeker met front-end is UX en waarom je dingen doet en de impact ervan erg belangrijk.
code:
1
2
| let wachtwoordHerhalenInput = document.getElementById('wachtwoordHerhalenInput').value;
let wachtwoordInput = document.getElementById('wachtwoordInput').value; |
Een wachtwoord limiteren op een maximaal aantal karakter is raar is zou ik alleen doen wanneer er harde limiet wordt opgesteld vanuit bijvoorbeeld een database.
Wachtwoorden worden veiliger naarmate ze langer zijn.
code:
1
2
3
| } else if (!(password1.length >= 8 && password1.length <= 15)) {
window.document.getElementById("errorMessage").innerText = window.document.getElementById("errorMessage").innerText + "\n Wachtwoord moet tussen de 8 tot 15 tekens lang zijn";
} |
Als je toch al Regex gebruikt, waarom niet alles checken met Regex. Voor het gebruik van een Regex zijn denk ik sowieso de meningen over verdeeld. Maar als je het gebruikt zou ik dan ook alles ermee checken.
code:
1
2
3
4
5
| } else if (!(password1.length >= 8 && password1.length <= 15)) {
window.document.getElementById("errorMessage").innerText = window.document.getElementById("errorMessage").innerText + "\n Wachtwoord moet tussen de 8 tot 15 tekens lang zijn";
} else if (!(passwordRules.test(password1))) {
window.document.getElementById("errorMessage").innerText = window.document.getElementById("errorMessage").innerText + "\n Wachtwoord moet een speciaal teken bevatten";
} |
Daarnaast zou je de regex dan als zodanig kunnen structureren dat het ook leesbaar is.
code:
1
2
3
4
5
6
7
| // Onderstaand is niet getest en is puur een voorbeeld
let passwordRegex = '^' + // Start of word
'(?=.*[A-Za-z])' + // At least a letter upper or lowercase
'(?=.*\\d)' + // At least a digit
'(?=.*[@$!%*#?&])' + // At least a special character
'[A-Za-z\\d@$!%*#?&]{8,}' + // Should be a minimal of 8 characters
'$' // End of word |
Een beetje gerefactored en dan kom ik hier op uit.
Opmerkingen:
- De if else constructie in continueRegistration kan misschien mooier
- Gebruik van if else statements is niet onderhoudbaar. Als je het generiek maakt mbv arrays / objecten kan je het met minimale effort de code uitbreiden met extra velden en validaties. Alleen moet je altijd een afweging maken tussen leesbaarheid en complexiteit (overengineering)
- De functies getInputValue en appendMessageToError, getInputValue en isInvalidString staan helemaal onderaan en niet op volgorde van gebruikt. Deze functies zijn zo klein en zouden zo eenduidige moeten zijn dat iemand niet zich afvraagt wat ze doen.
- teksten en constanten bovenaan eenmalig definiëren zodat je niet overal in de code hoeft te zoeken naar teksten
code:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
| function continueRegistration() {
let errorMessage = getFormError();
if (errorMessage != null) {
window.document.getElementById('errorMessage').innerText = errorMessage;
} else {
window.location.href = 'registreren2.html';
}
}
function getFormError() {
let error = '';
if (isInvalidString(getInputValue('voorNaamInput'))) {
error = appendMessageToError(error,"Vul uw voornaam in");
}
if (isInvalidString(getInputValue('achterNaamInput'))) {
error = appendMessageToError(error,"Vul uw achternaam in");
}
if (isInvalidString(getInputValue('emailadresInput'))) {
error = appendMessageToError(error,"Vul uw email in");
}
return appendInvalidPasswordError(error, getInputValue('wachtwoordInput'), getInputValue('wachtwoordHerhalenInput'));
}
function appendInvalidPasswordError(error, password, passwordRepeated) {
let passwordRegex = new RegExp('^' + // Start of word
'(?=.*[A-Za-z])' + // At least a letter upper or lowercase
'(?=.*\\d)' + // At least a digit
'(?=.*[@$!%*#?&])' + // At least a special character
'[A-Za-z\\d@$!%*#?&]{8,}' + // Should be a minimal of 8 characters
'$'); // End of word
if (isInvalidString(password) || isInvalidString(passwordRepeated)) {
return appendMessageToError(error, isInvalidString(password) ? 'Vun een wachtwoord in' : 'Herhaal je wachtwoord');
} else if (password !== passwordRepeated) {
return appendMessageToError(error, 'Wachtwoorden komen niet overeen');
} else if (!passwordRegex.test(password)) {
return appendMessageToError(error, 'Wachtwoord moet voldoen aan de volgende eisen: '
+ '\n - minimaal 1 letter'
+ '\n - minimaal 1 cijfer'
+ '\n - minimaal 1 speciale karakter'
+ '\n - minimaal 8 tekens');
}
}
function appendMessageToError(error, message) {
return error + '\n' + message;
}
function getInputValue(elementId) {
return document.getElementById(elementId) ? document.getElementById(elementId).value : null;
}
function isInvalidString(value) {
return typeof value !== 'string' && value.length === 0;
} |
Anyway, zie het niet als kritiek. Code reviews horen bij code schrijven en het is vrij standaard.
Het komt uiteindelijk vaak uit op smaak waarbij die smaak vaak bepaald wordt op basis van dingen die veel voorkomend zijn, standaarden binnen een bepaalde taal of techniek, ervaringen en meer van dat soort dingen. Ik ben dan ook vooral hoe mijn refactor beter had gekund
, maar niet om het topic te kapen zou dat misschien beter in een ander topic kunnen.
Ik denk alleen dat het goed is voor mensen om te zien dat het heel erg uitmaakt hoe je je code schrijft en dat naast dat het moet werken er ook een gedachte achter zit qua onderhoud. Want uiteindelijk is dat hetgeen wat je het meest doet als programmeur.
[
Voor 4% gewijzigd door
JustAnotherDev op 03-12-2020 00:44
]
:fill(white):strip_exif()/f/image/V6Ibs9ZGImlgmTPUnqYl8afY.png?f=user_large)
:strip_icc():strip_exif()/u/5326/crop5ff8e3fc691c6_cropped.jpeg?f=community)
:strip_icc():strip_exif()/u/53213/crop57ace1fa37f44.jpeg?f=community)
:strip_exif()/u/52703/crop6147f7b3384e7.gif?f=community)