<?php
$skipOptionalParts = FALSE;
$optionalPartCount = 0;
foreach ($this->uriPatternSegments as $uriPatternSegment) {
foreach ($uriPatternSegment as $routePart) {
if ($routePart->isOptional()) {
$optionalPartCount++;
if ($skipOptionalParts) {
if ($routePart->getDefaultValue() === NULL) {
return FALSE;
}
continue;
}
} else {
$optionalPartCount = 0;
$skipOptionalParts = FALSE;
}
if (!$routePart->match($requestPathSegments)) {
if ($routePart->isOptional() && $optionalPartCount == 1) {
if ($routePart->getDefaultValue() === NULL) {
return FALSE;
}
$skipOptionalParts = TRUE;
} else {
return FALSE;
}
}
if ($routePart->getValue() !== NULL) {
$matchResults[$routePart->getName()] = $routePart->getValue();
}
}
if (isset($requestPathSegments[0]) && F3::PHP6::Functions::strlen($requestPathSegments[0]) == 0) {
array_shift($requestPathSegments);
}
}
?>
Refactorings
No refactoring yet !
Ollie
December 23, 2008, December 23, 2008 06:19, permalink
Thanks for posting bwaidelich. I commend you for wanting to improve the readability of this code.
My suggestion is to use a couple of extract method refactors [http://www.refactoring.com/catalog/extractMethod.html]. Most obviously you can start with the duplicated lines (2-3 and 15-16; 9-11 and 20-22) and work from there. Replace variables with properties where you need to share data across methods.
I strive to keep levels of nesting below 3 or 4. If you have a foreach within a foreach within an if within an if etc. readability suffers. Breaking these apart into separate methods allows you to gives names to the operations you are performing resulting in code that is, in some sense of the term, self-documenting.
One example -- an extract method refactor as demonstrated below:
<?php
foreach ($this->uriPatternSegments as $uriPatternSegment) {
foreach ($uriPatternSegment as $routePart) {
$this->whateverItIsYouAreTryingToDo($routePart);
}
}
bwaidelich
January 9, 2009, January 09, 2009 17:12, permalink
Hi Ollie and thanks a lot for your suggestions!
I like the "extract method" refactoring to reduce duplication and I use it wherever I can.. But I can't imagine how that would make the code cleaner in the case. Or how would you e.g. extract lines 9-11/20-22 into a method without bloating the code even more?
Your second advise was really helpful! It made it clear to me, that the class is actually doing too much currently which made it difficult to add new properties without crossing the boundaries of the classes responsibility. So I'm gonna add a new Class which is only responsible for this very task. That will make it possible to act on your advice and separate this monster into smaller methods.
Thanks again!
Bastian
Hi everyone!
I'm working on a routing mechanism and got stuck here..
This is the part where the Route checks whether the current request matches.
$this->uriPatternSegments is an ArrayObject with each element being an array of one or more so called Route Parts.
There are optional Route Parts. If the first optional Route Part ($routePart->isOptional()) in a row of optional parts matches ($routePart->match() => TRUE), all other Route Parts have to match too. If the first optional Route Part does not match, following optional Route Parts must be skipped (until there is a non-optional Route Part)..
I don't think, that the many boolean returns are a problem. Performance is no big issue either as the result gets cached. What I have problems with is the bad readability - especially the temporary variables $skipOptionalParts and $optioanlPartCount smell bad..
Thanks for your help!