Testklasse korrekt?

Hallo!

Ich habe mich mal mit OOP beschäftigt und mir gleich eine Klasse geschrieben wie man sie in einem CMS vorfinden könnte. Jetzt wollte ich wissen ob diese OOP technisch korrekt aufgebaut ist d.h. ob der Konstruktor korrekt verwendet wird etc.

<?php
	class CMSystem {

		/**
		 * The name of the current module
		 *
		 * @var String
		 */
		private $module;

		/**
		 * True if the module is the default module
		 *
		 * @var Boolean
		 */
		private $isDefaultModule;

		/**
		 * The default module
		 *
		 * @var String
		 */
		private $defaultModule;

		/**
		 * True if the user has the permission to access the current page
		 *
		 * @var Boolean
		 */
		private $hasPermission;

		/**
		 * Contains informations like the username ...
		 *
		 * @var Array
		 */
		public $user;

		function __construct($module, $defaultModule) {

			$this->module = $module;
			$this->module = str_replace('..','',$this->module);
			$this->module = "./modules/{$this->module}/index.php";

			$this->defaultModule = $defaultModule;

			if (strcmp($this->module,$this->defaultModule))
				$this->isDefaultModule = true;
			else 
				$this->isDefaultModule = false;

			$this->user['userId'] = $_SESSION['CMSUser']['userId'];
			$this->user['loggedIn'] = $_SESSION['CMSUser']['loggedIn'];
			$this->user['username'] = $_SESSION['CMSUser']['username'];

			if ($this->user['loggedIn'] === true) {

				if ($this->isDefaultModule === true)
					$this->hasPermission = true;
				else if ($_SESSION['CMSUser']['permission'] == 'global')
					$this->hasPermission = true;
				else if (is_array($_SESSION['CMSUser']['permission'])) {

					// Authentification
					#$this->hasPermission = true;
					# OR
					# $this->hasPermission = false;
				}
				else 
					$this->hasPermission = false;
			}

		}

		/**
		 * Checks if the user has the permission to access the current page
		 *
		 */
		public function checkPermission() {

			if ($this->hasPermission === false) {
				$this->redirect('index.php?logout=true');
				exit();
			}
		}

		/**
		 * Loads any module of the cm system
		 *
		 */
		public function loadModule() {

			if (@file_exists($this->module))
				require_once ($this->module);
			else 
				require_once ($this->defaultModule);
		}
	}
?>

Reyx

Auf den ersten Blick fällt mir eigentlich nur auf, dass die Funktion CMSystem::redirect nicht existiert.
Zudem solltest du evtl. CMSystem als private deklarieren und dann eine Zugriffsfunktion dafür schreiben, sonst kann dir da jedes beliebige Plugin deine Daten verändern.

Beachte außerdem, dass sämtliche Dateien, die du in CMSystem::loadModul einbindest, direkt in der Funktion geparst werden. Du musst also sämtliche globalen Daten (Variablen etc.) vorher mit global verfügbar machen, sonst hast du in den eingebunden Dateien nur Zugriff auf die superglobalen Array und Konstanten.

Und bei "if (@file_exists($this->module))" in der Funktion CMSystem::loadModul ist das @ nun wirklich überflüssig

Ansonsten sehe ich da vom Prinzip her keine großen Probleme (hab den Code jetzt aber auch nur übeflogen). Allerdings ist dein Stil ziemlich schwammig.

Ja die redirect methode existiert nicht, das weiß ich. Was heißt schwammiger Stil?
Das Problem mit den Gültigkeitsbereichen ist mir bekannt - stört aber nicht weiter.

Was meinst du mit CMSystem als private? Kann man denn eine Klasse als private deklarieren?

und vielen Dank für die Kritik!

MfG

Reyx

Sorry, da hat meine Tastatur wohl nicht ganz mit gespielt, es sollte CMSystem::User heißen!

Mit schwammiger Stil meine ich ... mh ja, was eigentlich?
Schau dir mal deine Kommentare an, dann weißt du was ich meine.

Allerdings, wenn du schon deine Klassenvariablen penetrant als public, private oder protected deklarierst, solltest du von diesem Verhalten bei Funktionen nicht abweichen!

Das mit dem Programmierstil verstehe ich noch immer nicht aber ist erstmal egal.

es sollte CMSystem::User heißen!

Das verstehe ich nicht ganz. Du willst eine Methode die User heißt und was macht diese Methode?

Allerdings, wenn du schon deine Klassenvariablen penetrant als public, private oder protected deklarierst, solltest du von diesem Verhalten bei Funktionen nicht abweichen!

Wo weiche ich davon ab (außer beim Konstruktor)? Aber es ist klar, dass dieser public ist, weil er ansonsten keine Parameter haben dürfte.

Die anderen methoden müssen public sein.

Reyx

OOPNoob schrieb:

Das verstehe ich nicht ganz. Du willst eine Methode die User heißt und was macht diese Methode?

Keine Methode, sondern die Variable CMSystem::user. Diese hast du als public deklariert, so dass dir jedes Plugin (oder jeder andere Quellcode, der irgendwie in deinen kommen könnte) die darin enthaltenen Werte nach belieben verändern könnte.

OOPNoob schrieb:

Wo weiche ich davon ab (außer beim Konstruktor)? Aber es ist klar, dass dieser public ist, weil er ansonsten keine Parameter haben dürfte.

Es Verwirrt nur, wenn du offene Variablen explizit als public deklarierst, manche Funktionen (bzw. in diesem Fall nur den Konstruktor) aber nicht. Das ist kein Problem sondern nur uneinheitlicher Stil ... Wem's gefällt ...

oh ja das war mein Fehler. Ich muss user als private machen und eine Methode getUser implementieren. thx

Mfg