The solution could be to remove the extra comma, not to add useless brackets.
The solution could be to remove the extra comma, not to add useless brackets.
I won’t say I like 100% of it, but I like 95% of the Mono coding standard http://www.mono-project.com/Coding_Guidelines that’s very nice.
Open any framework file.
There is the bestest style.
Qiang not fool, he’s very smart,
Yii style is code art! 
if(($allow=$rule->isUserAllowed($user,$filterChain->controller,$filterChain->action,$ip,$verb))>0) // allowed
break;
else if($allow<0) // denied
{
$this->accessDenied($user);
return false;
}
For those that can easily read and prefer this code, you are much better than me. Even after 10 years of PHP coding, I struggle with code written like this and want to run screaming. Yes, I can eventually decipher it. But I often need to reference the framework code to supplement the documentation and it often gets pretty difficult to interpret. As far as braces, I feel Python’s indent-based blocks are clean and elegant, but I really don’t like how PHP requires braces for multiple lines yet allows single lines to omit them. It’s just another reason why PHP feels like a kludgy language without consistency.
Your example is very extreme because of the lengthy argument list for isUserAllowed. I agree that this is not a nice style. But i don’t think you find such “beasts” in the framework code.
That’s consistent and logical, braces are for grouping multiple lines, no multiple lines no braces.
That code block was pulled from CAccessControlFilter in the framework, which I was working on before I posted. I must be unfortunate, then, since I keep finding such beasts in the framework.
Another example: I wanted to better understand how controllers are determined, and whether I could work within Yii’s paradigm or needed to create my own handler. So I naturally analyzed the createController method of the CWebApplication class. Wow. I don’t know if my abilities are lacking compared to others, but I find this to be a monster:
public function createController($route,$owner=null)
{
if($owner===null)
$owner=$this;
if(($route=trim($route,'/'))==='')
$route=$owner->defaultController;
$caseSensitive=$this->getUrlManager()->caseSensitive;
$route.='/';
while(($pos=strpos($route,'/'))!==false)
{
$id=substr($route,0,$pos);
if(!preg_match('/^\w+$/',$id))
return null;
if(!$caseSensitive)
$id=strtolower($id);
$route=(string)substr($route,$pos+1);
if(!isset($basePath)) // first segment
{
if(isset($owner->controllerMap[$id]))
{
return array(
Yii::createComponent($owner->controllerMap[$id],$id,$owner===$this?null:$owner),
$this->parseActionParams($route),
);
}
if(($module=$owner->getModule($id))!==null)
return $this->createController($route,$module);
$basePath=$owner->getControllerPath();
$controllerID='';
}
else
$controllerID.='/';
$className=ucfirst($id).'Controller';
$classFile=$basePath.DIRECTORY_SEPARATOR.$className.'.php';
if(is_file($classFile))
{
if(!class_exists($className,false))
require($classFile);
if(class_exists($className,false) && is_subclass_of($className,'CController'))
{
$id[0]=strtolower($id[0]);
return array(
new $className($controllerID.$id,$owner===$this?null:$owner),
$this->parseActionParams($route),
);
}
return null;
}
$controllerID.=$id;
$basePath.=DIRECTORY_SEPARATOR.$id;
}
}
I know the framework is supposed to take care of these things for us, but I also thought that it’s not a closed box for a reason, allowing developers to get inside the code to better work with it. I like much about Yii, I just personally would request the code changes mentioned by the OP and others in this thread.
Hmm. I don’t see any “beast” in the code you pasted. As already discussed, things like this are a matter of personal taste. I like the coding style very much and would be annoyed if the code where stretched by inserting a lot of uneccessary spaces and newlines. It’s much more compact so you don’t have to scroll much as it uses the screen space very efficient.
Maybe you should try another code highlighting in your IDE?
This is how it looks in my vim. Colors are gross, but definitely help the eye to understand quickly.
Just for clarification, the color highlighting in the snippet I referenced is handled within the <php/> block and isn’t from my IDE.
The issue isn’t color for me, it is the lack of self-documenting style, and the very condensed nature you prefer. It’s not that I’m annoyed by the style – it’s that it often double/triples the time it takes me to reverse-engineer the code compared to other projects on which I’ve worked. This style favors people who have a sharper mind for technical processes, but at least for me is a big hindrance. I struggle through it since I do like so much of the other aspects of Yii.
The flip side of this would be the Kohana framework, which for me is much easier to follow in code, but I do not like the infrastructure as a whole compared to Yii. Why is that always the case? 
Anyway, I completely understand your perspective, since if I had your ability to understand Yii code at a glance I would favor the current format as well.
Usually I would adapt the frameworks coding style, but this in Yii is just not worth it. I totally accept, that performance is motivator for this framework. But if you choose 4ns parsetime over 20 seconds readability time you need to visually parse the code. You optimize at the wrong end.
Every language has - over the years - envolved a certain quasi standard, so that it is good readable and prevents code bugs just by staying with the code standards. I like the one for PHP from Drupal. A very huge project which powers thousends of big websites. http://drupal.org/coding-standards It’s not that far away from Zend.
Quang’s coding style is not found somewhere in PHP accepted coding standards. On the one hand he uses no empty lines even inside long code blocks, but curly braces go on the next line. No spaces inside signatures of functions and statements no mater how long it is.
http://framework.zend.com/manual/de/coding-standard.coding-style.html
Well Yii is young and will grow. The potential is there. The time will come when other people would like to contribute code, but not with this dicated code style. There are a LOT people which started script coding with PHP and hopefully they used zend, cake or drupal to do a few steps in the framework direction. They will totally dislike this C# or what ever background this code style has.
$foo = 10;
$goo = 'C';
$bar = 0;
if ($foo == 10)
if ($goo == 'B')
$bar = 1;
else
$bar = 2;
There is a good reason why in good coding practice and code styles curly bracets are standard and not ommited.
I guess I’ll wait a bit for Yii to use. Watched the blog video and was almost scared by the code I’ve seen there. Sad.
EASY Readability is a huge plus for a framework to grow a userbase.
Altered:
public function createController($route, $owner = null)
{
if ($owner === null) {
$owner = $this;
}
if (($route = trim($route, '/')) === '') {
$route = $owner->defaultController;
}
$caseSensitive = $this->getUrlManager()->caseSensitive;
$route .= '/';
while (($pos = strpos($route, '/')) !== false) {
$id = substr($route, 0, $pos);
if (!preg_match('/^\w+$/', $id)) {
return null;
}
if (!$caseSensitive) {
$id = strtolower($id);
}
$route = (string) substr($route, $pos + 1);
// first segment
if (!isset($basePath)) {
if (isset($owner->controllerMap[$id])) {
return array(
Yii::createComponent($owner->controllerMap[$id], $id, $owner === $this ? null : $owner),
$this->parseActionParams($route),
);
}
if (($module = $owner->getModule($id)) !== null) {
return $this->createController($route, $module);
}
$basePath = $owner->getControllerPath();
$controllerID = '';
}
else {
$controllerID .= '/';
}
$className = ucfirst($id) . 'Controller';
$classFile = $basePath . DIRECTORY_SEPARATOR . $className . '.php';
if (is_file($classFile)) {
if (!class_exists($className, false)) {
require($classFile);
}
if(class_exists($className, false) && is_subclass_of($className, 'CController')) {
$id[0] = strtolower($id[0]);
return array(
new $className($controllerID . $id, $owner === $this ? null : $owner),
$this->parseActionParams($route),
);
}
return null;
}
$controllerID .= $id;
$basePath .= DIRECTORY_SEPARATOR . $id;
}
}
Everybody can argue about his or her own style… but I think symfony got the right idea… one standard is better many standards… the same reason why all major frameworks are working together to get a auto loading standard…
http://test.ical.ly/2010/05/11/symfony-2-now-following-the-standards-rather-than-setting-them
Any reason why Yii is not in this group? 
http://groups.google.com/group/php-standards/web/phpdeveloper-org-blog-post
Playing nice together only helps developers in the end.
Can’t disagree more. None of them is useless.
The extra comma is to avoid making mistakes when you add a new element into array. That’s a good approach, I think.
And are the brackets useless? Did you ever encounter a problem after you added one line, that you thought was a part of an if statement and after an hour of debugging it turned out it wasn’t? Just because of the lack of brackets? I did and I suppose many people would appreciate that.
I agree that useless, extra characters are bad, but if it increases readability and reduces risk of making mistakes… it’s totally worth it.
Voting for useless brackets
Yes, in theory. Sometimes single line statement becomes multiple lines statement and it’s very easy to make a mistake here. It’s the same like with HTML code. The best practices says to close all tags that are optional to close. Don’t leave any space to possible mistakes.
This is where the art and science of programming clashes… There is no right answer to this issue since there is no real way of "proving" one way is better then another.
So it really comes down to consistency and and what the "leader" of the project wants.
I agree that a standard should be set and enforced. If a professor of the programming class defines that there should be no braces on single line if statements and you do, then it’s wrong.
Efficiency should be looked at from a bigger picture
Spaces and and extra brackets can be processed by code compactors and optimizers to make the code more efficient for execution.
Therefore, prefer developer efficiency over PHP parser efficiency.
(Bad algorithms are independent of this discussion)
2a. Not typing an extra space may be more efficient for the single programmer at the expense of the multiple programmers who need to read the code and understand it.
3a. Not adding brackets to if statements will (not may) introduce a bug which will take another programmer hours to identify. Polish notations help reduce bugs from assigning incorrect data types and helps the next programmer.
For me, the art and science of programming and what I measure a programmer on to determine if one is a Mozart or a Vincent van Gogh of programming has to do with how easily can I read someone else’s code and understand what is happening. Clear structure, flow, naming conventions and self documenting code help to make a masterpiece. This opinion is from years of experience maintaining other people’s code. A programmer can take a very complex and brilliant solution and make it easy for someone else to read and understand. That’s when you know a scientist and an artist is born.
There are a lot of computer scientists out there. What really separates one computer scientist from another is how beautiful is the code that is generated.
Just my 2 cents.
I think some of you are missing the point. This code
[spoiler]
public function createController($route,$owner=null)
{
if($owner===null)
$owner=$this;
if(($route=trim($route,'/'))==='')
$route=$owner->defaultController;
$caseSensitive=$this->getUrlManager()->caseSensitive;
$route.='/';
while(($pos=strpos($route,'/'))!==false)
{
$id=substr($route,0,$pos);
if(!preg_match('/^\w+$/',$id))
return null;
if(!$caseSensitive)
$id=strtolower($id);
$route=(string)substr($route,$pos+1);
if(!isset($basePath)) // first segment
{
if(isset($owner->controllerMap[$id]))
{
return array(
Yii::createComponent($owner->controllerMap[$id],$id,$owner===$this?null:$owner),
$this->parseActionParams($route),
);
}
if(($module=$owner->getModule($id))!==null)
return $this->createController($route,$module);
$basePath=$owner->getControllerPath();
$controllerID='';
}
else
$controllerID.='/';
$className=ucfirst($id).'Controller';
$classFile=$basePath.DIRECTORY_SEPARATOR.$className.'.php';
if(is_file($classFile))
{
if(!class_exists($className,false))
require($classFile);
if(class_exists($className,false) && is_subclass_of($className,'CController'))
{
$id[0]=strtolower($id[0]);
return array(
new $className($controllerID.$id,$owner===$this?null:$owner),
$this->parseActionParams($route),
);
}
return null;
}
$controllerID.=$id;
$basePath.=DIRECTORY_SEPARATOR.$id;
}
}
[/spoiler]
is bad primarily because it’s overcomplicated, contains too much logic, not because it lacks spaces or curly brackets on every line. Spaces were probably extremely important when there were no syntax highlighting. Now you can setup your IDE to highlight variables, operators, keywords etc. to distinguish them in the mess.
I can easily see where variables and keywords are, but it doesn’t help a bit. There’s just too much logic. This method is unreadable.
You suggets to add spaces, brackets etc. so that the code becomes like this:
[spoiler]
public function createController($route, $owner = null)
{
if ($owner === null) {
$owner = $this;
}
if (($route = trim($route, '/')) === '') {
$route = $owner->defaultController;
}
$caseSensitive = $this->getUrlManager()->caseSensitive;
$route .= '/';
while (($pos = strpos($route, '/')) !== false) {
$id = substr($route, 0, $pos);
if (!preg_match('/^\w+$/', $id)) {
return null;
}
if (!$caseSensitive) {
$id = strtolower($id);
}
$route = (string) substr($route, $pos + 1);
// first segment
if (!isset($basePath)) {
if (isset($owner->controllerMap[$id])) {
return array(
Yii::createComponent($owner->controllerMap[$id], $id, $owner === $this ? null : $owner),
$this->parseActionParams($route),
);
}
if (($module = $owner->getModule($id)) !== null) {
return $this->createController($route, $module);
}
$basePath = $owner->getControllerPath();
$controllerID = '';
}
else {
$controllerID .= '/';
}
$className = ucfirst($id) . 'Controller';
$classFile = $basePath . DIRECTORY_SEPARATOR . $className . '.php';
if (is_file($classFile)) {
if (!class_exists($className, false)) {
require($classFile);
}
if(class_exists($className, false) && is_subclass_of($className, 'CController')) {
$id[0] = strtolower($id[0]);
return array(
new $className($controllerID . $id, $owner === $this ? null : $owner),
$this->parseActionParams($route),
);
}
return null;
}
$controllerID .= $id;
$basePath .= DIRECTORY_SEPARATOR . $id;
}
}
[/spoiler]
Does it help a bit? No. It’s as hard as it were to understand what’s going on. In fact, it became harder to understand now, because it doesn’t fit completely on my screen.
You all probably know the rule of thumb that a method should fit into screen, otherwise it should be split. Compare the result:
public function createController($route, $owner = null)
{
if ($owner === null)
$owner = $this;
if (($route = trim($route, '/')) === '')
$route = $owner->defaultController;
$route .= '/';
$basePath = '';
$controller = null;
$controllerID = '';
while (($pos = strpos($route, '/')) !== false)
{
$id = $this->getNextRouteSegmentId($route, $pos);
if ($id == null)
return null;
if ($basePath == '') { // first segment
if ($this->tryLoadControllerFromMap($controller, $route, $id, $owner))
return $controller;
$basePath = $owner->getControllerPath();
}
else
$controllerID .= '/';
if ($this->tryLoadControllerFromFile($controller, $route, $id, $owner, $basePath, $controllerID))
return $controller;
$controllerID .= $id;
$basePath .= DIRECTORY_SEPARATOR . $id;
}
}
private function getNextRouteSegmentId(&$route, $pos)
{
$id = substr($route, 0, $pos);
if (!preg_match('/^\w+$/', $id))
$id = null;
else if (!$this->getUrlManager()->caseSensitive)
$id = strtolower($id);
$route = (string)substr($route, $pos + 1);
return $id;
}
private function tryLoadControllerFromFile(&$controller, $route, $id, $owner, $basePath, $controllerID)
{
$className = ucfirst($id) . 'Controller';
$classFile = $basePath . DIRECTORY_SEPARATOR . $className . '.php';
if (is_file($classFile)) {
if (!class_exists($className, false))
require($classFile);
if (!class_exists($className, false) || !is_subclass_of($className, 'CController')) // forgotten case...
throw new CException("Controller $className not found in file '$classFile'.");
$controller = array(
new $className($controllerID . lcfirst($id), $owner === $this ? null : $owner),
$this->parseActionParams($route),
);
return true;
}
return false;
}
private function tryLoadControllerFromMap(&$controller, $route, $id, $owner)
{
if (isset($owner->controllerMap[$id]))
$controller = array(
Yii::createComponent($owner->controllerMap[$id], $id, $owner === $this ? null : $owner),
$this->parseActionParams($route),
);
if (($module = $owner->getModule($id)) !== null)
$controller = $this->createController($route, $module);
return $controller !== null;
}
When you look at this code, you can understand what’s going on within ten seconds. That’s what self-documenting code is. Yes, it’ll be compiled 5 nanoseconds slower. But it’ll be ten times easier to understand the code, to modify it, to fix bugs. (By the way, is it by design that if file doesn’t contain proper controller class, this method silently returns NULL?!)
Now, I’m not against proper formatting, I’d greately appreciate to see more nicely formatted code, but it’s not the primary problem with the mentioned method.
Regarding formatting. Come on guys, every single proper PHP IDE in the world supports reformatting. It’s a matter of clicking one button. I admit, I’m as lasy as one of the yii developers who has replied here; I don’t put spaces and newlines everywhere. But after typing some code I just press magic button and the code becomes nicely formatted. It’s doesn’t add unnecessary curly brackets, but I’m not a fan of them anyway; in the case of “if-if-else” IDE will just put “else” below second “if”, so I’ll easily notice the problem.
And optimizing for PHP compilers is a bad excuse. It’s framework code, so time of developers is much much much more important. With APC installed (and PHP optimizers are the first thing that should be done if one cares about speed) it becomes absolutely irrelevant.
I think you do not understand the philosophy of this framework and it is very sad. Sorry, but your version of createController() and splitting it to several methods (wich can not be reused) is no good. Splitting for readability is very poor idea. Splitting when it’s REAL needed is a good way. Especially when it comes to framework.
You have strange ideas about refactoring Maybe better to throw dice and split when will fall out 3(or 5, or 6)? 
No! 
This should be splitted, when it should be splitted.
And the philosophy is…?
What’s so special about framework code? Optimization for speed?
I don’t know what your programming background is, but splitting for readability is a common practice in large companies. That is, when there’re ten, hundred, thousand programmers. Because it greately improves effeciency of programmers.
Yes, I had been avoiding splitting into methods, unless it’s absolutely necessary. I’ve written an RTS game this way. 2/3 of the game code was in one long method, consisting of thousands of lines and going as deep as 7 levels of contructs (if-if-if-for-while-if…). But that was when I was learning in school and was 16 years old. 
It’s not just me. It’s kinda… the whole world.
I don’t know any tools for PHP static code analysis, but I’ve worked with C# and Java tools which have this rules.
Okay, imagine this situation. A user comes to you and complains createController returns null, even though the file with controller is if fact included. You don’t know anything about this method (except for signature), but you want to help. How long will it take you to analyse that he has forgotten to write “extends Controller” in both cases, with original code and with my code?
Now I’ve heard it all.
Break in several small -and self documented- methods IS GOOD.
With the power of computers today, there is no reason to sacrifice readability.
I really love using YII in my projects, but the thing is (not just for me, it seems) the "standard" it uses sucks.
I think it makes developers not to want to contribute that much…
And by the way, there is no need to be so arrogant when replying to other people, you can give your point of view with respect.
In end-user applications, yes. This is framework, feel the difference.
Sorry, but this is unreal situation. Absolutely imaginary.
This is no arrogance. Just facts. Open fw files. There is no splitting like Athari show. Why? Laziness? No. Deliberate resistance of readability? No. Rationalism? Yes. And I think that this is one of the parties to the philosophy of the framework.
Please read link you give once more time. It has nothing to do with what you say and how you made the refactoring of createController() method.
getNextRouteSegmentId(), tryLoadControllerFromFile(), tryLoadControllerFromMap() unusable, except to grow readability. It’s not refactoring. It’s overcoding. Do not confuse these two concepts