Hjelp med opprydding og fiksing av kode

[uranus]

Hei!

 

Her er koden min som heter visforum.php. Dette viser trådene i en kategori.

 

Her er greia.

 

http://tgatebil.no/index.php?s=forum/visforum&id=6

 

"Viktig:" skal alltid være øverst, altså utenom while. Men tenker teit nå så får det ikke til. Ellers forslag til ryddigere kode taes imot.

 

Håper noen kan hjelpe!

 

 

 

<?php
include("./config.php");
$tbl_name="forum_question"; // Table name
$kategori_id = $_GET['id'];

$sql="SELECT * FROM $tbl_name WHERE category = $kategori_id ORDER BY 'id' DESC";
// OREDER BY id DESC is order result by descending
$result=mysql_query($sql);

if ($kategori_id == 1) { echo "<h2 style=\"margin: 0; padding: 0;\">Telemark Gatebil</h2>"; }
elseif ($kategori_id == 2) { echo "<h2 style=\"margin: 0; padding: 0;\">Motorsport</h2>"; }
elseif ($kategori_id == 3) { echo "<h2 style=\"margin: 0; padding: 0;\">Happenings</h2>"; }
elseif ($kategori_id == 4) { echo "<h2 style=\"margin: 0; padding: 0;\">Mediaforum</h2>"; }
elseif ($kategori_id == 5) { echo "<h2 style=\"margin: 0; padding: 0;\">Offtopic</h2>"; }
elseif ($kategori_id == 6) { echo "<h2 style=\"margin: 0; padding: 0;\">Carspotting</h2>"; }
elseif ($kategori_id == 7) { echo "<h2 style=\"margin: 0; padding: 0;\">Prosjektforum</h2>"; }
elseif ($kategori_id == 8) { echo "<h2 style=\"margin: 0; padding: 0;\">Kontroll</h2>"; }
else { }
echo "<a href=\"?s=forum/forside\">Tilbake til forum forsiden.</a><br><br>";

?>
<table width="100%" border="0" align="center" cellpadding="3" cellspacing="1" bgcolor="#CCCCCC">
<tr>
<td width="6%" align="center" bgcolor="#E6E6E6"><strong>#</strong></td>
<td width="53%" align="center" bgcolor="#E6E6E6"><strong>Tråd</strong></td>
<td width="15%" align="center" bgcolor="#E6E6E6"><strong>Svar</strong></td>
<td width="13%" align="center" bgcolor="#E6E6E6"><strong>Visninger</strong></td>
<td width="13%" align="center" bgcolor="#E6E6E6"><strong>Dato/tid</strong></td>
</tr>

<?php
while($rows=mysql_fetch_array($result)){ // Start looping table row
?>

<?php if ($rows['status'] == 1) { ?>
<tr>
<td bgcolor="#FFFFFF"><div align="center"><img src="forum/ikon.gif" alt="ikon" width="31" height="31" /></div></td>
<td bgcolor="#FFFFFF"><img src="http://www.diskusjon.no/style_images/main_gfx/newpost.gif" align="bilde" /> <span style="font-size:10px;">Viktig:</span> <a href="?s=forum/view_topic&id=<?php echo $rows['id']; ?>&kategori=<?php echo $_GET['id']; ?>">
<?php echo $rows['topic']; ?></a><BR></td>
<td align="center" bgcolor="#FFFFFF"><?php echo $rows['reply']; ?></td>
<td align="center" bgcolor="#FFFFFF"><?php echo $rows['view']; ?></td>
<td align="center" bgcolor="#FFFFFF"><?php echo date("d.m.Y H:i:s",$rows['datetime']); ?></td>
</tr>
<?php } ?>

<?php if ($rows['status'] == 0) { ?>
<tr>
<td bgcolor="#FFFFFF"><div align="center"><img src="forum/ikon.gif" alt="ikon" width="31" height="31" /></div></td>
<td bgcolor="#FFFFFF"><a href="?s=forum/view_topic&id=<?php echo $rows['id']; ?>&kategori=<?php echo $_GET['id']; ?>"><?php echo ucwords($rows['topic']); ?></a><BR></td>
<td align="center" bgcolor="#FFFFFF"><?php echo $rows['reply']; ?></td>
<td align="center" bgcolor="#FFFFFF"><?php echo $rows['view']; ?></td>
<td align="center" bgcolor="#FFFFFF"><?php echo date("d.m.Y H:i:s",$rows['datetime']); ?></td>
</tr>
<?php } ?>

<?php
// Exit looping and close connection
}
mysql_close();
?>
<tr>
<td colspan="5" align="right" bgcolor="#E6E6E6"><a href="?s=forum/create_topic&id=<?php echo $kategori_id; ?>"><strong>Ny tråd </strong> </a></td>
</tr>
</table>

 

 

[kakkle]

Siden det sikkert kommer til å være flere VIKTIG:, så looper du bare igjennom de postene som har status 1 (regner med at dette tilsvarer "viktig:", og skriver disse ut, før du skriver ut de andre:

SELECT * FROM $tbl_name WHERE category = $kategori_id AND status=1 ORDER BY 'id' DESC

 

Tror jeg ville gjort det slik

[uranus]

Thank you

 

Da var det i orden hvertfall

 

Setter ellers pris på hjelp i opprydding/forbedring

[Garanti]

Husk å escape input!

Som f.eks

$kategori_id = mysql_escape_string($_GET['id']);

[uranus]

Okai, men hva vil det si?

[Martin A.]

Siden jeg skrev litt mer enn de andre, og noe av det jeg nevner ble nevnt tidligere blir det noen gjentagelser her. Rett og slett fordi jeg ikke gidder å fjerne det.

 

Bruk en eksisterende/lag din egen templateklasse. Blande HTML og PHP ser alltid like grusomt ut.

Tror heller ikke diskusjon.no er kjempeglad for at du hotlinker til det bildet.

Forumforsiden skrives i ett ord.

Ikke avslutt PHP (?>) så fort du er ferdig med noe. Det gjør koden enda styggere.

<?php 
if ($rows['status'] == 0) 
{
echo "<tr>
	<td bgcolor='#FFFFFF'><div align='center'><img src='forum/ikon.gif' alt='ikon' width='31' height='31' /></div></td>
	<td bgcolor='#FFFFFF'><a href='?s=forum/view_topic&id={$rows['id']}&kategori={$_GET['id']}'>" . ucwords($rows['topic']) . "</a><!-- FYSJAMEGEN --><BR><!-- FYSJAMEGEN slutt--></td>
	<td align='center' bgcolor='#FFFFFF'>{$rows['reply']}</td>
	<td align='center' bgcolor='#FFFFFF'>{$rows['view']}</td>
	<td align='center' bgcolor='#FFFFFF'>" . date('d.m.Y H:i:s',$rows['datetime']) . "</td>
</tr>";
} 
?>

Lenker skal/bør inneholder full URL, ikke relativ. Les hva W3C skriver om URI, mener det står der.

bgcolor er vel også en attribut som er på vei ut. "style='background-color: #Farge'" er vel å foretrekke. Aller helst skal slikt kontrolleres av CSS.

SELECT * FROM foo; er talentløst med mindre du bruker absolutt alle feltene. Bruker du * kjøres det egentlig to spørringer. En for å finne ut hvilke felt du har i tabellen, og en for å hente ut selve dataen. Spesifiserer du hvilke felt du bruker sparer du deg for det.

Denne burde kanskje stått øverst, men det er jo ingen som har lært noe av det når jeg har skrevet det øverst. Scriptet ditt er åpent for SQL Injections. Aldri stol på brukeren, valider all input.

 

 

EDIT: egentlig bør du kjøre en test for å se om $_GET['id'] is_numeric().

if( !is_numeric( $_GET['id'] ) )
{
die( "Hut deg hjem" );
}
$_GET['id'] = (int)$_GET['id'];

Endret 15. april 2008 av M4rTiN

[uranus]

Takker for info