[php]Początki OOP Opcje

[Mefiuu]

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ę (IMG:style_emoticons/default/smile.gif)

Dziękuję i pozdrawiam.

Ten post edytował Mefiuu 3.08.2011, 20:46:10

[Crozin]

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

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.

Jednak dalej nie wiem, jak zmienić ten główny blok try-catch.

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 

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?

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.