PHPhulp.nl
35k+ leden Over PHPhulp Offerte opdracht Contact
?

Feedback op mijn OOP script

Door Onbekende gebruiker op 02-02-2012 12:15 gewijzigd op 02-02-2012 12:19
1.181 views
Hallo allemaal,

Ik ben nieuw in de OOP hoek van PHP en heb iets geschreven wat voor mij wel werkt, maar weet niet of dit goed en op de juiste manier is.
Mijn school heeft mij wel wat over OOP verteld maar was allemaal erg onduidelijk.

Ik heb dit geschreven samen met behulp van een tutorial "http://net.tutsplus.com/tutorials/php/object-oriented-php-for-beginners/"
Zouden jullie mij tips e.d kunnen geven op dit script en ook zeggen wat ik "fout" doe of misschien onlogisch.

Geen afkraak dingen of modder gooien aub, een kind die net gaat schaatsen sta je ook niet ui te lachen op het ijs.


Alvast bedankt!

PHP CLASS:

<?php
	/**
	*	Menu functies en classes
	*
	*	Voor het installeren van het menu zijn een aantal
	*	checks en configuraties nodig. Deze checks worden
	*	op een "INSTALL" pagina gedaan en getoont. De daad-
	*	werkelijke checks worden dmv een class of functie
	*	uitgevoerd
	*	
	*	@author 		Rick de Graaff 
	*	@date			18-01-2012
	*	
	*/
	
	
	class checkModule {
		
		
		/**
		*	Vars voor de class
		*
		*	@param s_Module 	= modulenaam
		*	@param a_Tables 	= array met tabelnamen
		*	@param a_Files 		= array met bestandsnamen
		*	@param a_Folders 	= array met mapnamen
		*	@param i_Integer 	= teller zodat elke tabel een eigen nummer krijgt (tijdelijk nummer)
		*	@param a_Errors 	= array waar errors in kunnen staan (deze gebruik ik nog niet)
		*	@param s_Debug 		= Als de debug aanstaat word er bij de __destruct een printr getoont met de errors
		*
		*/
		
		public $s_Module;
		public $a_Tables;
		public $a_Files;
		public $a_Folders;
		public $a_Errors;
		public $s_Debug;
		
		
		
		/* Constructor */
		public function __construct($sModule, $aTables, $aFiles, $aFolders, $sDebug){
			$this->s_Module		= $sModule;
			$this->a_Tables		= $aTables;
			$this->a_Files		= $aFiles;
			$this->a_Folders	= $aFolders;
			$this->a_Errors		= array();
			$this->s_Debug		= $sDebug;
			
			// Laat zien om wat voor module het gaat
			echo '<h2>MODULE '.strtoupper($this->s_Module).'</h2>'."\n";
			echo '<p>Kijk welke tabellen en/of de module al ge&iuml;nstalleerd staat. Alle tabellen zijn nodig in een module.<br />Vink anders aan wat je wilt installeren.</p>'."\n";
			echo '<div class="background">'."\n\n";
			
			echo '<h3>Module actief</h3>'."\n";
		}
		
		/* Destructor */
		public function __destruct(){
			
			
			// Sluit de background div
			echo '</div>'."\n\n";
			
			if($this->s_Debug == true){
				printr($this->a_Errors);
			}
		}
		
		
		/**
		*	Controleren of module bestaat
		*
		*/
		
		public function moduleCheck(){
			
			// Start van de UL LIST
			echo '<div class="formlist-nodrag">'."\n";
			echo '<ul>'."\n";
			
			
			
				$qCheckInstalled	= 'SELECT * FROM modules WHERE moduleNaam = "'.$this->s_Module.'" LIMIT 1';
				$mCheckInstalled	= mysql_query($qCheckInstalled) or die($sError = '<div><strong>qCheckInstalled error: </strong><br />'.$qCheckInstalled.'<br /><pre></div>'.mysql_error().'</pre>');
				$fCheckInstalled	= mysql_fetch_array($mCheckInstalled);
				$cCheckInstalled	= mysql_num_rows($mCheckInstalled);
				
				// Parse voor de mysql error
				if(!$mCheckInstalled){ echo $sError;}
				
				// Melding of de module al geïnstalleerd is
				if($cCheckInstalled >= 1){
					echo '<li><img src="images/icons/silk/accept.png" alt="Wel ge&iuml;nstalleerd" title="Wel ge&iuml;nstalleerd" /> Module '.$this->s_Module.' is actief</li>';
					
				}	elseif($cCheckInstalled == 0){
						echo '<li><img src="images/icons/silk/exclamation.png" alt="Nog niet ge&iuml;nstalleerd" title="Nog niet ge&iuml;nstalleerd" /> Module '.$this->s_Module.' is nog <strong>niet</strong> ge&iuml;nstalleerd <span><input type="checkbox" name="'.stripStr(strtolower($this->s_Module)).'" /></span></li>';
						
						// Error aanmaken in de "error array"
					}
					
					
					
			// Einde van de UL LIST
			echo '</ul>'."\n";
			echo '</div>'."\n\n";
		}
		
		
		
		/**
		*	Functies en uitvoeringen voor tabels checks
		*
		*/
		
		public function tablesCheck(){
			
			// Start van de UL LIST
			echo '<br /><h3>Tabellen</h3>'."\n";
			echo '<div class="formlist-nodrag">'."\n";
			echo '<ul>'."\n";
				
				
				/* Doorloop alle tables die in de array staan */
				foreach($this->a_Tables as $sTable){
					
					$qCheckTables		= 'SHOW TABLES LIKE "'.$sTable.'"';
					$mCheckTables		= mysql_query($qCheckTables) or die($sError = '<div><strong>qCheckTables error: </strong><br />'.$qCheckTables.'<br /><pre></div>'.mysql_error().'</pre>');
					$fCheckTables		= mysql_fetch_array($mCheckTables);
					
						if($fCheckTables[0] == $sTable){
							echo '<li><img src="images/icons/silk/database.png" alt="Wel ge&iuml;nstalleerd" title="Wel ge&iuml;nstalleerd" />  Tabel '.$sTable.' bestaat al</li>';
							
						} 	else {
								echo '<li><img src="images/icons/silk/database_error.png" alt="Nog niet ge&iuml;nstalleerd" title="Nog niet ge&iuml;nstalleerd" />  Tabel '.$sTable.' is niet aangemaakt <span><input type="checkbox" name="'.stripStr(strtolower($sTable)).'" /></span></li>';
							}
						
					// Parse voor de mysql error
					if(!$mCheckInstalled){ echo $sError;}
				}
				
				
			// Einde van de UL LIST
			echo '</ul>'."\n";
			echo '</div>'."\n\n";
		}
		
		/**
		*	Mappen check
		*
		*	Check of de mappen die nodig zijn voor deze module aanwezig zijn
		*/
		
		public function foldersCheck(){
			
			// Start van de UL LIST
			echo '<br /><h3>Mappen</h3>'."\n";
			echo '<div class="formlist-nodrag">'."\n";
			echo '<ul>'."\n";
			
			
				/* Waardes 1 voor 1 uit de array halen en controleren */
				foreach($this->a_Folders as $sIndexKey => $sFolder){
					
					if(is_dir($sFolder)){
						echo '<li><img src="images/icons/silk/folder.png" alt="Folder bestaat" title="Folder bestaat" />  Map "'.$sIndexKey.'" bestaat al</li>';
						
					} 	elseif(!is_dir($sFolder)){
							echo '<li><img src="images/icons/silk/folder_error.png" alt="Folder niet aanwezig" title="Folder niet aanwezig" />  Map "'.$sIndexKey.'" is niet aangemaakt <span><input type="checkbox" name="'.stripStr(strtolower($sIndexKey)).'" /></span></li>';
						}
				}
				
				
			// Einde van de UL LIST
			echo '</ul>'."\n";
			echo '</div>'."\n\n";
		}
		
		/**
		*	Bestands check
		*
		*	Een array die word uigelezen en checkt of de bestanden wel aanwezig zijn
		*/
		
		public function filesCheck(){
		
			// Start van de UL LIST
			echo '<br /><h3>Bestanden</h3>'."\n";
			echo '<div class="formlist-nodrag">'."\n";
			echo '<ul>'."\n";
				
				/* Waardes 1 voor 1 uit de array halen en controleren */
				foreach($this->a_Files as $sIndexKey => $sFile){
					
					if(file_exists($sFile)){
						echo '<li><img src="images/icons/silk/script.png" alt="Bestand bestaat" title="Bestand bestaat" />  Bestand "'.$sIndexKey.'" bestaat al</li>';
						
					} 	elseif(!file_exists($sFile)){
							echo '<li><img src="images/icons/silk/script_error.png" alt="Bestand niet aanwezig" title="Bestand niet aanwezig" />  Bestand "'.$sIndexKey.'" is niet aangemaakt <span><input type="checkbox" name="'.stripStr($sIndexKey).'" /></span></li>';
						}
				}
				
				
			// Einde van de UL LIST
			echo '</ul>'."\n";
			echo '</div>'."\n\n";
			
		}
	}
?>



PHP CODE OM CLASS AAN TE ROEPEN:


<?php
				/*
				*	MODULE CHECKEN
				*	
				*	Bekijk of alle bestanden en tabellen juist zijn en ook bestaan.
				*	Als deze niet bestaan krijg je een nette melding en kan je ze
				*	alsnog laten maken door het CMS
				*	
				*	@param, alle variabelen staan hieronder die nodig zijn om alles
				*	te controleren voor de module (de inhoud van de variabelen verschilt per module).
				*	
				*	Opbouw module check:
				*	
				*	- Array met Tabellen
				*	- Array met Bestanden
				*	- Array met Mappen
				*	- Variabele met "module naam"
				*	
				*	
				*	
				*/
				
				$aTables	= array('menu', 'menu_talen', 'jaja');
				$aFiles		= array(".htaccess" => ''.$sExtSub.'.htaccess', ".index.asp" => ''.$sExtSub.'.index.asp');
				$aFolders	= array("pages" => ''.$sExtSub.'pages/', "bla" => ''.$sExtSub.'bla/',);
				$sModule	= 'menu';
				
				// Nieuwe module check aanmaken
				$sMenu		= new checkModule($sModule, $aTables, $aFiles, $aFolders, false);
				
				// Controleer of module actief staat in de DB
				echo $sMenu->moduleCheck();
				
				// Controle of de tabellen wel aanwezig zijn die nodig zijn
				echo $sMenu->tablesCheck();				
				
				// Controle of de tabellen wel aanwezig zijn die nodig zijn
				echo $sMenu->foldersCheck();
				
				// Controle of de tabellen wel aanwezig zijn die nodig zijn
				echo $sMenu->filesCheck();
				
				// Sluit de class af en check dmv de destructor voor errors
				unset($sMenu);
			?>
P
Pieter Jansen
02-02-2012 12:17 gewijzigd op 02-02-2012 12:20
Ik zie een hele grote fout en dat is dat je echo`s gebruikt in je klasse, dat is juist niet goed. Wat je gebruikt is de return statement in PHP. Geef je strings terug kun je in externe bestanden aanroepen. Eigenlijk zoals je $sMenu->moduleCheck(); doet.

[edit]
ter aanvulling, objecten gebruik je om de logica te onderscheiden van je visuele representatie. Er zijn uitzonderingen hier op, maar eigenlijk hoor je die te negeren. Je moet er altijd voor zorgen dat objecten logic doen, en view files ( templates etc ) de representatie.

Ik zie ook dat je hele menu structuren hanteert in je object, dat zou ik niet doen. Dat zou ik in een template bestand gooien en je object dus gewoon een array laten terug geven. Vervolgens (recursief) door je array lopen en dat als output genereren.
[/edit]
?
Onbekende gebruiker
02-02-2012 12:22
Als ik de echo's vervang door return's dan krijg ik niets terug.. Hier stuitte ik al op en daarom gebruikte ik de echo maar.

Er zitten while loops in de functies, ik denk dat dit daardoor komt.

Oplossing? Alles parsen naar een variabele (binnen de functie) en dan parsen?

[size=xsmall]Toevoeging op 02/02/2012 12:23:35:[/size]

Das wel een hele goeie oplossing ja. Een template laden en daar de waardes in laten plaatsen.

(y) Hier heb ik wat aan! :D

Kijk het werkt wel, maar wist niet of ik het goed deed..
Hartstikke bedankt!

Meerdere feedbacks zijn welkom hoor! Lees ze allemaal :)
P
Pieter Jansen
02-02-2012 12:30 gewijzigd op 02-02-2012 12:30
Hehe goed zo. Je zou het inderdaad op kunnen slaan in een tijdelijke var, dat gebeurt redelijk veel. Maar ik zou verder geen html willen parsen in een object. Dat is namelijk representatie en geen logic. Zoiets bijv:


<?php

public function filesCheck(){
        $a = array();
	/* Waardes 1 voor 1 uit de array halen en controleren */
        foreach($this->a_Files as $sIndexKey => $sFile){
        	
		if(file_exists($sFile)){
                       $a[$sKindexKey] = array($sFile, true); 
               	} elseif(!file_exists($sFile)){
			$a[$sKindexKey] = array($sFile, false);
                }
       	}
	return $a;
}

?>
?
Onbekende gebruiker
02-02-2012 12:31
Ja ik vatte hem. had het eerst ook niet, maar toen had ik in het "oproepen" van de class weer heel veel HTML staan.. :\

Maar wat jij zegt met een template laden, is 100x makkelijker en fijner..

Gaat helemaal goed komen!

En bedankt voor je voorbeeld scriptje
P
Pieter Jansen
02-02-2012 12:35
Mooi zo en geen dank. Succes er mee! OO levert heel wat voordelen op t.o.v. normale code, het kost alleen wat meer tijd om ze op te zetten. Maar eenmaal goedwerkende klasses tot je beschikking hebbende, kun je veel sneller projecten starten, klasse inladen en je hebt direct extra functionaliteit.
TJVB tvb
02-02-2012 15:07
Er hoort geen echo in je functies, alleen een return.
Een constructor heeft geen return (je krijgt het object al terug) maar daarin hoor je verder ook geen echo te plaatsen. Bij de andere methode moet als je een tekst terug geeft die met echo wel te laten zien. Anders gaat er iets anders fout.

Dan verder over je code, als je het commentaar direct boven je functie zet (i.p.v.) een lege regel er tussen kan het opgepakt worden door tools als doxygen om je documentatie te genereren.

Kijk ook even naar je inspringen, je spring wel heel snel in. (Naar mijn mening)

Leuk dat je tijd wilt steken in OOP en dat hier duidelijk toont. Daar kunnen anderen veel van leren. Over een jaar wil je niet anders ;)

Reageren

Inloggen om te reageren