De PHPhulp kroeg
Mening schrijfstijl
Ik heb al eerder wat vragen geplaatst hier, maar nu is mijn vraag;
Wat vinden jullie van mijn schrijfstijl? Ik hoor graag wat ik kan verbeteren.
Ook wil ik dingen graag weten wat ik beter kan doen met beveiliging of fout afhandeling.
//waarschijnlijk komen en reactie van 'het werkt niet', maar dat geeft niet.
Code (php)
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
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
<?php
if($_SERVER['REQUEST_METHOD'] == 'POST') //Kijken of het formulier goed is verzonden.
{
require_once 'connect.php'; //Connectie db.
if(empty($_POST['wachtwoord'])) //kijken of het form. wel goed is ingevuld.
{
echo "De gegevens zijn niet correct ingevuld.";
}
else
{
if ($_POST['wachtwoord'] == $_POST['cwachtwoord']) //Controle of gegevens goed zijn ingevuld.
{
mysql_select_db("shop", $con); //$con bevind zich op de connect.php pagina.
mysql_query("
INSERT INTO klanten (unaam, upass, email)
VALUES ('" . $_POST['gebruikersnaam'] . "', '". $_POST['wachtwoord']. "', '". $_POST['email']. "')"); //De invoering van de naam en pass in db.
}
else
{
echo "De ingevoerde wachtwoorden komen niet overeen!";
}
}
}
else
{
echo "U moet wel gegevens invullen!";
}
mysql_close($con);
?>
if($_SERVER['REQUEST_METHOD'] == 'POST') //Kijken of het formulier goed is verzonden.
{
require_once 'connect.php'; //Connectie db.
if(empty($_POST['wachtwoord'])) //kijken of het form. wel goed is ingevuld.
{
echo "De gegevens zijn niet correct ingevuld.";
}
else
{
if ($_POST['wachtwoord'] == $_POST['cwachtwoord']) //Controle of gegevens goed zijn ingevuld.
{
mysql_select_db("shop", $con); //$con bevind zich op de connect.php pagina.
mysql_query("
INSERT INTO klanten (unaam, upass, email)
VALUES ('" . $_POST['gebruikersnaam'] . "', '". $_POST['wachtwoord']. "', '". $_POST['email']. "')"); //De invoering van de naam en pass in db.
}
else
{
echo "De ingevoerde wachtwoorden komen niet overeen!";
}
}
}
else
{
echo "U moet wel gegevens invullen!";
}
mysql_close($con);
?>
edit: Foutje.
Gewijzigd op 12/01/2011 08:08:03 door Ocirina Ocirina
- Je script is niet beveiligd, maak gebruik van mysql_real_escape_string()
- Gebruik je 1 database? Dan zou ik mysql_select_db("shop", $con); eruit halen en in connect.php erbij zetten.
- Mysql_close is nergens voor nodig?
mijn voorkeur geeft ook een iets andere notatie, maar dat is voor iedereen verschillend:
ipv
Yea Rupie op 12/01/2011 08:38:57:
Nee juist niet.
De tweede manier is veel duidelijker.
Sowieso nooit wachtwoorden zomaar opslaan, altijd een afgeleide (md5 of sha1 hash bijv.). Geen uitzonderingen.
Altijd oppassen met data in je sql stoppen -> mysql_real_escape_string voor strings waarvan je niet weet wat erin zit. (Bij sha1 hoeft het bijv. niet bij omdat daar altijd iets a-zA-Z0-9 uit komt, nooit iets wat lijkt op SQL of wat je query zou kunnen breken.)
require_once 'connect.php' impliceert dat je maar op één plek je databaseverbinding kan gebruiken.
En gebruik meer functies. Functies zijn er niet voor hergebruik, maar om je code in hapbare brokken op te splitsen.
Code (php)
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
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
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
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
<?php
if($_SERVER['REQUEST_METHOD'] == 'POST' && do_register_user())
{
header('Location: win.php');
exit;
}
else
{
// toon formulier
}
function do_register_user()
{
// ik check ze allemaal maar even zodat je niet tegen undefined indexes aanloopt
if (empty($_POST['gebruikersnaam']) || empty($_POST['wachtwoord'])
|| empty($_POST['cwachtwoord']) || empty($_POST['email']))
{
echo 'De gegevens zijn niet correct ingevuld.';
return false; // return stopt deze functie, en false geeft aan dat er
// iets mis was, en we dus het formulier weer terugwillen.
}
if ($_POST['wachtwoord'] != $_POST['cwachtwoord'])
{
echo 'De ingevoerde wachtwoorden komen niet overeen.';
return false;
}
if (!register_user($_POST['gebruikersnaam'], $_POST['wachtwoord'], $_POST['email']))
{
echo 'Sorry, computer says no...';
return false;
}
return true;
}
// Splits je code in aparte delen:
// do_register_user is om een formulier te verwerken
// register_user is om een gebruiker te registreren in je database
// get_db_connetion is om een database verbinding te pakken te krijgen.
function register_user($gebruikersnaam, $wachtwoord, $email)
{
$con = get_db_connection();
// sprintf om query en data wat gescheiden te houden, kan je sneller
// je query controleren op fouten.
$stmt = sprintf("
INSERT INTO
klanten (unaam, upass, email)
VALUES ('%s', '%s', '%s')",
mysql_real_escape_string($gebruikersnaam), // en al je data natuurlijk escapen.
sha1($something_salty . $wachtwoord), // geen rauwe wachtwoorden opslaan. Nooit. Niet eens de suggestie wekken!
mysql_real_escape_string($email));
// je query gaat ook mis wanneer je bijv. een unique constraint op
// gebruikersnaam of email hebt zitten, dus controleren of de query lukt
// lijkt mij vrij belangrijk.
return mysql_query($stmt, $con);
}
// met deze function kan je tenminste bij je verbinding vanuit verschillende
// contexten (functies) terwijl je nog steeds maar één keer verbinding maakt.
function get_db_connection()
{
static $connection;
if (!$connection)
{
require_once 'connect.php';
mysql_select_db('shop', $con); // hoort dit niet in connect.php?
$connection = $con;
}
return $connection;
}
?>
if($_SERVER['REQUEST_METHOD'] == 'POST' && do_register_user())
{
header('Location: win.php');
exit;
}
else
{
// toon formulier
}
function do_register_user()
{
// ik check ze allemaal maar even zodat je niet tegen undefined indexes aanloopt
if (empty($_POST['gebruikersnaam']) || empty($_POST['wachtwoord'])
|| empty($_POST['cwachtwoord']) || empty($_POST['email']))
{
echo 'De gegevens zijn niet correct ingevuld.';
return false; // return stopt deze functie, en false geeft aan dat er
// iets mis was, en we dus het formulier weer terugwillen.
}
if ($_POST['wachtwoord'] != $_POST['cwachtwoord'])
{
echo 'De ingevoerde wachtwoorden komen niet overeen.';
return false;
}
if (!register_user($_POST['gebruikersnaam'], $_POST['wachtwoord'], $_POST['email']))
{
echo 'Sorry, computer says no...';
return false;
}
return true;
}
// Splits je code in aparte delen:
// do_register_user is om een formulier te verwerken
// register_user is om een gebruiker te registreren in je database
// get_db_connetion is om een database verbinding te pakken te krijgen.
function register_user($gebruikersnaam, $wachtwoord, $email)
{
$con = get_db_connection();
// sprintf om query en data wat gescheiden te houden, kan je sneller
// je query controleren op fouten.
$stmt = sprintf("
INSERT INTO
klanten (unaam, upass, email)
VALUES ('%s', '%s', '%s')",
mysql_real_escape_string($gebruikersnaam), // en al je data natuurlijk escapen.
sha1($something_salty . $wachtwoord), // geen rauwe wachtwoorden opslaan. Nooit. Niet eens de suggestie wekken!
mysql_real_escape_string($email));
// je query gaat ook mis wanneer je bijv. een unique constraint op
// gebruikersnaam of email hebt zitten, dus controleren of de query lukt
// lijkt mij vrij belangrijk.
return mysql_query($stmt, $con);
}
// met deze function kan je tenminste bij je verbinding vanuit verschillende
// contexten (functies) terwijl je nog steeds maar één keer verbinding maakt.
function get_db_connection()
{
static $connection;
if (!$connection)
{
require_once 'connect.php';
mysql_select_db('shop', $con); // hoort dit niet in connect.php?
$connection = $con;
}
return $connection;
}
?>
Gewijzigd op 12/01/2011 09:08:42 door Jelmer -
- SanThe - op 12/01/2011 08:51:35:
...
Nee juist niet.
De tweede manier is veel duidelijker.
Nee juist niet.
De tweede manier is veel duidelijker.
Hier volg ik toch Yea Rupie.
Als je consequent omgaat met die indenteringsregels is er geen enkel probleem van leesbaarheid.
De sluitende accolade komt op de zelfde plaats (uiteraard lager) dan de plaats waar het commando begint, waar je de accolade opent.
bv. de i van if() komt dan op de zelfde plaats als de sluitende accolade.
Een bijkomend voordeel is dat je meer code krijgt op minder lijnen.
Kris Peeters op 12/01/2011 09:25:16:
Een bijkomend voordeel is dat je meer code krijgt op minder lijnen.
Dan moet je mijn versie pakken :)
Uiteindelijk is het denk ik gewoon een kwestie van smaak / persoonlijke voorkeur. Wel grappig dat er zoveel varianten zijn. Uiteraard is die van mij het beste :P maar leuk om te zien dat iedereen z'n eigen maniertje heeft. Anyhow, het is geen kwestie van wat beter is, maar wat voor jou het prettigste werkt.
Maar dit is niet in overeenstemming met het principe.
Het principe is juist dat je je commando begint waar de accolade zal sluiten. Dit doe je dus niet.
Een bijkomend ding: je begint een commando niet op een volgende lijn.
Wat jij beschrijft, is enkel een systeem voor if/elseiff/else, waardoor je dus niet meer consequent omspringt met de rest.
--------
Tenzij je denkt een uitleg te kunnen geven waarin je toont dat er wel een consequent systeem zit in wat je doet.
Gewijzigd op 12/01/2011 09:41:59 door Kris Peeters
Ik doe het altijd als volgt:
If statement:
If-else statement:
Gebeurt het op gelijk welke andere plek dat je iets zet rechts van een accolade?
Hoe past dit dan in een globaal systeem van indentering?
Dit topic is gesloten.