<style> attribute 'type="text/stylesheet"' missing (?)

short opening tag
$roles not initialized as array
missing quotes

<img>-Tag not closed, missing quotes @ align (is align attribute xhtml1.0 strict conform?)

<H1> should be <h1>

no evaluation of $_GET-data

there is no Array class in php

missing $-signs, calls count() every iteration, foreach looks better

missing </li>

</b></i> should be </i></b>

shouldn't we use <strong> and <em> instead of <b> and <i>?

did i get everything? :o

two additions:
it should be "font-family: blah" in the style area and you should specify more than one font. and times new sucks. ;)

and the :

$list[] = "Peach":

should be a ;

What knuspermagier said, plus

Font style demands this font, and none other, to be installed, font-family is a better option.

Usage of indices andi and detlef without quotes is valid, but might lead to trouble if someone decides to define constants named like this, I'd add some single-quotes

Closing / in img tag is missing (not valid xhtml this way).

<?= is deprecated if I'm not mistaken

$list[] = "Peach": => $list[] = "Peach";

And I miss a <title>

:)

Talked about single quotes, and missed the obvious: All strings should be defined in single quotes here, as none of them uses inline variables but would get parsed for them anyway.

Excellent answers from both of you. You found most of the obvious and I think all of the non obvious ones. Still a few left. Amazing how many errors you can put into such a small amount of code, eh? :-)

Hmm... missing list environment (<ul>...</ul> or <ol>...</ol>), plus enclosing <p>...</p> (I think that is obligatory, I'm not sure right now though)

Don't see anything else though

Hmmm, nothing left I think except that the <img> tag should have an alt="" attribute and the doctype definition is missing.

Okey, everything metnioned plus
- missing DTD
- styles in external file
- why not using style/class definition for the <li>'s instead of <em>/<i> or <b>/<strong>
- image tag missing some size/title attributes
- the user/role model is limit^H^H^H^H^H total crap

Besides what everyone else said, align=left should be quoted, and the title element is missing.

And crap, looks like those 2 were caught, but I'm going to post this comment anyway because my captcha says 'IROFL'

Also, it's bad for performance to context switch in php like that from html, to php, to html, to php, and back to html. Stick all the php code in the same section.

The list with <b><i>'s should be closed in the reverse order that they are opened. Instead of "<b><i>stuff</b></i>" it should be "<b><i>stuff</i></b>".

Though I do agree with just using css to set the li tag.

15... of various kind of mistakes

I am not a PHP guy at all and so I have absolutely no idea what is wrong with the code you posted (give me Ruby or Java and I am back in business ;-) ).

But what I can tell you: You should check your footnote-code: If I hover on http://www.splitbrain.org/blog/ over the 1)-footnote of this article FF3 shows me the footnote text of http://www.splitbrain.org/blog … _relations which is displayed on the same page at the moment. Something is broken there.

In addition you could add a newline by means of \n after the closing list item tag in the echo line like this (the style should be put into css as mentioned above):

echo "<li>".$list[$i]."</li>\n";

So you get a cleaner output of HTML source code like this:

<ul>
<li>Apple</li>
<li>Peach</li>
<li>Orange</li>
</ul>

instead of

<ul><li>Apple</li><li>Peach</li><li>Orange</li></ul>

I think most people focused code styling problems and HTML/CSS specific problems but not the PHP errors themselves. I'd say $roles['foo'] must be used instead of $roles[foo] and those 'i' in for statement should be '$i'.

I'm not sure asking for the errors is particularly useful.  Syntax checkers and html validators will find many and a simple copy paste will get those.  

I'd ask for it to be rewritten in the candidate's best style and making use of best practices.

Of all the errors in that piece of code, the only one I'd rate as significant is the unfiltered use of $_GET, everything else could be overcome by asking the developer to familiarise themselves with your organisation's coding standards - in an evening or two.

How come the job description isn't available in english? ;)

Chris, the idea is to have an easy way to find a skilled and experienced PHP programmer.

An experienced programmer should be able to point out the things that might cause problems in bigger projects or when the product has to run on different targets. And there are not only PHP errors/style problems, there are also errors in the created HTML and the (mis)use of CSS.

It's a very basic and simple task. I don't expect people to remember the specific syntax of a command for example. I'd expect an experienced PHP programmer to find those problems regardless of any CosmoCode specific coding standards.

Unfortunately this seems to be too much for some candidates already.

BTW. speaking German is an important requirement for the job, that's why the description is not available in English.

What if $_GET['User'] isn't defined in the titles array? This should be checked first.

I agree with xarumanx that this practice is not good but without even checking the input against the hash, it's very bad.

One thing that comes to mind that I'd criticize (and many PHP people would probably disagree) is that the PHP code and the HTML are not separated. No matter how small the application is, separating both things makes finding bugs and working in teams easier (especially if some people from the team are "only" designers, not coders).

The obvious criticism that there is no comment whatsoever goes without saying.

Just a few things that come to mind (yeah I'm late to the party so all the good things had already been found ;) ).

I think the only error not already mentioned is the style error.

The font element must have the font-size and the font-family.

And the order is very important. Most browsers won't show what you want if you mix it up.

Well, I think nobody mentioned this: It would be faster (in a large project, that is) to use '' consistently. Also, one could have simply written:

echo "<li><!-- ... -->$list[$i]<!-- ... --></li>";

But, of course, you should for better speed use '.$list[$i].'
Other things: I would use a configuration for the role definitions, but again, that's more for bigger projects.

But the ultimate sin is: Times New Roman is damn ugly!

I think that we've found everything now. :)

Well, you may read my response to your code 'challenge' at my site ;)

cu, w0lf.

I'm bored, and feel like summing all the errors up:

1.) Missing XML declaration
2.) Missing DOCTYPE
3.) Missing default namespace declaration
4.) in style section, font should be font-family, and it should also contain serif to fall back to;
5.) Short open tags are bad.
6.) $roles[andi] and $roles[detlef] should probably be $roles['andi'] and $roles['detlef']
7.) In the whole script, there is no need to use ", ' should be used in stead to increase performance.
8.) align=left in img tag misses quotes.
9.) img tag not closed
10.) img tag missed obrigatory alt property
11.) <H1> and </H1> should be <h1> and </h1>
12.) <?= is an incorrect open tag
13.) Just placing $roles[$_GET['user']] doesn't do anything unless you echo it.
14.) no space before the closing tag after $roles[$_GET['user']]
15.) The $list array is created and then has it's values filled in in an very performance-costfull way. It should be: $list = new Array('Apple','Peach','Orange');
16.) The two PHP blocks should be joined to improve performance.
17.) Using for on an array like this is terrible for readability. Foreach should be used instead.
18.) Missing <ul> or <ol> block.
19.) Missing </li> closing tag.
20.) <b><i> should be <strong><em> and should be closed in reverse order: </em></strong>

Furthermore, I'm not really sure why the user is grabbed from $_GET... Whould make more sense if it was grabbed from $_SESSION.

Argh, wasn't paying attention at point 15. $list = new Array() should of course be $list = array().
I must have been coding too much Java/ActionScript lately. ;-)

21.) align="left" in img tag is deprecated in (X)HTML strict. Should be: style="text-align: left"

Yogarine, points 12 + 13 are wrong - <?= expression ?> is shorthand for <? echo expression ?>. See here:
http://www.php.net/manual/en/l … syntax.php

I'm not a php developer, I just program http://www.faceresearch.org in my spare time, but I am shocked you can't find professionals who can fix this. Here's my try. Sorry for any typos, I did it one-handed while breastfeeding my baby.


<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
"http://www.w3.org/TR/html4/str … d">;
<html>
<head>

<!-- Ideally, put styles in a linked .css file -->

<style>
 body { font: "Times New Roman"; }
 .fruitlist { font-weight: bold; font-style: italic; }
 .logo { align: left; }
</style>

</head>
<body>

<?php
   $roles = array(
      'andi'   => 'Developer',
      'detlef' => 'Boss'
   );

   $position = (array_key_exists($_GET['user'], $roles)) ? $roles[$_GET['user']] : 'You';

?>

<img src="logo.gif" class="logo">

<h1>Hello <?php echo $position; ?>!</h1>

<?
   // set array, I would give it a more useful name, like $fruits
   $list = array('Apple', 'Peach', 'Orange');

   // display $list in a list
   echo '<ul class="fruitlist">\n';
   foreach ($list as $fruit) {
       echo '    <li>'.$fruit.'</li>\n';
   }
   echo '</ul>\n\n';
?>

</body>
</html>

I am a new comer to PHP. I'm actually a "desktop" programmer as opposed to web one. But recently I had to convert some of my skills so I could run a web site. So here are my two pennies for this piece of code:
Just as a rapid overview, the code lacks comments and a consistent way of writing: like coding standard (this can sometimes be only cosmetic but can greatly increase code read-ability during maintenance phase).
Then with a second look, the code lacks "semantic". I mean if a software would read this code and would have to "speak" it to a blind user, the software won't have enough information to describe the page. Technically speaking, at least the 'img' tag should have an 'alt' attribute.
Furthermore, the HTML written is a mixture of pre- and post-CSS eras (I do not know how to call them, but it almost looks like web page code of the 90s). This lacks consistence and quality ==> a source of problem during maintenance. E.g.: <img src="logo.gif" align=left> the use of 'align=left' I think is deprecated. It would be better to use a class and to define in one place (e.g. the CSS file) the style for this class. Especially this kind of coding should not be mixed with CSS. If someone has to do later an evolution, he might be surprised at how the changes do not apply to all objects, if some are using global CSS instructions and some aren't.
Then for typo mistakes, 2 times the symbol '<?' is used instead of '<?php'. The <ul></ul> tags are missing. No error check is performed for the $_GET statements. I do not think '<?=' will actually work, something like '<?php echo ' will probably be a better start.

That was my advice from a C/C++ programmer ;-)

@John McCollum

I read the link and indeed, you're right. But nevertheless, <?= only works when short_open_tags is enabled, which makes it less portable than using '<?php echo'.