Witam jestem poczatkujacy w sprawach php ucze sie dopiero kilkanascie dni ale napisałem juz pare skryptów dzisiaj chciałem dac pod ostrzał skrypt do wyswietlania newsów z bazy.

Struktóra bazy danych jest następująca:
ID | TITLE | SHORTNEWS | LONGNEWS | DATA

A o to kod pliku newsy_wyswietl.php

[PHP] pobierz, plaintext 
<?php
$mysql = '../conf/mysql.ini.php';
 
/* Za pomocą funkcji if sprawdzamy czy istnieje plik ze zmiennej $mysql, gdy funk
cja zwróci wartosc TRUE
   wczytujemy plik w innym przypadku wyswietlamy blad. */
 
if ( file_exists($mysql)) 
{
	require_once($mysql);  
} else echo ('Nie można się połączyć z bazą ponieważ plik'.$mysql.' nie istnieje.');
 
/* Deklarujemy funkcje shortnews() */
function shortnews()
{
	/* ile na strone */
	$ile = 10;
	$numrows = mysql_num_rows(mysql_query("SELECT * FROM cms_news"));
	if(!$p) $p = 0;
	/* zabezpieczenie przed nienumerycznymi wartosciami */
	$p = (int)$p; 
	$ile = (int)$ile;
 
	$zapytanie = "SELECT * FROM cms_news ORDER by data LIMIT $p,$ile";
	$wykonaj = mysql_query($zapytanie);
 
	while($dane=mysql_fetch_array($wykonaj)) { echo "<table width="500px"><tr>
	  <td>".$dane['title']."</td></tr>
	   <tr><td>".$dane['shortnews']."<br><a href="news_wyswietl.php?wiecej=1&id=".$dane['id']."">Więcej ...</a></td></tr>
		<tr><td>".$dane['data']."</td>
		</tr></table><br /><br />";
		}
	 
	echo "strona: ";
	for($i=0;$i<ceil($numrows/$ile);$i++) {
	echo '<a href="'.$PHP_SELF.'?p='.($i*$ile).'">'.($i+1).'</a> ';};
 
}
 
/* Deklarujemy funkcję longnews() */
function longnews()
{
 
	if (is_numeric($_GET['id'])) 
	{
		$id = $_GET['id'];
	}
	else
	{
		$id = rand(); /* Jeżeli id nie bedzie liczba lub bedzie puste wtedy zostanie wylosowane
						 jezeli nie bedzie go w bazie to nic sie nie wyswietli a 
						 jezeli dany id bedzie w bazie wyswietli on w formie dludiej newsa o wylosowanym
 id */ 
	};
 
	$zapytanie2 = "SELECT * FROM cms_news WHERE id=$id LIMIT 1";
	$wykonaj2 = mysql_query($zapytanie2);
 
	if ($dane2=mysql_fetch_array($wykonaj2)) { echo "<table width="500px"><tr>
	  <td>".$dane2['title']."</td></tr>
	   <tr><td>".$dane2['shortnews']."<br /><br />".$dane2['longnews']."</td></tr>
		<tr><td>".$dane2['data']."</td>
		</tr></table>";};
}
 
/* Jezeli zmienna $wiecej jest pusta wtedy zostana wyswietlone wszystkie newsy w 
formie krótkiej,
jezeli wartos ta bedzie liczba oraz bedzie równa 1 wtedy zostanie wyswietlony je
den news w całości,
w innym wypadku zostanie wyświetlony bład */
 
$wiecej = $_GET['wiecej'];
 
if (empty($wiecej)) 
{
	shortnews();	
} 
else 
{
		if (is_numeric($wiecej) && $wiecej = 1) 
		{
			longnews();
		} 
		else echo ('Błąd wartość zmiennej $wiecej jest nie prawidłowa !!!');	   
};
 
?>
[PHP] pobierz, plaintext 

Proszę o ocenę i ew. propozycje poprawy i zmian

Jak na jeden z pierwszych skryptow wyglada niezle


Zmienne, ktore maja byc stale, a do tego sa w jakis spsob konfiguracja w skrypcie (jak $ile = 10) powinny byc definiowane przez define() na poczatku skryptu.
Jezeli bedziesz chcial zmienic liczbe wyswietlanych newsow, to nie bedzie szukal po calym pliku tej zmiennej.


Losowanie newsa raczej jest zbedne. Nie podales przedzialu losowania, wiec raczej napewno wylosuje nieistniejacy. Moze lepiej wyswietlic news z ID 1 ?


Inna sprawa to taka, ze mozna by sie pokusic o jakis system wzorcow aby odseparowac, warstwe prezentacji. Bedzie czytelniej i latwiej w utrzymaniu (zmianach).


Ostatnia chyba sprawa to nazewnictwo zmiennych i komentarze.

$p, $ile nic nie mowia. Jezeli wrocisz do tego skryptu po jakims czasie, to tez Ci one nic nie powiedza (bez analizy kodu).

Dodatkowo, mozna by sie pokusic o inna firme zapisu zmiennych.
Np:
- $alaMaKota albo
- $iAlaMaKota lub $intAlaMaKota.

Pierwszy spsob wyroznia czlowny wyrazow zmiennej. Jest to znacznie czytelniejsze niz ciag nazwa pisana cala z malych liter $alamakota.

Drugi przyklad wprowadza na poczatku nazwy zmiennej infromacje o typie zmiennej (o ile mnei pamiec nie myli, nazywa sie to notacja wegierska?).
i dla int
f dla float
s dla stringa
o dla object itd.

Mozna tez uzyc dluzszej formy typu $intCos, $strTekst, $floatZmienna


Komentarze, ktore sa jedna linia nie trzeba otaczac /* */ do tego jest // na poczatku linii.
Niby mala roznica ale ulatwia np. odkomentowywanie danych linii.


Polecam Ci poczytac takze o standardach kodowania.
http://wortal.php.pl/wortal/artykuly/pomys...dardy_kodowania
Dzieki temu wprowadzisz do swoich skryptow, pewien porzadek i przemyslana strukture.

Milej lektury

Co do zmian to je wprowadze u siebie tu juz nie bede pisal a co do standartów kodowania to chyba tak zle nie jest starałem sie pisac tak zeby bylo czytelnie i czesc z tego co wyczytalem w artykule to juz mam w tym skrypcie zrobione

Ale bede staral sie to lepiej formatowac

Dzięki za opinie

a jak jescze mozesz to napisz cos a propos zabezpieczen tzn w sumie niema co tu zabezpieczacc ale chodzi mi o to uzycie is_numeric czy Ci sie podoba itd

Jak na kilkanaście dni to wymiatasz Mnie kolega do bazy danych przekonywał z 2 miesiące

[PHP] pobierz, plaintext 
<?php
$wiecej = $_GET['wiecej'];
 
		if (is_numeric($wiecej) && $wiecej = 1) 
 
		{
 
			longnews();
 
		} 
 
		else echo ('Błąd wartość zmiennej $wiecej jest nie prawidłowa !!!');
?>
[PHP] pobierz, plaintext 

Tego kodu nie rozumiem ;p przekombinowałeś ;] Ja bym to napisał w 2 linijkach

[PHP] pobierz, plaintext 
<?php
  if(isset($_GET['wiecej'])) longnews();
	else shortnews();
?>
[PHP] pobierz, plaintext 

Co mnie obchodzi czy ktoś w linku napisze index.php?wiecej=1 czy index.php?wiecej=oTakChceWiecej ważne aby zmienna miała wartość true (no zeby w ogole byla zadeklarowana)

notacja węgierska => http://pl.wikipedia.org/wiki/Notacja_węgierska moze nie jest stworzona do php(brakuje w nim kilku bajerow) ale zawsze mozna sie pokusić o stosowanie, Nie wiem jak ty ale ja po pewnym czasie doszedłem do wniosku że polskie nazwy zmiennych są be i teraz pisze po en i kod jest bardziej dostępny dla obcokrajowcow ;]

~SHIP a wiec w tym kodzie chodzi o to iż za pomocą funkcji is_numeric sprawdzam czy zmienna $wiecej jest liczba jezeli tak to czy jest jeden bo w innych wypadkach gdy zmienna jest np literami wyswietli nam bład chodzi tu o to iz chce w jaki kolwiek sposób zabepiczyc skrypt przed atakami SQL Iniection czy jakos tak sie to pisało wiem ze w sumie niema tu co zabezpieczac bo hasla sie za pomoca tego nie wyjmie tzn w tym skrypcie bo tu niema jak narazie hasla ale to tak zebym juz pamietam o tym zabezpieczenia a jescze jezeli wartosc bedzie liczba inna niz 1 to wtedy zostanie wyswietlony tylko pierwszy news w całosci.

Liczba 1 działa jak TRUE a pozostałe jak FALSE

a co bo baz danych to wydaja mi sie o wiele latwiejsze niz pliki a pozatym przedewszystkim sa bezpieczniejsze no i wydaje m isie za bardziej funkcjonalne

Mylisz pojęcia ;] sql injection jest mozliwe tylko gdy zmienna jest użyta w zapytaniu SQL np...

[PHP] pobierz, plaintext 
<?php
mysql_query('SELECT * FROM tabelka WHERE id="'.$_GET['id'].'"');
?>
[PHP] pobierz, plaintext 

Taki kod jest niebezpieczny i w tym przypadku przydałoby sie sprawdzić $_GET['id']

U ciebie zmienna $_GET['wiecej'] jest tylko sprawdzana pod względem wartości, nie jest przekazywana do jakiegokolwiek zapytania wiec moim zdaneim twoje rozwiązanie jest przekombinowane ;] co za różnica czy zmienna $_GET['wiecej'] bedzie cyfrą czy nie i tak skrypt nie wygeneruje żadnych błedów i nie da okazji na sql_injection ;]
A skoro nie widać różnicy to po co przepłacać? To zawsze tych kilka microsekund ;]

Spoko masz racje moj blad (brak wiedzy) ale cóz czlowiek uczy sie na błędach
zaraz w takim razie to poprawie i bede pamiętał na następny raz.

[PHP] pobierz, plaintext 
<?php
if (isset($wiecej) && $wiecej = 1) longnews();
		else shortnews();
?>
[PHP] pobierz, plaintext 

tak moze byc ?

ps. czyli to:

[PHP] pobierz, plaintext 
<?php
function longnews()
{
 
	if (is_numeric($_GET['id'])) 
	{
		$id = $_GET['id'];
	}
	else ...
?>
[PHP] pobierz, plaintext 

mam zostawic bo ta zmienna jest wstawiana do zapytani sql

tak tylko

[PHP] pobierz, plaintext 
<?php
if (isset($wiecej) && $wiecej = 1) longnews();
		else shortnews();
?>
[PHP] pobierz, plaintext 

uciąłeś jeden znak '='. Pozatym nie wiem po co montować dodatkową zmienną $wiecej ;] lepiej sie odwołać do $_GET['wiecej']

Spoko dzięki za podszkolenie