Page 1 of 1

Do you consider this code hard to read?

Posted: Thu Apr 08, 2010 2:54 am
by josh

Code: Select all

<?php
  
if ( $this->is_premium ) {
	// premium only get skyscraper if they have a campaign:
	if( $this->hasCampaign() && $this->getCampaign()->hasSkyscraper() )
	{
		echo $this->getCampaign()->displaySkyscraper();
		echo "<br />";
	}
	
	if( $this->hasCampaign() && $this->getCampaign()->hasSquare() )
	{
		echo $this->getCampaign()->displaySquare();
	}

} else if ( ! $_REQUEST[ 'pfv' ] ) {
?>
					<div id="masterdiv" class="links">
						<ul>
							<li><a href="<?=URL_HOME_U?>/">Home</a></li>
							<li><a href="#" onClick="SwitchMenu('sub3')">Directory</a></li>
								<span class="submenu" id="sub3">
								<ul>
<?php
	require( CONFIGURATION_PATH . '/categories.inc.php' );
	foreach( array(
		'abc',
		'xyz',
		'abc',
		'xyz',
		'abc',
		'xyz',
                'abc',
		'xyz',
		'abc',
		'xyz',
		'abc',
		'xyz',
                'abc',
		'xyz',
		'abc',
		'xyz',
		'abc',
		'xyz',
	) as $code ) {
		$description = $categories[ $code ];
?>
									<li><a href="<?=URL_HOME_U?>/index?a=&b=<?=htmlentities( $code , ENT_QUOTES, 'UTF-8' )?>"><?= $description ?></a></li>
<?php
	}
?>
								</ul>
								</span>
							<li><a href="<?=URL_HOME_U?>/asdf/">aasdf</a></li>
							<li><a href="<?=URL_HOME_U?>/asdfasdf">asdf</a></li>
							<li><a href="<?=URL_HOME_U?>/sdf">sdf</a></li>
							<li><a href="#" onClick="SwitchMenu('sub1')">sdf</a></li>
								<span class="submenu" id="sub1">
								<ul>
								    <li><a href="<?=URL_HOME_U?>/sdf/">SDF</a></li>
								    <li><a href="<?=URL_HOME_U?>/sdf/">sdf</a></li>
								</ul>
								</span>
							<li><a href="<?=URL_HOME_U?>/forums/">Forums</a></li>
						</ul>	
					</div>
					<br />
<?= $button ?>
<br />
<br />

<?php
$this->display_translate_widget();
?>
	

					<br />

					
					<?php
                    if( $this->hasCampaign() && $this->getCampaign()->hasSkyscraper() )
                    {
                        echo $this->getCampaign()->displaySkyscraper();
                    }
  
				?>
<?php
// No left nav 
}
?>
I'm trying to negotiate a coding standard for a project with a co-worker. We've the proverbial curly brace on same line or next line debate over, I agreed to put them on the same line even though I like them on the next line so the line up, but now we've locked horns on where the opening/closing tags should go when mixing html+php.

Read the above code over closely before opening the image below. Read it over slow and carefully, Then take a look at the image below, then I want to know did you mis-read the code?


...

Click > http://img13.imageshack.us/img13/734/95180883.jpg

Did you think opening bracket labelled one was actually matching closing bracket labelled two? Do you think this code formatting style makes the code unreadable? The supposed logic behind is it that you can look down the left hand side to see where PHP begins & ends, but I personally think it is unorthodox and unreadable.

Also, have any of you guys seen any code written like this before? How do you think it would impact your productivity, personally, to have to work on code like this. Do you think its a serious problem or just a minor annoyance?

PS. my example was edited down too, the original example that sparked the debate was so long that you couldn't see the whole problem at once, you had to scroll. That is not the case with this edited example.

Re: Do you consider this code hard to read?

Posted: Thu Apr 08, 2010 3:42 am
by Christopher
I don't find it unreadable. Or should I say any less readable than most mixed HTML/PHP pages.

The only thing I would do differently is this. I don't like the multi-line array definition in the foreach. I'd rather see:

Code: Select all

        $code_list = array(
                'abc',
                'xyz',
                'abc',
                'xyz',
                'abc',
                'xyz',
                'abc',
                'xyz',
                'abc',
                'xyz',
                'abc',
                'xyz',
                'abc',
                'xyz',
                'abc',
                'xyz',
                'abc',
                'xyz',
                );
        foreach ($code_list as $code) {
I also noticed in editing that code that I write control structures K&R style as you would use parens in written English e.g., "if ($condition)" instead of "if( $condition )". Both ways seem readable though, so I wouldn't mind getting code either way.

Re: Do you consider this code hard to read?

Posted: Thu Apr 08, 2010 3:50 am
by requinix
(Hey Chris: feel like moving this to Critique?)

I didn't have any problems with it, probably due in part to it being similar to my own style.

I, for one, don't give a damn whether people put the opening braces on the same or the next line. It's such a small thing and I'm sure developers would spend more time arguing about it than they would just learning to read the code.

I support at-a-glance reading, but when people glance at code and feel something's wrong, they should look harder. Writing really nice looking code can be a hassle and not worth the extra effort. Of course there's quite a bit of emphasis on that "can": there is the basic hygiene of coding (indentation, whitespace, commenting) but then there are the people who feel the need to scrub their hands every hour, and that's the point at which it starts interfering with their lives.

Indentation with PHP can be a bit trickier because it can be embedded in something completely different. Which is why any good developer uses an editor with syntax highlighting: so they can differentiate the PHP from the not-PHP. That helps. For me, HTML is in darker colors and PHP in brighter.
But sometimes more is necessary. For example I use

Code: Select all

<html>
<head>
<title><?php echo $title; ?></title>
<?php foreach ($stylesheets as $s) { ?>
<link rel="stylesheet" type="text/css" href="<?php echo $s; ?>" />
<?php } ?>
</head>

<body>
<?php

// a lot of complicated logic

?>
</body>
</html>
1. Short echos use minimal space
2. Simple loops use simple code
3. Anything beyond that involves big PHP blocks, with opening and closing tags on individual lines and whitespace after and before respectively
Plus
4. If I find that I keep switching between PHP and HTML, I stick the HTML in an echo and run everything through PHP

IMO, the whole "coding standards" thing has been blown way out of proportions.


About the huge array:
It's possible to have too much whitespace. If the array is very long, like that one, I'd move it to a variable like Chris said, but I'd reformat it to something more like

Code: Select all

$code_list = array(
	'abc', 'xyz', 'abc', 'xyz', 'abc', 'xyz', 'abc', 'xyz',
	'abc', 'xyz', 'abc', 'xyz', 'abc', 'xyz', 'abc', 'xyz',
	'abc', 'xyz',
);

Re: Do you consider this code hard to read?

Posted: Thu Apr 08, 2010 4:43 am
by dejvos
From my point of view it is readable but personally I prefer to not mix HTML with PHP document.

Re: Do you consider this code hard to read?

Posted: Thu Apr 08, 2010 5:30 am
by josh
dejvos wrote:From my point of view it is readable but personally I prefer to not mix HTML with PHP document.
There's always going to be some amount of mixing, always. Even putting the HTML in an echo statement is still mixing.

Thanks for the responses. My reason I gave my co-worker to not do it was that noone would find it readable. I think I have been proven wrong, and wrote him an email saying we'll do it his way (or at least agree to have mixed styles on the project). The other thing that I concluded & wrote to him, was that the reason I found it unreadable was all the other secondary (really primary) problems, such as the abuse of white space. If we just clean up some of the other stuff, then most of my complaints go away, I think.

Thanks for your input. Made me realize this is small potatoes compared to other stuff, like moving the big blocks of code inside the conditionals to their own includes. I think a good heuristic would be try to keep the opening & closing brace both visible without scrolling.