(worst) Code practices - Issue #1

Din cand in cand imi cad pe mana coduri sursa dintre cele mai proaste. Din cand in cand voi posta cate un astfel de cod intr-un thread nou cu acelasi titlu, lasandu-l pe mana voastra, a celor incepatori, sa il corecteze, sa il discute, sa il imbunatateasca.

Issue #1 - originar: http://www.pastebin.ca/976798

<div id="text"> GROVE COMMUNICATIONS LTD - DATABASE
<center>
<table type="text" border="1" cellpadding="1" cellspacing="0" width="100%" style="border-collapse: collapse" bordercolor="#1875D6" bgcolor="#C2D4E3">
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="3%"><font face="Arial" size="1">No.</font></td>
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="6%"><center><font face="Arial" size="1">Type</font></td>
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="6%"><center><font face="Arial" size="1">Leader</font></td>
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="3%"><center><font face="Arial" size="1">Date</font></td>
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="6%"><center><font face="Arial" size="1">Closer</font></td>
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="10%"><font face="Arial" size="1">Client</font></td>
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="10%"><font face="Arial" size="1">CompanyName</font></td>
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="6%"><center><font face="Arial" size="1">PhoneNumber</font></td>
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="3%"><center><font face="Arial" size="1">Network</font></td>
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="3%"><center><font face="Arial" size="1">Mobiles</font></td>
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="3%"><center><font face="Arial" size="1">Monthly Spend</font></td>
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="3%"><center><font face="Arial" size="1">ContractEnd</font></td>
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="3%"><center><font face="Arial" size="1">CallBack</font></td>
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="3%"><center><font face="Arial" size="1">Contract Length</font></td>
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="10%"><center><font face="Arial" size="1">Comment</font></td>
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="10%"><center><font face="Arial" size="1">CloserComment</font></td>
<td valign="bottom" align="center" bgcolor="#FFFBFF" width="10%"><center><font face="Arial" size="1">LeaderComment</font></td>
</tr>

<?php
$server = "localhost"; // numele serverului sql
$username = "root"; // numele de conectare la bd sql
$password = "passw0rd"; // parola pentru conectarea la sql
$database = "catalin"; // numele bazei de date sql
$tabel = "catalin"; // numele tabelului in care lucram
$coloana_bd1 = "Type"; $coloana_bd2 = "LeaderName";$coloana_bd3 = "DateMade";$coloana_bd4 = "Closer";
$coloana_bd5 = "ClientName";$coloana_bd6 = "CompanyName";$coloana_bd7 = "ContactNumber";
$coloana_bd8 = "Network";$coloana_bd9 = "NumberOfMobilePhones";$coloana_bd10 = "MonthlySpend";
$coloana_bd11 = "Contract";$coloana_bd12 = "Call";$coloana_bd13 = "Log";
$coloana_bd14 = "Comment";$coloana_bd15 = "CloserComennt";$coloana_bd16 = "LeaderComment";
$IDU=($_GET['id']);
$IDU2=$IDU++;

$rezultate_pagina = 100;
$numere_rand = 30; // specificati dupa cate numere se va trece la rand nou 

$Day7 = $_GET['Day7'];
$Month7 = $_GET['Month7'];
$Day8 = $_GET['Day8'];
$Month8 = $_GET['Month8'];

$numele_paginii = "pag1.php?Month7=$Month7&Day7=$Day7&Month8=$Month8&Day8=$Day8"; 

$conectare = mysql_connect($server, $username, $password) or die(
	'<br>Eroare: Nu se poate conecta la baza de date la linia '.__LINE__.
	'<br> in fisierul '.$_SERVER['PHP_SELF'].'?'.$_SERVER['QUERY_STRING'].
	'<br>'.mysql_error());

mysql_select_db($database, $conectare) or die(
	'<br>Eroare: Nu se poate selecta baza de date la linia '.__LINE__.
	'<br> in fisierul '.$_SERVER['PHP_SELF'].'?'.$_SERVER['QUERY_STRING'].
	'<br>'.mysql_error());

$comanda_sql = "SELECT count(*) FROM `".$tabel."` WHERE `Type` = 'Lead' AND DateMade BETWEEN '".$first_date."' AND '".$last_date."'";

if ((isset($_GET['id']) and !empty($_GET['id']) and is_numeric($_GET['id'])) 
	or (isset($_GET['id']) and $_GET['id'] != '0'))
	{
	// daca se cere o anumita pagina 

$first_date = $_GET['Month7'].'-'.$_GET['Day7'];
$last_date = $_GET['Month8'].'-'.$_GET['Day8'];
$id = mysql_real_escape_string($_GET['id']);

$comanda_sql = "SELECT * FROM `".$tabel."` WHERE DateMade BETWEEN '".$first_date."' AND '".$last_date."'  ORDER BY id ASC LIMIT ".$id.",".$rezultate_pagina;
	}
else
	{
	// daca se acceseaza pagina prima data se afiseaza ultimele inregistrari din bd 

$first_date = $_GET['Month7'].'-'.$_GET['Day7'];
$last_date = $_GET['Month8'].'-'.$_GET['Day8'];
$comanda_sql = "SELECT * FROM `".$tabel."` WHERE DateMade BETWEEN '".$first_date."' AND '".$last_date."'  ORDER BY id ASC LIMIT 0,".$rezultate_pagina;
	} 

$interogare_sql = mysql_query($comanda_sql) OR die(
	'<br>Eroare: Nu se poate efectua introgarea la baza de date la linia '.__LINE__.
	'<br> in fisierul '.$_SERVER['PHP_SELF'].'?'.$_SERVER['QUERY_STRING'].
	'<br>'.mysql_error());

$total = mysql_num_rows($interogare_sql);

if ($total != 0)
	{
	$nr = 1;
	while ($rand = mysql_fetch_array($interogare_sql))
		{
echo '<tr>';
echo '<td>' .stripslashes($IDU);
echo '<td>' .stripslashes($rand[$coloana_bd1]);echo '</td>';echo '<td>' .stripslashes($rand[$coloana_bd2]);echo '</td>';
echo '<td>' .stripslashes($rand[$coloana_bd3]);echo '</td>';echo '<td>' .stripslashes($rand[$coloana_bd4]);echo '</td>';
echo '<td>' .stripslashes($rand[$coloana_bd5]);echo '</td>';echo '<td>' .stripslashes($rand[$coloana_bd6]);echo '</td>';
echo '<td>' .stripslashes($rand[$coloana_bd7]);echo '</td>';echo '<td>' .stripslashes($rand[$coloana_bd8]);echo '</td>';
echo '<td>' .stripslashes($rand[$coloana_bd9]);echo '</td>';echo '<td>' .stripslashes($rand[$coloana_bd10]);echo '</td>';
echo '<td>' .stripslashes($rand[$coloana_bd11]);echo '</td>';echo '<td>' .stripslashes($rand[$coloana_bd12]);echo '</td>';
echo '<td>' .stripslashes($rand[$coloana_bd13]);echo '</td>';echo '<td>' .stripslashes($rand[$coloana_bd14]);echo '</td>';
echo '<td>' .stripslashes($rand[$coloana_bd15]);echo '</td>';echo '<td>' .stripslashes($rand[$coloana_bd16]);echo '</td>';
echo '</tr>';
$nr++;
$IDU++;
		}
	$randuri = floor($nr_total_pagini/$numere_rand);
	$nrr = 0;

	for ($ee=1;$ee<=$randuri;$ee++)
		{
		$nrr = $nrr+$numere_rand;
		$rnd[$ee] = $nrr; 
		}

	$id_pentru_link = 0;
	echo '<div align="center">'; 

	for ($pagina_nr=1;$pagina_nr<=$nr_total_pagini+1;$pagina_nr++) 
		{  
		echo ' - <a href="'.$numele_paginii.'?id='.$id_pentru_link.'">' .$pagina_nr.' </a>'; 
	   if (isset($rnd))
			{
			foreach($rnd as $val) 
				{ 
				if ($pagina_nr == $val) 
					{
					// daca au fost afisate $numere_rand se trece la rand nou si se afiseaza in continuare
					echo "<br>"; 
					} 
				}
			} 
	   $id_pentru_link = $id_pentru_link + $rezultate_pagina; 
	   } 
	echo ' - <br><br></div>';
	}
?>

Distractie placuta.

PS: scopul este educativ, nu derâdere, deci rog stati on-topic

Edited by OriginalCopy, 08 April 2008 - 16:14.

pai prima chestie care am observat-o a fost ca lipseste dupa <table type="text" border="1" cellpadding="1" cellspacing="0" width="100%" style="border-collapse: collapse" bordercolor="#1875D6" bgcolor="#C2D4E3"> un <tr>
o alta chestie care tine tot de html sunt culorile si felul in care au fost alese...decat sa pun pentru fiecare td as fi un pentru <tr> ul care lipseste

$IDU=($_GET['id']);
$IDU2=$IDU++;

aici parantezele nu isi au rostul si ar fi fost mai frumos sa scrie $IDU2=$_GET['id']+1;

o alta chestie care am vazut-o e ca foloseste prea multe "variabile" sa le spun asa in url

nicaieri nu se atribuie nici o valoare variabilelor $first_date si $last_date

asta e cea mai tare if ((isset($_GET['id']) and !empty($_GET['id']) and is_numeric($_GET['id'])) din moment ce verifici daca id are o valoare la ce sa mai vezi daca e goala ? daca stau sa ma gandesc cred ca nu avea ce sa caute nici is_numeric pentru ca id-urile intotdeauna sunt numerice
or (isset($_GET['id']) and $_GET['id'] != '0')) din cunostintele mele auto incrementarea porneste de la 1

$id = mysql_real_escape_string($_GET['id']); nu era nevoie de nici un mysql_real_escape_string

nu ar fi frumos daca toate paginile ar avea un nume de genul $numele_paginii = "pag1.php?Month7=$Month7&Day7=$Day7&Month8=$Month8&Day8=$Day8";  ? )=)))))))

primul if cred ca nu isi are rostul pentru ca idul isi va lua o valoare diferita de 0 tot timpul

putea sa faca o functie pentru erori ca sa nu puna de fiecare data toata polologhia aia

echo '<td>' .stripslashes($rand[$coloana_bd1]);echo '</td>';echo '<td>' .stripslashes($rand[$coloana_bd2]);echo '</td>'; strislashes nu isi are rostul aici...e pus degeaba...si ar fi putut scrie echo '<td>'.$rand[$coloana_bd1.'</td><td> .......

$randuri = floor($nr_total_pagini/$numere_rand); nu prea imi dau seama ce a vrut sa zica cu asta...probabil a vrut sa calculeze numarul de pagini..dar nu imi amintesc ca $nr_total_pagini sa aiba vre-o valoare pe undeva

for ($ee=1;$ee<=$randuri;$ee++)
        {
        $nrr = $nrr+$numere_rand;
        $rnd[$ee] = $nrr;
        }
ce o fi vrut sa zica cu asta ?):|

m-am plictisit de studiat toata prostia aia...

o impresie despre cod: ceva unic,ceva nemaivazut...variabile incurcate,functii folosite unde nu trebuie,chestii care nu-si au rostul

daca am gresit la ceva corectati-ma

@vali,

Ai un stil haotic de a studia un script haotic. Iese o mare varza. Linisteste-te putin, ia-o sistematic, ajuta-ne sa intelegem ce vrei sa spui .

$IDU=($_GET['id']);
$IDU2=$IDU++;

Te-ai legat de paranteze si de faptul ca "ar fi mai frumos scris $IDU2=$_GET['id']+1;".
Este o eroare (grava), totusi, ascunsa in aceste 2 randuri. Care si de ce?

in postul precedent am vrut sa ating cat mai multe puncte intr-un timp cat mai scurt din motive personale si din cauza asta cred ca a iesit ceva haotic

$IDU=($_GET['id']);
$IDU2=$IDU++;

Quote

aici nu mi se pare a fi vre-o gresala doar ca nu mi-a placut mie cum a fost scris codul

Quote

care e eroarea ?  

Dupa secventa

$IDU=$_GET["id"];
$IDU2=$IDU++;

ce valori vor avea variabilele $IDU si $IDU2?


Te-ai prins?


EDIT:

Se pare ca eu aveam o gresala in logica . Revin cu detalii.


EDIT2:

Am incurcat putin borcanele.

$a++ => expresia are valoarea variabilei inainte de incrementare.
++$a => expresia are valoarea variabilei dupa incrementare.


Prin urmare, din punct de vedere logic este corect:

$IDU=$_GET["id"];
$IDU2=$IDU++;

Daca ar fi fost scris

$IDU=$_GET["id"];
$IDU2=++$IDU;

ar fi existat o gresala de logica, pentru ca in acest caz atat $IDU2 cat si $IDU ar fi avut aceeasi valoare ($IDU + 1).



Totusi, linia "$IDU2=$IDU++" produce efectul "$IDU2 = $IDU" si nu "$IDU2 = $IDU + 1"

Edited by crz, 10 April 2008 - 13:57.

dau si eu o mica parere :
- folosire de variabile globale in script ( eu prefer sa le transform in variabile locale cu mysql escape unde e cazul )
- sql injection security bug
- variabile create fara rost ( $coloana_bd )
- incrementari fara rost $nr++;$IDU++; ( se putea folosi mysql_num_rows )
- comanda in evaluare end for  ( for ($pagina_nr=1;$pagina_nr<=$nr_total_pagini+1;$pagina_nr++)  )
- ultimul for in if in for in if ... nu imi pot imagina ce e acolo ... si ce rost ar avea

si ar mai fi poate multe altele ... dar asa la primul ochi

Quote

corect. id-urile sunt mereu numerice, dar nu si inputul utilizatorilor

BETTER code practices

eu zic doar atat... codul trebuie separat cat mai mult de continut....iar securitatea....practic nu exista

daca siteul ar avea 1000 de pagini, oare in cat timp ar schimba el parola la baza de date =))

Edited by iulyan_25, 20 October 2008 - 13:04.