Witam. Rozpocząłem żmudne zagłębianie się w tajniki programowania zorientowanego obiektowo, przeczytałem kilka artykułów i pomyślałem że najlepiej jest sprawdzić swoją wiedzę w praktyce. Tak oto powstała klasa do uploadu plików. Przyznam się, że pobrałem sobie podobną klasę z phpclasses.org, aby zobaczyć mniej więcej jak to wygląda. Chciałbym Was zapytać czy ma to coś w ogóle wspólnego ze stylem OOP ?

class.upload.php

[PHP] pobierz, plaintext 
<?php
class Upload {
	var $folder;
	var $types = array('application/msword', 'application/octet-stream', 'image/jpeg'); 
 
	function __construct($folder) {
		$this->folder = $folder;
		$this->upload();
	}
 
	public function check() {
            if (isset($_POST['wyslij'])) {
		if($_FILES['plik']['error']>0) {
			echo "Wystąpił problem: ";
			switch($_FILES['plik']['error']) {
				case 1: echo "rozmiar pliku przekroczył wartość xMB.<br /><a href='java script:history.back(1)'>Wróć</a>"; break;
				case 2: echo "rozmiar pliku przekroczył wartość xMB.<br /><a href='java script:history.back(1)'>Wróć</a>"; break;
				case 3: echo "plik wysłany częściowo.<br /><a href='java script:history.back(1)'>Wróć</a>"; break;
				case 4: echo "nie wysłano żadnego pliku.<br /><a href='java script:history.back(1)'>Wróć</a>"; break;
				case 6: echo "nie można wysłać pliku: nie wskazano katalogu tymczasowego.<br /><a href='java script:history.back(1)'>Wróć</a>"; break;
				case 7: echo "nie zapisano pliku na dysku.<br /><a href='java script:history.back(1)'>Wróć</a>"; break;
				}
			}
		else {
			if (in_array($_FILES['plik']['type'], $this->types)) {
				return true;
			}
		}
	}
}
 
public function upload() {
		if($this->check()) {
			if (is_uploaded_file($_FILES['plik']['tmp_name'])) {
					if(!file_exists($this->folder.'/'.$_FILES['plik']['name'])) {
						if(move_uploaded_file($_FILES['plik']['tmp_name'], $this->folder.'/'.$_FILES['plik']['name'])) {
								throw new Exception('Dodano plik pomyślnie');
					}
					else {
						throw new Exception('Nie dodano pliku!');
					}
				}
				else {
					throw new Exception('Taki plik już istnieje!');
				}
			}
		}
		else {
			throw new Exception('Niepoprawny typ!');
		}
	}
}
?>
[PHP] pobierz, plaintext 

index.php

[PHP] pobierz, plaintext 
<?php
 
include('class.upload.php');
 
try {
	$upload = new Upload('files');
}
catch (Exception $e) {
		echo $e->getMessage();
}
 
?>
[PHP] pobierz, plaintext 

Mam co do tego pytania :

1) Wiem że w stylu OOP są wyjątki. Czy to, jak ja je zastosowałem to odpowiednia metoda? Czy jest ich za dużo ? Bo mi nie pasują za bardzo ...
2) Czy wyjątki powinno się dawać do udanych operacji ?
3) Czy w konstruktorze wykonywać metodę 'upload' czy odwołać się do niej poza konstruktorem, już w pliku index?
4) Jeśli mam operacje na bazie danych bądź plikach tekstowych to czy tworzyć destruktor do ich zamykania ?

To takie ogólne pytania i byłbym bardzo wdzięczny gdyby ktoś mi to 'sprawdził', wytknął błędy i polecił dobrą literaturę

Dziękuję i pozdrawiam.

Tak na początek: najlepiej, jeśli klasa nie zwraca żadnego tekstu - to najlepiej wyłapywać w trakcie programu.

czyli rozumiem że wyłapywać w index? Ale nie bardzo rozumiem jak można to tam przechwycić

$_FILES['plik']['error'] - i już wiem że twoja klasa nie pomoże przy przesłaniu 2 plików lub pliku $_FILES['avatar'].
Echo w klasie uploadu ?!

Jeśli coś się udało czemu zgłaszasz wyjątek?


html w klasie upload?!

Na początek dwie ogólne uwagi:
1. Nie mieszaj języków (patrz: wywal polskie indeksy tablic). Angielski jest jedynym słusznym w takim miejscu.
2. Nigdy nie doprowadzaj do czegoś takiego:

[PHP] pobierz, plaintext 
if () {
	if () {
		if () {
			if () {
 
			} else {
 
			}
		} else {
 
		}
	} else {
 
	}
} else {
 
}
[PHP] pobierz, plaintext 

Tyle poziomów zagłębień to koszmar przy późniejszej pracy. Niemal zawsze da się to zamienić na coś w stylu:

[PHP] pobierz, plaintext 
// Obsługa czterech "błędów"
if (!...) {
 
}
 
if (!...) {
 
}
 
if (!...) {
 
}
 
if (!...) {
 
}
 
// Obsługa po pomyślnym przejściu przez powyższe
...
[PHP] pobierz, plaintext 

Pamiętaj, że wyrzucenie wyjątku przerywa wykonywanie bloku TRY.

Co do Twoich pytań:
1. Nie jest ich za dużo. W każdym miejscu gdzie występuje błąd, który wystąpić nie powinien masz prawo, a wręcz obowiązek skorzystać z wyjątku.
2. Nie.
3. Nie powinieneś. Konstruktor służy przygotowaniu obiektu do pracy, nie wykonywaniu samej pracy.
4. To zależy. Jeżeli plik jest otwarty na przestrzeni "działania obiektu" to w destruktorze dobrze byłoby upewnić się, że plik zostanie zamknięty. Przy czym raczej powinna być osobna metoda do zamknięcia, użytkownik powinien zawsze ręcznie ją wywoływać, a destrukor mógłby co najwyżej w ostateczności zamykać. Coś w ten deseń:

[PHP] pobierz, plaintext 
class File {
  private $opened = false;
 
  public function open(...) {
     ...
 
     $this->opened = true;
  }
 
  public function write(...) {
     ...
  }
 
  public function close() {
     ...
 
     $this->opened = false;
  }
 
  public function __descruct() {
    if ($this->opened) {
      $this->close();
    }
  }
}
[PHP] pobierz, plaintext 

I jeszcze kilka uwag:
1. Nazwa klasy powinna być rzeczownikiem, np. Uploader. Tworząc obiekt (new) tworzysz jakiś byt, a następnie każesz owemu bytowi wykonywać jakieś akcje, np. wgrać coś ($uploader->uploadSomething()) dlatego też nazwy metod powinny być czasownikami. Kod jest wtedy bardziej logiczny.
2. W PHP5 nie ma czegoś takiego jak "var". Masz trzy modyfikatory zasięgu: private, protected, public i to z nich powinieneś korzystać.
3. Nigdy nie powinieneś korzystać ze stanu globalnego (patrz $_POST, $_FILES). Te dane powinny zostać przekazane do obiektu z zewnątrz.
4. Akurat na kod błędu (1..7) masz ładne stałe, które są bardziej wymowne niż jakiś numerek.
5. Nigdy nie powinieneś mieć w kodzie takiej klasy żadnego ECHO. Jest błąd? To wywalasz wyjątek. Jakaś inna część aplikacji może go przechwycić i zrobić coś pożytecznego z nim (jak chcociażby wyświetlenie informacji użytkownikowi).
6. Nigdy nie stosuj bezpośrednio klasy Exception. Jest zbyt ogólna. Użyj bardziej wyspecjalizowanego typu. A jeżeli w SPL-u nie ma odpowiedniego, stwórz własny.
7. Jak już przechwytujesz wyjątek (blok CATCH) zrób z nim coś pożytecznego. Wyświetlenie treści wyjątku nie jest czymś takim. Co najwyżej całą aplikację możesz objąć blokiem TRY .. CATCH i tutaj wyświetlenie treści błędu ma sens - bo oznacza to, że aplikacja się wysypała i nic już nie da się zrobić.

Bardzo dziękuję za odpowiedzi i pomoc ! Crozin, Twoje uwagi okazały się bardzo pomocne. Zmieniłem nieco mój kod i obecnie wygląda on tak:

class.uploader.php

[PHP] pobierz, plaintext 
<?php
class Uploader {
	public $folder;
	public $types = array('application/msword', 'application/octet-stream', 'image/jpeg'); 
	public $file;
 
 
	function __construct($file, $folder) {
		$this->folder = $folder;
		$this->file = $file;
	}
 
	public function checkType() {
		if($_FILES[$this->file]['error']>0) {
			switch($_FILES[$this->file]['error']) {
				case 1: throw new Exception ("rozmiar pliku przekroczył wartość xMB."); break;
				case 2: throw new Exception ("rozmiar pliku przekroczył wartość xMB."); break;
				case 3: throw new Exception ("plik wysłany częściowo."); break;
				case 4: throw new Exception ("nie wysłano żadnego pliku."); break;
				case 6: throw new Exception ("nie można wysłać pliku: nie wskazano katalogu tymczasowego."); break;
				case 7: throw new Exception ("nie zapisano pliku na dysku."); break;
			}
		}
		if (!in_array($_FILES[$this->file]['type'], $this->types)) {
			return false;
		}
		else {
			return true;
		}
	}
 
	public function uploadFile() {
		if(!$this->checkType()) {
			throw new Exception('Niepoprawny typ pliku!');
		}
 
		if (!is_uploaded_file($_FILES[$this->file]['tmp_name'])) {
			throw new Exception('Nie wysłano pliku!');
		}
 
		if(file_exists($this->folder.'/'.$_FILES[$this->file]['name'])) {
			throw new Exception('Taki plik już istnieje');
		}
 
		if(!move_uploaded_file($_FILES[$this->file]['tmp_name'], $this->folder.'/'.$_FILES[$this->file]['name'])) {
			throw new Exception('Nie dodano pliku');
		}
 
		return true;
	}
}
?>
[PHP] pobierz, plaintext 

index.php

[PHP] pobierz, plaintext 
<?php
 
include('class.uploader.php');
 
try {
	$upload = new Uploader('file','files');
	if ($upload->uploadFile()) {
		echo 'Udało się wysłać plik!';
	}
}
catch (Exception $e) {
		echo $e->getMessage();
}
 
?>
[PHP] pobierz, plaintext 

Mam teraz kolejne pytanie, czy poprawnie został użyty kod :

[PHP] pobierz, plaintext 
if ($upload->uploadFile()) {
		echo 'Udało się wysłać plik!';
}
[PHP] pobierz, plaintext 

?

Jednak dalej nie wiem, jak zmienić ten główny blok try-catch. Ponadto, jak Crozin wspomniałeś, nie powinienem stosować tablic globalnych, tylko je przekazywać do obiektu. W jaki sposób (przykładowo) mogę przekazać do obiektu tablicę $_FILES?

@Spawnm:
Nie wiem czy o to Ci chodziło, ale teraz do konstruktora przekazuję nazwę $_FILES i jeśli jest to $_FILES['avatar'] to wystarczy że to do konstruktora przekażę. Czy źle zrozumiałem?
Ponadto pozbyłem się wyjątku w udanej operacji, oraz kodu html.

Dziękuję jeszcze raz za wszelką pomoc i proszę o kolejne uwagi.

Pozdrawiam.


P.S. Spawnm to Twoje 400 'Pomógł'. Gratulacje

Tak. Generalnie o to właśnie chodzi.

Jednak nadal pewne rzeczy są zrobione źle / w nienajlepszy sposób:
1. Zacznij korzystać z autoloaderów (jest wiele dobrych, gotowych rozwiązań).
2. Przykładaj większą uwagę do nazewnictwa. Nie ma to wpływu na działanie programu, ale zdecydowanie ułatwia korzystanie z niego. Zmienna $upload powinna raczej nazywać się $uploader w tym przypadku, $file (nazwa indeksu z tablicy $_FILES) jest dosyć myląca. W takim kontekście sugeruje ona raczej nazwę wgrywanego pliku czy coś takiego.
3. Jeżeli metoda checkType() jest wyłącznie do użytku wewnętrznego powinieneś ją oznaczyć jako niepubliczną.
4. Jak już wspomniałem: http://www.php.net/manual/en/features.file-upload.errors.php - to nieco oczyści kod.
5. Dlaczego metoda checkType(), której nazwa dosyć jasno wskazuje na to, że metoda sprawdza czy typ mime pliku jest odpowiedni zajmuje się jakimiś pierdołami w stylu sprawdzania błędów z $_FILES? To zadanie dla innej metody.
6.

[PHP] pobierz, plaintext 
if (!in_array($_FILES[$this->file]['type'], $this->types)) {
			return false;
		}
		else {
			return true;
		}
 
// VS
 
return !n_array($_FILES[$this->file]['type'], $this->types);
[PHP] pobierz, plaintext 

Komentarz chyba zbędny?
7. Nadal używasz bezpośrednio klasy Exception. Nie powinieneś tego robić.
8. $_FILES['...']['type'] nie jest wiarygodnym źródłem danych - może być dowolnie modyfikowane. W tej chwili nie ma problemu by wgrać przykładowo plik PHP przez ten skrypt.

Przykład ilustrujący o co chodziło:

[PHP] pobierz, plaintext 
<?php
 
try {
  // cała strona (cały system) tutaj
 
  try {
    $uploader = new Uploader( ... );
    if ( ... ) {
      ..
    }
  } catch (FileUploadException $fue) {
    $this->showMessage($fue->getMessage());
  } catch (IOException $ioe) {
    // jakaś obsługa błędu po stronie serwera
    // albo jak się nic nie da sensownego zrobić:
    throw $ioe;
  }
 
  // cała strona (cały system) tutaj
} catch (Exception $e) {
  die('Coś się posypało, błąd krytyczny!');
}
[PHP] pobierz, plaintext 

Tak jak masz obecnie nie jest najgorzej. Wątpię byś musiał się na chwilę obecną zamartwiać nad problemem uploadu pliku, który nie jest obsługiwany przez $_FILES, więc możesz zostawić jak jest.

Dziękuję, chyba jak na razie poprawiłem znacząco mój kod i wiem mniej więcej czym się kierować w dalszej nauce. Z Twoich wyliczeń zrobiłem prawie wszystko (oprócz punktu 1 i 8 ale o tym jeszcze doczytam / przemyślę). Dziękuję serdecznie za pomoc.