Optymalizacja klasy, jak? Opcje

[mijagi]

Napisałem ostatnio u siebie na blogu klase uploadującą jeden plik. Dodam, że głównie skupiałem się na bezpieczeństwie. Są osoby, które twierdzą, że ta klasa to "struktura w obiekcie". Chciałbym się dowiedzieć jak napisać taka klase lepiej, chodzi mi o wskazówki. Z góry dziex!
Tutaj klasa :

[PHP] pobierz, plaintext 
<?php
##################
# autor : mijagi #
##################
class uploadzik {
	public $rozmiar_max; 
	public $rozszerzenia = array();
	public $typy_mime = array();
	public $nazwa_input;
 
	private $nazwa_tmp;
	private $typ_mime;
	private $nazwa;
	private $rozmiar;
	private $bledy;
 
	public function __construct($rozmiar_max, $rozszerzenia, $typy_mime, $nazwa_input){
		if(isset($_FILES[$nazwa_input]) && is_array($_FILES[$nazwa_input])){
			if(!is_array($_FILES[$nazwa_input]['tmp_name']) && !is_array($_FILES[$nazwa_input]['name']) && !is_array($_FILES[$nazwa_input]['size']) && !is_array($_FILES[$nazwa_input]['error']) && !is_array($_FILES[$nazwa_input]['type'])){
				$this->nazwa_tmp = $_FILES[$nazwa_input]['tmp_name'];
				$this->typ_mime = $_FILES[$nazwa_input]['type'];
				$this->nazwa = $_FILES[$nazwa_input]['name'];
				$this->rozmiar = $_FILES[$nazwa_input]['size'];
				$this->bledy = $_FILES[$nazwa_input]['error'];
			}
 
		}
 
		if($this->sprawdzanie($rozmiar_max) == 1){
			if(is_array($rozszerzenia) && is_array($typy_mime)){
				$znaki = strlen($nazwa_input);
				if($znaki > 1 && $znaki < 5){
					$this->rozmiar_max = $rozmiar_max;
					$this->rozszerzenia = $rozszerzenia;
					$this->typy_mime = $typy_mime;
					$this->nazwa_input = $nazwa_input;
					$this->druk_input($this->ile_plikow);
				}else $this->komunikaty(4);
			}else $this->komunikaty(2);
		}else $this->komunikaty(3);
	} #<- konstruktor, sprawdzenie czy obiekt zostanie poprawnie utworzony
 
	private function sprawdzanie($x){
		if(isset($x)){
			if(is_numeric($x) && is_int($x)){
				return 1;
			}
		}
	} #<- funkcja pomagajaca konstruktorowi sprawdzic poprawnosc danych
 
	private function komunikaty($ktory){
		if(isset($ktory)){
			switch($ktory)
			{
				case 1:
				echo "Brak pliku";
				break;
 
				case 2:
				echo "Bledna wartosc dla rozszerzenia lub mime";
				break;
 
				case 3:
				echo "Bledna wartosc dla rozmiaru";
				break;
 
				case 4:
				echo "Bledna wartosc dla nazwy inputa";
				break;
 
				case 5:
				echo "Pliki o tym rozszerzeniu nie sa dopuszczalne";
				break;
 
				case 6:
				echo "Bledna wartosc zmiennej";
				break;
 
				case 7:
				echo "Zly wybor";
				break;
 
				case 8:
				echo "Plik nie jest obrazkiem";
				break;
 
				default:
				echo "Nieznany blad";
				break;
			}
		}
	} #funkcja drukujaca info o bledzie
 
	private function druk_input($ile){
		echo "<form action=\"\" method=\"POST\" enctype=\"multipart/form-data\">
		<input name=\"$this->nazwa_input\" type=\"file\" /><br />
		<input type=\"radio\" name=\"wybor\" value=\"zdjecie\"/>Fotka 
		<input type=\"radio\" name=\"wybor\" value=\"archiwum\" />Zip/rar<br/>
		<input type=\"submit\" name=\"submit\" value=\"up up!\"/></form>";
	} #funkcja drukujaca formularz
 
	private function rozszerzenie($roz){
		if(!is_array($roz)){
			$ext = substr($roz,strrpos($roz,'.')+1);
			return $ext;
		}
	} #funkcja sprawdzajaca rozszerzenia
 
	private function mimy($mime){
		if(in_array($mime, $this->typy_mime)){
			return 1;
		}
	}#funkcja sprawdzajaca mime
 
	private function wybor($wybor){
		if(!empty($wybor)){
			if(!is_array($wybor)){
				switch($wybor)
				{
					case 'zdjecie':
					return 1;
					break;
 
					case 'archiwum':
					return 2;
					break;
 
					default:
					return 3;
					break;
				}
			}else $this->komunikaty(6);
		}else $this->komunikaty(6);
	} #funkcja odpowiada za poprawnosc przypisania sprawdzania danych do zmiennej wybor
 
	private function rozmiar_pliku($rozmiar){
		if($rozmiar <= $this->rozmiar_max){
			return 1;
		}
	} #funkcja sprawdzajaca rozmiar pliku
 
	private function GD_img($tmp){
		$image = @getimagesize($tmp);
		if(is_array($image) && $image[0] > 2){
			return 1;
		}
	} #sprawdzanie czy obraz jest poprawny w GD
 
	private function zmiana_nazwy($nazwa){
		$nazwa = 'upload/'.md5(time()).'-'.$_SERVER['REMOTE_ADDR'].'-.'.$this->rozszerzenie($this->nazwa);
		return $nazwa;
	} #zmiana nazwy wg. wzoru
 
	private function info($x){
		if($x){
			echo 'Na serwer przeslano plik "'.$this->nazwa.'". O rozmiarze: '.$this->rozmiar.' bajtow.';
		}
	} # drukowanie info, jesli wszystko sie powiodlo
 
	public function up_up(){
		if(isset($_POST['submit']) && !is_array($_POST['submit'])){
			if($this->wybor($_POST['wybor']) < 3){
				if(is_uploaded_file($this->nazwa_tmp)){
					if($this->rozmiar_pliku($this->rozmiar) == 1){
						 if($this->wybor($_POST['wybor']) == 1){
						 	if(in_array($this->rozszerzenie($this->nazwa),$this->rozszerzenia[0])){
						 		if(in_array($this->typ_mime,$this->typy_mime[0])){
						 			if($this->GD_img($this->nazwa_tmp) == 1){
						 				$uped = move_uploaded_file($this->nazwa_tmp, $this->zmiana_nazwy($this->nazwa));
						 				$this->info($uped);
						 			}else echo $this->komunikaty(8);
						 		}else echo $this->komunikaty(2);
						 	}else echo $this->komunikaty(2);
						 }elseif($this->wybor($_POST['wybor']) == 2) {
						 	if(in_array($this->rozszerzenie($this->nazwa),$this->rozszerzenia[1])){
						 		if(in_array($this->typ_mime,$this->typy_mime[1])){
						 			$uped = move_uploaded_file($this->nazwa_tmp, $this->zmiana_nazwy($this->nazwa));
						 			$this->info($uped);
						 		}else echo $this->komunikaty(2);
						 	}else echo $this->komunikaty(2);
						 }
					}else echo $this->komunikaty(2);
				}else $this->komunikaty(1);
			}else $this->komunikaty(6); 
		}
	} #funkcja odpowiadajaca za sprawdzenie danych pliku i upload jesli wszystko gra
 
 
}
 
 
$rozmiar = 6291456; #max rozmiar pliku
$nazwa = 'plik'; #nazwa inputa
 
$roz_fot = array("jpg","png","gif"); #rozszerzenia dla obrazka
$roz_arch = array("zip","rar"); #rozszerzenia dla zip/rar
 
 
$mime_fot = array("image/gif","image/jpeg","image/png","image/jpg"); #typy mime dla obrazka
$mime_arch = array("application/zip","application/rar","application/x-compressed","application/x-zip-compressed","multipart/x-zip"); #typy mime dla rar/zip
 
#dalej palcow nie pchaj
$typy_mime = array($mime_fot,$mime_arch);
$rozszerzenia = array($roz_fot,$roz_arch);
$up = new uploadzik($rozmiar,$rozszerzenia,$typy_mime,$nazwa);
$up->up_up();
?>
[PHP] pobierz, plaintext 

Ten post edytował mijagi 21.08.2009, 08:29:55

[golaod]

Wywal klasę i napisz od początku. Na pewno będzie zoptymalizowana. Zero przemyślenia wygląda jakbyś po prostu jedną funkcję na sztywno podzielił na kilka metod i myślał, że to jest świetna klasa.

Po co dajesz komuś w formularzu wybór tego co wysyła ? On ma po prostu coś wysłać, a ty masz sprawdzić na miejscu jakie ma rozszerzenie i rozmiar. Btw. jeszcze jest np. rozszerzenie jpEg, tar, tar.gz, gz itp itd.
Dalej lecąc to po co mieszasz angielski z polskim ?
nazwa_tmp, typy_mime i nagle jeszcze mimY - WTF? co to jest up_up ?

Nie wiem czemu jednak osobiście jestem zrażony gdy widzę echo "coś" w metodzie klasy nie bardzo mi się to podoba chyba, że jest pod parametrem

[PHP] pobierz, plaintext 
function metoda( $output = false ) {
 
  if( ! $output ) {
   return "coś";
  }
  echo "coś";
 
}
[PHP] pobierz, plaintext 

Metoda komunikaty w ogóle nie jest przydatna. Zrobić metodę tylko po to by użyć switcha dla komunikatu błędu ?
Nie będę Ci pisać jak pisać swoje klasy. Polecam po prostu poczytać kod i zobaczyć jak to inni robią. Zrozumiesz wtedy, że konstrukcja konkretnej jednej Twojej metode jest rozbity na inne bardziej przejrzyste i lepsze jeśli chodzi o inną funkcjonalność( dodatkową )

[mijagi]

Wywal klasę i napisz od początku. Na pewno będzie zoptymalizowana. Zero przemyślenia wygląda jakbyś po prostu jedną funkcję na sztywno podzielił na kilka metod i myślał, że to jest świetna klasa.

Gdybym tak myślał to bym to nie przyszedl z prośbą o porady
Fakt z rozszerzeniami tar.gz, byłby problem, ze względu, że szukam ostatniej kropki.
Specjalnie dalem komentarze w tablicach z rozszerzeniami i mime, żeby user to rozszerzył sobie jeśli chce, a co do "jpEg" to mogłem najpierw użyć funkcji strlower - dzieki

Btw, nie czepiajcie sie nazw chciałbym się dowiedzieć czegos jak poprawic ten kod, nazwy naprawde sa tu nieistotne.

[erix]

[PHP] pobierz, plaintext 
if(isset($_POST['submit']) && !is_array($_POST['submit'])){
			if($this->wybor($_POST['wybor']) < 3){
				if(is_uploaded_file($this->nazwa_tmp)){
					if($this->rozmiar_pliku($this->rozmiar) == 1){
						 if($this->wybor($_POST['wybor']) == 1){
						 	if(in_array($this->rozszerzenie($this->nazwa),$this->rozszerzenia[0])){
						 		if(in_array($this->typ_mime,$this->typy_mime[0])){
						 			if($this->GD_img($this->nazwa_tmp) == 1){
						 				$uped = move_uploaded_file($this->nazwa_tmp, $this->zmiana_nazwy($this->nazwa));
						 				$this->info($uped);
						 			}else echo $this->komunikaty(8);
						 		}else echo $this->komunikaty(2);
						 	}else echo $this->komunikaty(2);
						 }elseif($this->wybor($_POST['wybor']) == 2) {
						 	if(in_array($this->rozszerzenie($this->nazwa),$this->rozszerzenia[1])){
						 		if(in_array($this->typ_mime,$this->typy_mime[1])){
						 			$uped = move_uploaded_file($this->nazwa_tmp, $this->zmiana_nazwy($this->nazwa));
						 			$this->info($uped);
						 		}else echo $this->komunikaty(2);
						 	}else echo $this->komunikaty(2);
						 }
					}else echo $this->komunikaty(2);
				}else $this->komunikaty(1);
			}else $this->komunikaty(6); 
		}
[PHP] pobierz, plaintext 

O wyjątkach, to nie słyszałeś? Czytelność kodu jest fatalna, wszystko można by było zgrabnie zrobić.

[PHP] pobierz, plaintext 
}else $this->komunikaty(4);
[PHP] pobierz, plaintext 

Hmm, ciekawe co zrobisz, gdy zechcesz rozbudować klasę i dodać nowe komunikaty o błędach, zobaczymy, do jakiego momentu się nie pogubisz. Już nawet PHP dla $_FILES['file']['error'] używa STAŁYCH, a nie surowych kodów numerycznych.

[PHP] pobierz, plaintext 
private function rozszerzenie($roz){
		if(!is_array($roz)){
			$ext = substr($roz,strrpos($roz,'.')+1);
			return $ext;
		}
	}
[PHP] pobierz, plaintext 

A o pathinfo nie słyszał?

[PHP] pobierz, plaintext 
private function GD_img($tmp){
		$image = @getimagesize($tmp);
		if(is_array($image) && $image[0] > 2){
			return 1;
		}
	}
[PHP] pobierz, plaintext 

A teraz patrz na dokumentację:

The getimagesize() function does not require the GD image library.

Co jeszcze?

1. Szkoda, że nie masz żadnej obsługi uploadu wielu plików zapisanych jako tablica; trzeba dla każdego ręcznie z osobna.
2. brak phpDoc
3. nazewnictwo nieco dziwne; ani intuicyjne, ani konsekwentne - w jednym miejscu masz up_up, w drugim mimy, że tylko Ty wiesz, o co chodzi.
4. za bardzo strukturalny, aż prosi się o wyjątki, a Ty robisz drzewko warunków
5. komunikaty o błędzie lepiej zwracać właśnie przez wyjątki
6. jak mam z poziomu skryptu sprawdzić, czy upload się powiódł? Żadnego return, o wyjątku z odpowiednim kodem błędu nie wspomnę

Poczytaj trochę o programowaniu obiektowym, bo na razie to jest tylko strukturalny ubrany w inne nazwy zmiennych...

[cojack]

A co te public $zmienna robią? Nie słyszał Pan o zasadzie hermetyzacji?

Ten post edytował cojack 22.08.2009, 23:37:57