Codeoptimierung - wiederkehrende Elemente

[jobo]

Hallo,

in meiner Search-Klasse habe ich zwei private Funktionen, die zu teilen im Code identisch sind:

  
	// sucht aus einer Datei alle Zeilen, in denen die übergebenen Wert in der Spalte zutriffen  
	private static function _getRowsByColsValue($tablePath, $colsHash) {  
		$fileHandle = @fopen($tablePath,"r");  
		$rows = array();  
		$firstRow = false;  
		if ($fileHandle) {  
			while (($row = fgetcsv($fileHandle)) !== false) {  
				//exclude empty lines  
				if (!is_array($row)) {  
					continue;  
				}  
				// Whitespaces/Leerzeichen am Anfang und Ende einer jeden Zelle entfernen  
				// keine Leerzeichen überhaupt in einer Zeile  
				if(trim(implode($row))) {  
					$row = array_map("trim",$row);  
					if($firstRow === false) {  
						$firstRowFlipped = array_flip($row);  
						$firstRow = $row;  
					} else {  
						$filter = true;  
						foreach($colsHash as $colName => $searchValue) {  
							if($row[$firstRowFlipped[$colName]] != $searchValue) {  
								$filter = false;  
							}							  
						}  
						if($filter) {  
							foreach ($row as $colNr => $cellValue) {  
								$rowAssoc[$firstRow[$colNr]] = $cellValue;  
							}  
							$rows[] = $rowAssoc;  
						}  
					}				  
				}  
			}	  
			// merci handle, go free  
			fclose($fileHandle);  
		}  
		self::$_rowsByColsValue = array_merge(self::$_rowsByColsValue, $rows);  
	}	  

Die Funktion öffnet eine CSV-Datei zum Lesen, durchläuft die Zeilen, kickt Zeilen mit nur Whitespaces raus, merkt sich dann, wann die erste verwertbare Zeile kommt, registriert diese als "erste Zeile" (weil sich daraus der Spaltentitel ergibt) und sucht dann in den weiteren Zeilen, das ist der "individuelle Teil", ob Zeilen mit bestimmten Spaltenwerten vorhanden sind und trägt diese dann ins Ausgabearray ein.

Die Funktion, die alle möglichen Werte einer Spalte in eine Liste füllt, ist im oberen Teil ident (Filehandle, while-Schleife, trimmen und kicken der Leerzeilen, merken der ersten Zeile). Erst ab da differiert sie:

  
	private static function _getValuesPerColumn($tablePath) {  
		$fileHandle = @fopen($tablePath,"r");  
		$rows = array();  
		$firstRow = false;  
		if ($fileHandle) {  
			while (($row = fgetcsv($fileHandle)) !== false) {  
				//exclude empty lines  
				if (!is_array($row)) {  
					continue;  
				}  
				// Whitespaces/Leerzeichen am Anfang und Ende einer jeden Zelle entfernen  
				// keine Leerzeichen überhaupt in einer Zeile  
				if(trim(implode($row))) {  
					$row = array_map("trim",$row);  
					if($firstRow === false) {  
						$firstRow = $row;  
					} else {  
						foreach($row as $colNr => $cellValue) {  
							if($firstRow[$colNr] != "Text" && $cellValue != "") {  
								self::$_possibleValuesPerColumn[$firstRow[$colNr]][$cellValue] = $cellValue;  
							}							  
						}  
					}				  
				}  
			}	  
			// merci handle, go free  
			fclose($fileHandle);  
		}  
	}  

1. Diesen doppelten (gecopied und gepasteten) Teil, kann ich ja nicht wirklich in eine Funktion auslagern. Oder?

2. Die Funktion _getValuePerColumn - die mir alle möglichen Werte eine bestimmten Zeile liefert, wird auf der Suchseite immer aufgerufen, weil damit die Selectbox befüllt wird. Für die View habe ich also:
$possibleValuesPerColumn = Search::getAllPossibleValuesPerColumn();

Wenn das Formular abgeschickt wurde, habe ich zusätlich noch

$possibleValuesPerColumn  Search::getRowsByColsValue($suchWerte);  

Schön wäre natürlich, wenn die Funktion "_getAllPossibleValuesPerColumn" beim Durchlaufen der CSV-Datei(en) gleich die passenden gesuchten Zeilen mitliefert, die mit getRowsByColsValue() ausgegeben werden - wenn denn das Formular abgeschickt wurde.

Andereseits ist es wohl auch eher unübersichtlich, wenn dann alles in einer "Monsterfunktion" steckt, der ich per Parameter noch mitgeben muss, ob nun Suchzeilen mit ausgegeben werden sollen oder nicht, um dann wohlmöglich als Rückgabe noch ein Hash-of-Hashes zu haben mit:

  
$possibleValuesPerColumn_AND_searchedRows = Search::getAllPossibleValuesPerColumnAndRowByColsValue($suchWerte);  
$possibleValuesPerColumn = $possibleValuesPerColumn_AND_searchedRows["possibleValuesPerColumn"];  
$row = $possibleValuesPerColumn_AND_searchedRows["rows"];  

Vielleicht ists auch manchmal einfach einfacher, mit redundantem Code zu leben?

Gruß

jobo

[jobo]

Hallo,

// sucht aus einer Datei alle Zeilen, in denen die übergebenen Wert in der Spalte zutriffen
private static function _getRowsByColsValue($tablePath, $colsHash) {
$fileHandle = @fopen($tablePath,"r");
$rows = array();
$firstRow = false;
if ($fileHandle) {
while (($row = fgetcsv($fileHandle)) !== false) {
//exclude empty lines
if (!is_array($row)) {
continue;
}
// Whitespaces/Leerzeichen am Anfang und Ende einer jeden Zelle entfernen
// keine Leerzeichen überhaupt in einer Zeile
if(trim(implode($row))) {
$row = array_map("trim",$row);
if($firstRow === false) {
$firstRowFlipped = array_flip($row);
$firstRow = $row;
}

  
Ich habe jetzt diesen Teil, der in beiden Funktionen ident ist, in den Konstruktor gepackt bzw. noch eine Unterfunktion \_setFirstLine() gebaut.  
  
Jetzt habe ich zwar einiges mehr an Klassenvariablen und auch an Unterfunktionen, dafür ist das ganze aber irgendwie sauberer aufgeteilt, so dass man über die statschen getter bzw. eine (noch) private \_init() funktion und einen Parameter im Konstruktor steuern kann, ob bei der Iteration gleich mit nach Zeilen gesucht werden soll oder nicht.  
  
Wens interessiert hier mal die komplette Klasse (da ich nicht weiß, wie sinnvoll jetzt Auszüge präsentieren):  
~~~php
  
<?php  
class SearchRefactored  
{  
	  
	private $_fileHandle = NULL;  
	private $_firstRow = false;  
	private $_firstRowFlipped = false;  
	private $_actualTablePath = NULL;  
	private $_actualRow = NULL;  
  
	private static $_possibleValuesPerColumn = array();  
	private static $_rowsByColsValue = array();  
	  
	private static function _getTablesList() {  
		$tablesList = array();  
		return glob($GLOBALS["dataDir"] . "*.csv");  
	}  
	  
	private function __construct($tablePath, $colsHash = false) {  
		$this->_fileHandle = @fopen($tablePath,"r");  
		if ($this->_fileHandle) {  
			  
			$this->_setFirstRow();  
			  
			// if there is a search, then we need the columncount by columnname  
			if (is_array($colsHash)) {  
				$this->_firstRowFlipped = array_flip($this->_firstRow);  
			}		  
			  
			while (($row = fgetcsv($this->_fileHandle)) !== false) {  
				//exclude empty lines  
				if (!$this->_validRow($row)) {  
					continue;  
				}  
				$this->_actualRow = array_map("trim",$row);  
				  
				$this->_setPossibleValuesPerColumn();  
				  
				if (is_array($colsHash)) {  
					$this->_firstRowFlipped = array_flip($this->_firstRow);  
					$this->_setRowsByColsValue($colsHash);  
				}  
			}  
			// merci handle, go free  
			fclose($this->_fileHandle);  
		}	else {  
			throw new Exception ("No Database named " . $tablePath . " found.");  
		}  
	}  
	  
	//sets first Row which contains column titles  
	private function _setFirstRow() {  
		while ($this->_firstRow === false) {  
			$row = fgetcsv($this->_fileHandle);  
			//exclude empty lines  
			if (!$this->_validRow($row)) {  
				continue;  
			}  
			// Whitespaces/Leerzeichen am Anfang und Ende einer jeden Zelle entfernen  
			// keine Leerzeichen überhaupt in einer Zeile  
			$row = array_map("trim",$row);  
			$this->_firstRow = $row;  
		}  
	}  
	  
	// checks if row ist empty  
	private function _validRow($row) {  
		if (is_array($row) && trim(implode($row)) != "") {  
			return true;  
		}  
		return false;  
	}  
	  
	private function _setPossibleValuesPerColumn() {  
		foreach($this->_actualRow as $colNr => $cellValue) {  
			if($this->_firstRow[$colNr] != "Text" && $cellValue != "") {  
				self::$_possibleValuesPerColumn[$this->_firstRow[$colNr]][$cellValue] = $cellValue;  
			}							  
		}  
	}  
	  
	// sucht aus einer Datei alle Zeilen, in denen die übergebenen Wert in der Spalte zutriffen  
	private function _setRowsByColsValue($colsHash) {  
		$filter = true;  
		// walk trough cols hash, which contains $colsHash["colname"] = "searchvalue"  
		foreach($colsHash as $colName => $searchValue) {  
			if($this->_actualRow[$this->_firstRowFlipped[$colName]] != $searchValue) {  
				$filter = false;  
			}							  
		}  
		if($filter) {  
			foreach ($this->_actualRow as $colNr => $cellValue) {  
				$rowAssoc[$this->_firstRow[$colNr]] = $cellValue;  
			}  
			self::$_rowsByColsValue[] = $rowAssoc;  
		}  
	}	  
	  
	private static function _init($colsHash = false) {  
		foreach(self::_getTablesList() as $tablePath) {  
			$csvTable = new self($tablePath, $colsHash);  
		}  
	}  
	  
	// auch in der Suche PHP benutzt  
	public static function getAllPossibleValuesPerColumn() {  
		self::_init();  
		foreach (self::$_possibleValuesPerColumn as $colName => $valueList) {  
			ksort(self::$_possibleValuesPerColumn[$colName]);  
		}  
		return self::$_possibleValuesPerColumn;  
	}  
  
	// aufgerufen aus der suche.php  
	public static function getRowsByColsValue($colsHash) {  
		self::_init($colsHash);  
		return self::$_rowsByColsValue;  
	}  
}  

Gruß

jobo

[Texter mit x]

1. Diesen doppelten (gecopied und gepasteten) Teil, kann ich ja nicht wirklich in eine Funktion auslagern. Oder?

Ohne jetzt länger als 10 Sekunden drüber nachgedacht zu haben, in manchen Programmiersprachen kann man eine Bezeichnung definieren und das was die Bezeichnung im Code ersetzen soll, in der Regel Konstanten. Außer der Regel kann man aber auch Code als Ersatz angeben. Ich benutze das z.B. gern bei der Bearbeitung von Bitmustern.

#define schleudersitz_ausgeloest status = status OR 10000b '

Wie mir nun aufging, geht es hier wohl konkret um php aber ich schicke den Beitrag trotzdem mal ab.

[jobo]

Hallo und Danke für die (einzige) Anwort,

Ohne jetzt länger als 10 Sekunden drüber nachgedacht zu haben, in manchen Programmiersprachen kann man eine Bezeichnung definieren und das was die Bezeichnung im Code ersetzen soll, in der Regel Konstanten. Außer der Regel kann man aber auch Code als Ersatz angeben. Ich benutze das z.B. gern bei der Bearbeitung von Bitmustern.

#define schleudersitz_ausgeloest status = status OR 10000b '

Wie mir nun aufging, geht es hier wohl konkret um php aber ich schicke den Beitrag trotzdem mal ab.

Es geht ja auch um die Grundlage, das zu strukturieren. Nur 10 Codezeilen sind ja in einer Konstanten auch komisch untergebracht. Der Denkansatz war eigentlich flasch. Im Grunde hätte so auch ein Goto geholfen (;-). Und Goto ist ja "böse". Der Konstruktor eröffnet jetzt das Dateihandle, und startet dann die verarbeitenden Funktionen, mit einer Weiche, ob gleich Sucheregbnisse mitgenommen werden sollen oder nicht.

Gruß

jobo