I’m with tom that it’s a LOT to take in at once – needs to be broken down section by section.
But a few observations can be made at least about code style, I’ll just go through a few files and point out the more obvious little tweaks I’d suggest:
index.php
Not wild about adding values to the server superglobal… especially since in the past I’ve run across a few servers that make them read only. (Not sure how it was done, but I’ve seen it)
You’re setting a number of values in the index.php on $RQ (Request object) that I’m wondering why you’re not doing that on the object’s constructor… though I’d have to see every instance of that object being built to say for sure, I’d be passing those three variables to the __CONSTRUCT and handling it there… or letting it call App::getCFPForUrl to pull those values itself. In general you’re passing a lot of values back and forth on the method calls – which begs the question why use objects in the first place on that? I could see it for setting values on the object, but you’re passing by reference to return results, kind-of defeating the point.
Startup.php:
A lot of pointless comments – the closing curly bracket and double line break isn’t enough, you’ve got to put a giant run of minus signs or asterisks in there? More useful/cleaner to read would be to replace those with saying WHAT is being closed.
So instead of:
}
//--------------------------------------------------------------------------
Do:
} // method getCFPForUrl
Which is what I was trying to say in another thread and got shot down because nobody seemed to pay attention to the intent and just focused on the performance part.
Back tracking APP to APP_BASE in _core.php I’m seeing a init() function – why isn’t that a constructor, especially since you’re passing no values to it?
Looking at that init() function, you could probably benefit greatly by unrolling that loop; You’ve got a bunch of ‘logic for nothing’ in there with building an array on a local var, iterating the array, etc… etc… and frankly, I’m not seeing the point…
Since it LOOKS like you are combining $_GET, $_POST, $_COOKIE, $_ENV and $_SERVER into one giant array – Good lord why? Anything that calls that is probably going to be insecure because of it… You’re basically creating your own registerglobals; which you then include code to unset for legacy versions of PHP… I’d just assume everyone’s using 5.2 or newer and aren’t DUMB ENOUGH to have register_globals enabled. (Might be a bad assumption though…)
You should be aware that they WERE talking about breaking loose include calls on future versions of php… as a rule of thumb I dislike them JUST because they’re inconsistent from every other function call… for clarity/consistency I’d be putting the parenthesis around every include/require.
… and I’d probably swap a LOT of your includes for require_once. If you end up calling that “include $configFile” more than once, there’s probably something wrong with the code or you’re dealing with a code appendage attack.
The getURLParts method is just calling $_GET or $_SERVER, and isn’t setting ANYTHING inside the object – this is an example of “objects for nothing” as there’s no good reason for that function to be inside an object in the first place.
You combine the two, and you end up asking “what’s this app class even FOR?!?” – the only part of it that actually looks to do anything that would be useful in the program is including the config files.
Back to startup.php, the extended version of this class is similarly… iffy… though at least it looks to be doing something a bit more useful with the object by it having meaningful properties. That said again I’m not seeing why you would be putting getURLforCFP, redirectToCFP, sendMail, translateClass or a dozen other of the routines in there inside the object as they neither modify the object, or require the object to run… I smell the “Object just for namespace” ideology in action on that; which IMHO is not a legitimate reason to use objects.
I’m also seeing some evaluations that could be simplified to take advantage of short-circuit eval and processing evaluations.
For example inside getCFPForURL
$func = $urlParts[sizeof($urlParts) - 1];
if (!$func || ($class == 'Controller_Application')) {
$func = 'index';
}
if (substr($func, -4) == '.php') {
$func = substr($func, 0, -4);
}
“overevaluation”… The variable assignment and the check for true could be condensed down into the if statement, while the second IF should NEVER be true if the first if statement fires – so get that behind an else. String compare’s in boolean checks are also slower than the various functions for handling, so I’d be using strrpos to handle that.
if (
!($func=$urlParts[sizeof($urlParts)-1]) ||
($class=='Controller_Application')
) {
$func = 'index';
} else if strrpos($func,'.php',-4) {
$func=substr($func,0,-4);
}
The real change though is the else – Programming 101 - NEVER test for values you already know the condition of… if the first condition evals true you are setting func to ‘index’ so there’s no reason to run the second check… You’ve got a number of those across the entire codebase.
Likewise thanks to short-circuit eval and inline eval, we can do the assignment to $func the same time as checking it’s result. Less code == better.
that same function also seems to have a few variables for nothing… $get for example. What not just pass $params directly? I’m also not fond of having variables with the same name as reserved words and/or superglobals… It’s confusing when the only difference is an underscore.
I also prefer count instead of sizeof (that’s more a personal preference… sizeof always makes me think bytes not elements) and you are brute-forcing in a number of places pulling the value from the last element, when the END function can provide that same functionality. Likewise strings can be indexed as arrays of character, negating the need for STRPOS when accessing a single character.
I’d probably have written that section a bit more like this (raw/untested quicky – probably buggy, just so you get the idea)
public static function getCFPForUrl(& $class, & $func, & $params, $url = false) {
if ($url===false) $url=$_SERVER["REQUEST_URI"];
self::$originalUrl=$url;
$parts=parse_url($url);
$path=isset($parts['path']) ? $parts['path'] : '/';
if (isset($parts['query'])) {
parse_str($parts['query'],$params);
} else {
$params=array();
}
if $url[0]!=='/' $url='/'.$url;
self::$urlParts==explode('/',substr($path,1));
$class='Controller_'.(
count(self::$urlParts)==1) ?
'Application' :
ucfirst(self::$urlParts[0]);
);
if (
!($func=end(self::urlParts)) ||
($class=='Controller_Application')
) {
$func='index';
} else if strrpos($func,'.php',-4) {
$func=substr($func,0,-4);
}
} // method getCFPForURL
But all that is just minor stuff – For the most part your code is reasonably clean and easy to follow – I do end up with a few questions about how/why but nothing that couldn’t be figured out by looking a bit deeper into it.
It’s very refreshing to see sensible and consistent indentation, even if it is spaces for tabs (and four space tabbing at that)… but that’s a personal preference thing. (I’ve always been a die hard for using tabs for tabs, then setting the editor to show however much tabbing I like; typically 2 space)
Are you following a style guide of some sort? It looks like you are – or have developed pretty decent coding habits.
There is ONE thing I’m noticing that I’m not wild about – you have your markup output in the logic flow… makes skinning the project a royal pain; I would seriously look into separating anything that handles data from the code that outputs the data. It’s why I usually have procedural theme_ functions loaded depending on the overall function the page is trying to handle, being called by my objects when it comes time to output markup.
Much like separating presentation from content in HTML/CSS, it can make your code clearer to separate your output from your data handling… functions like ‘linkfor’ and “redirectToCFP” really don’t belong inside the data handling object IMHO… that’s outputting markup, and hardlining it into the object is just making any future skinning efforts harder in the long term…
See Wordpress for examples of that type of sillyness in action with it’s outputting tons of unnecessary classes and other elements that you either end up resorting to regex to replace (ugly/slow) or have to modify the actual code for (neutering your upgrade path). If it was in a separate skin file or handled by the skinning system, it wouldn’t be a problem.
I also avoid using <<<END END; for echo… The handful of characters you save by going $url instead of ‘,$url,’ isn’t made up by the extra characters on the open and close, and it just feels a lot less clear what’s going on to me. (YMMV).
Around HTML output I like to throw in extra carriage returns – it makes it clearer we’ve started something new and unlike the long breaks of minus signs or equal signs, it’s one character; big deal. To that end I also like to not indent the markup to match the PHP indentation – I can usually keep them straight… and even if it is just a simple redirect that’s no reason to ignore something like heading orders and semantic markup… and when multiple sections are basically outputting the same thing, just change the parts that are DIFFERENT. so your redirect function for example:
public static function redirectToCFP($class, $func='index', $params=array()) {
$url=self::getUrlForCFP($class,$func,$params);
echo '
<html><head>
<title>';
if (Config::$debugRedirect) {
echo 'Debug Redirecting</title>
</head><body>
<h1>Debug redirection is turned on.</h1>
<p>
Script is redirecting you to:<br />
<a href="',$url,'">',$url,'</a>
</p>';
} else {
header("Location: $url");
echo 'Redirecting...</title>
<meta http-equiv="refresh" content="1;url=',$url,'">
</head><body>
<h1>Redirecting.</h1>
<p>
If your browser isn't automatically redirected,
please click <a href="',$url,'">here</a>.
</p>';
}
echo '
</body></html>';
die;
} // redirectToCFP
Though that could also be more of a personal preference… but formatting differences aside notice I moved the like outputs before/after each conditional instead of saying the same thing over again inside them. Sometimes it’s even better to run multiple if’s, even of the same condition, than it is to say the same code twice. (oops, forgot to move HEADER up before the start…)
