// der php hacker

// archiv

Workshop: Brawler – Code Review, Teil 5

Geschrieben am 11. Dez 2009 von Cem Derin

Zuvor:

  1. Workshop: Brawler – The Web Application Security Scanner, Teil 0
  2. Workshop: Brawler – Eine Frage der Lizenz, Teil 1
  3. Workshop: Brawler – Alles im Rahmen, Teil 2
  4. Workshop: Brawler – Not unplugged, Teil 3
  5. Workshop: Brawler – Der Rutengänger, Teil 4

Ich hab zwar beim letzten mal gesagt, dass ich die Plugin-Schnittstelle implementieren werde sowie die ersten Zeilen für das Scanning schreibe, doch beim durchsehen des Codes fielen mir schon so viele Stellen ins Auge, die ein Code Review mit einhergehendem Refactoring unumgänglich machen. Einige davon kamen sogar von Lesern. Vielen Dank an dieser Stelle.

Ein paar Worte zum Vorgehen: Ich gehe Datei für Datei und Zeile für Zeile durch. Ich werde hier allerdings nur Änderungen kommentieren.

Die erste Stelle, die mir unangenehm ins Auge stach (und auch schon beim schreiben irgendwie unbehaglich war) ist die _checkRoute-Funktion des Routers. Diese sieht derzeit so aus:

		/**
		 * Checks a route for matching conditions
		 *
		 * @param Brawler_Router_Route $route
		 * @return Bool
		 */
		protected static function _checkRoute(Brawler_Router_Route $route) {
			// check arguments
			$argumentIterator = $route->getArguments()->getIterator();
			$match = true;
			while($argumentIterator->valid()) {
				if(!Brawler_Console::getArgument($argumentIterator->current()->getFlag())) {
					// flag is missing break;
					$match = false;
					break;
				}

				if($argumentIterator->current()->getOnValue()) {
					if(!Brawler_Console::getArgument($argumentIterator->current()->getFlag())->getValue()) {
						// value is missing, break
						$match = false;
						break;
					}

					if($argumentIterator->current()->getSpecificValue()) {
						$specValue = $argumentIterator->current()->getSpecificValue();
						if(Brawler_Console::getArgument($argumentIterator->current()->getFlag())->getValue() != $specValue) {
							// value does not match, break
							$match = false;
							break;
						}
					}
				}
				$argumentIterator->next();
			}

			// dispatch
			if($match) {
				$route->getController()->dispatch($route->getAction());
			} else {
				return true;
			}

			// check chain
			return $route->getChain();
		}

Ich finde, hier wird ein bisschen zu viel abgehandelt. Das wirkt zwar alles so, als gehöre es auch zusammen, es tut aber zum einen nicht weh, das aufzutrennen, und zum anderen bleibe ich so schön flexibel! Daher werden wir zwei neue Methoden einführen: _checkFlag, _checkValue und _checkSpecificValue. Die _checkRoute Methode sieht danach so aus:

		/**
		 * Checks a route for matching conditions
		 *
		 * @param Brawler_Router_Route $route
		 * @return Bool
		 */
		protected static function _checkRoute(Brawler_Router_Route $route) {
			// check arguments
			$argumentIterator = $route->getArguments()->getIterator();
			$match = true;

			while($argumentIterator->valid()) {
				$match = self::_checkFlag($argumentIterator->current());
					if(!$match) break;

				$match = self::_checkValue($argumentIterator->current());
					if(!$match) break;

				$match = self::_checkSpecificValue($argumentIterator->current());
					if(!$match) break;

				$argumentIterator->next();
			}

			// dispatch
			if($match) {
				$route->getController()->dispatch($route->getAction());
			} else {
				return true;
			}

			// check chain
			return $route->getChain();
		}

Eine ganze Ecke schlanker also. Dazu kommen die drei neuen Methoden:

		/**
		 * Checks whether a route argument is present in call request
		 *
		 * @param Brawler_Router_Argument $routeArgument
		 * @return Bool
		 */
		protected static function _checkFlag($routeArgument) {
			return Brawler_Console::getArgument($routeArgument->getFlag());
		}

		/**
		 * Checks whether a route arguments value is present
		 *
		 * @param Brawler_Router_Argument $routeArgument
		 * @return Bool
		 */
		protected static function _checkValue($routeArgument) {
			if($routeArgument->getOnValue()) {
				$argument = Brawler_Console::getArgument($routeArgument->getFlag());
				if(!$argument OR !$argument->getValue()) {
					return false;
				}
			}

			return true;
		}

		/**
		 * Checks whether a route arguments vakue matches to the required
		 *
		 * @param Brawler_Router_Argument $routeArgument
		 * @return Bool
		 */
		protected static function _checkSpecificValue($routeArgument) {
			if($routeArgument->getSpecificValue()) {
				if(self::_checkValue($routeArgument)) {
					$argument = Brawler_Console::getArgument($routeArgument->getFlag());
					return ($argument->getValue() == $routeArgument->getSpecificValue());
				}
			}

			return true;
		}

Ebenfalls alle schön schlank. Besser. Weiter zu den nächsten Verbesserungsbedürftigen Stellen. Und lange suchen muss ich auch nicht. In der forward-Methode der Controller-Klasse hat sich der Copy-Paste-Teufel eingeschlichen. Ich rufe derzeit eine Methode über call_user_func auf, obwohl ich genau weiß wie selbige heißt. Da hätte ich mal besser aufpassen sollen. Danke an onemorenerd für diesen Hinweis!

Als nächstes habe ich das etwas unschöne Parsen von Argumenten in der Console-Klasse. Das Gewirr sieht derzeit so aus:

		/**
		 * Parses a single argument
		 *
		 * @param $argument
		 * @return void
		 */
		protected static function _parseArgument($argument) {
			if(strstr($argument, '=')) {
				// definition
				$name = substr($argument, 1, 1);

				$parts = split('=', $argument);
				if(substr($parts[1], 0, 1) == '"') {
					$value = substr($parts[1], 1, strlen($parts[1] - 2));
				} else {
					$value = $parts[1];
				}

				self::$_arguments->append(new Brawler_Console_Argument($name, $value));
			} else {
				// option or optiongroup
				for($i = 1; $i < strlen($argument); $i++) {</pre>
<pre> 					self::$_arguments->append(new Brawler_Console_Argument(substr($argument, $i, 1)));
				}
			}
		}

Das geht wesentlich schöner: Hier muss gekapselt werden. Alleine schon, weil ich Plane, in naher Zukunft auch Argumente mit Doppel-Minus anzunehmen, die keine Gruppen darstellen. Die Prüfung hier einzubauen würde in noch mehr Gestrüpp enden. Also frisch ans Werk und einmal mit der Sense durch! Zwei neue Methoden: _parseNonValuedArgument und _parseValuedArgument – außerdem bedienen wir uns statt Zeichenketten-Methoden einfach Regulären Ausdrücken:

		/**
		 * Parses a single argument
		 *
		 * @param $argument
		 * @return void
		 */
		protected static function _parseArgument($argument) {
			$return = self::_parseNonValuedArgument($argument);
				if($return) return;

			$return = self::_parseValuedArgument($argument);
				if($return) return;
		}

		/**
		 * Parses an non valued argument or an argument group
		 *
		 * @param String $argument
		 * @return Bool
		 */
		protected static function _parseNonValuedArgument($argument) {
			if(preg_match('#-([a-zA-Z0-9]+)$#', $argument, $return)) {
				for($i = 0; $i < strlen($return[1]); $i++) { 					$newArgument = new Brawler_Console_Argument($return[1][$i]); 					self::$_arguments->append($newArgument);
				}
				return true;
			}
			return false;
		}

		/**
		 * Parses a valued argument
		 *
		 * @param String $argument
		 * @return Bool
		 */
		protected static function _parseValuedArgument($argument) {
			if(preg_match('#-([a-zA-Z0-9]{1})=(.+)#', $argument, $return)) {
				if(preg_match('#^"(.+)"$#', $return[2], $vReturn)) {
					$value = $vReturn[1];
				} else {
					$value = $return[2];
				}

				$newArgument = new Brawler_Console_Argument($return[1], $value);
				self::$_arguments->append($newArgument);

				return true;
			}

			return false;
		}

Wesentlich schicker, oder? Übrigens seht ihr da auch direkt einen kleinen Tipp: Man kann auf einen String wie auf ein Array zugreifen, wenn man an einzelne Zeichen will. Das ist in dem Fall wahrscheinlich die schnellste Methode.

Kommen wir nun zu einer Stelle, die ich bereits im Code hervorgehoben habe. Es geht um das mergen zweiter ArrayObject-Instanzen. Dieses scheinbar so einfach klingende Problem habe ich mit folgendem Konstrukt erschlagen:

			// Argument List
			$list = new Brawler_Plugin_Argument_List();

			// Plugin arguments
			// @TODO find a better way to merge ArrayObjects
			$plugins = Brawler_Plugin_Loader::getPlugins();
			$i = $plugins->getIterator();
			while($i->valid()) {
				$pluginArguments = $i->current()->getArguments();
				$k = $pluginArguments->getIterator();
				while($k->valid()) {
					$list->append($k->current());
					$k->next();
				}
				$i->next();
			}

Für Array gibt es array_merge(). Für ArrayObject-Instanzen … nicht. Eine solche Methode ist allerdings schnell geschrieben. Eine neue Klasse angelegt, ArrayObject abgeleitet und eine Methode merge rein:

	/**
	 * Array Object WITH merge method
	 *
	 * @package     Brawler
	 * @subpackage  Array
	 * @author      Cem Derin,
	 * @copyright   2009 Cem Derin,
	 */
	class Brawler_Array_Object extends ArrayObject {
		/**
		 * Merges an Array Object in this one
		 *
		 * @param ArrayObject $array
		 * @return void
		 */
		public function merge(ArrayObject $array) {
			$i = $array->getIterator();
			while($i->valid()) {
				$this->append($i->current());
				$i->next();
			}
		}
	}

Das war einfach. Nur noch statt von ArrayObject von dieser Klasse ableiten und gut ist. Denkste! Das Ding Merged nämlich nicht wirklich, sondern ergänzt die eine Liste um die andere – und berücksichtigt somit nur eine Ebene. Ein Array als Element erkennt unser Freund nicht. Ist aber derzeit auch gar nicht notwendig.

Das war es auch schon. Phew. Ich habe zwar hier und dort noch ein paar kleinere Anpassungen vorgenommen, das waren aber maximal Formatierungsänderungen (die mache ich übrigens bei Quellcode lesen oft schon unbewusst – und das kann Kollegen manchmal in den Wahnsinn treiben ;) ).

Auf Martin‘s Anmerkung, statt Klassen Objektinstanzen zu verwenden, will ich aber noch kurz eingehen: Kommt. Das würde nämlich doch etwas mehr Arbeit bedeuten, die ich heute nicht auf mich nehmen will ;)


#001
18. Dez 2009
onemorenerd

“Daher werden wir zwei neue Methoden einführen: _checkFlag, _checkValue und _checkSpecificValue.”
Das sind drei. :)

Warum verwendest du eigentlich kein Type Hinting? Manchmal gibst du einen Type im phpDoc an (z.B. @param Brawler_Router_Argument $routeArgument), aber in der Methodensignatur fehlt der Type Hint.


#002
18. Dez 2009
Cem Derin

An manchen stellen verwende ich schon Type hinting. Dort wo nicht habe ich es entweder vergessen oder war zu faul ;)

// kommentieren

// senden
theme von mir, software von wordpress, grid von 960 grid system. funktioniert in allen browsern, aber der safari bekommt das mit der schrift am schönsten hin.