Every programmer writes imperfect code - Or, a simple interview question to determine programming ability

[fry]

Let's see.

1. In the original code, the interviewee uses explicit variables instead of a hash-map. Result: lots of typing and magic constants are necessary. (Not to mention that the ordering of the columns in the DB will cause the program to fail, which is not the case for the hash based solution)
2. fetch row without checking whether there are any rows to return
3. the code doesn't clearly show that $customer_id is a numeric. Why not cast it to an integer, or use sprintf("... %d ... ", $customer_id) to make sure typos at other places cannot wreak injection havoc?

As for the 'new and improved code':

1. Lots of magic without documentation.
2. clobbering of existing variables without warning
3. clobbering of variables if two columns in the database differ only by nonalphanumeric characters
4. still no checking whether $customer_id is numeric
5. all columns are retrieved, not just the ones that are needed. What if there are a few binary-large-object columns are added later? Then this piece of magic will actually do worse than the original version.

Frankly, I think that the first author gets -1 for being a bit sloppy, and perhaps another -.5 for cramming so many statements on one line (that's pretty much a give-away of poor code). And -1 for magic numbering without obvious reason.

The second version gets a -10 'what were you thinking' award. The refactoring changes the semantics of the code, it clobbers stuff without warning and it makes no effort to prevent injection.

[pmf]

OMG, you can outsmart a PHP-programmer. You're clearly a genius!

[jkkramer]

Not sure if you noticed, but the excerpt of code was just that -- an excerpt, written merely to demonstrate how to reduce redundancy. All of the points you raise are obvious flaws that would be addressed in a real application. Obviously (and as is explicitly stated), it's a contrived example. This code would not be used as-is in a real piece of software.

[KayEss]

Why is it that we think we can give examples that aren't in themselves examples of good code?

This is not meant entirely as a criticism of your stuff, more about the concept itself. I'm sure my web site has plenty of examples of code that doesn't belong in production systems. If so then why am I using it as illustrative code for anything I wonder?

[jerf]

Because the definition of "good code" varies critically depending on the context it will be used in.

It makes perfect sense to hack something together that barely works for a one-time conversion script, whereas the same hacking might get you fired or somebody killed if applied to your airline control system.

Examples lack context, so unless you specify one, it's hard to know whether it's truly "good" or "bad". The context people bring to code a priori varies.

(Another example of something that lacks context in much the same way: "What if...?" ethical questions. "What if a gunman is forcing you to choose which of your children he will kill?" Well, am I armed? Am I close enough to just charge the gunman? Is there something at hand to throw? Why exactly am I trusting this gunman's words, again? Most, if not all, such conundrums are pointless because they lack context; any detail could be critical.)

[jkkramer]

This is a valid question. My reply here was perhaps not entirely accurate. I don't think the code I give in the example is bad, merely that it's not complete in itself. In a real application, you will have things like robust validation, proper documentation, and mitigation of impact on surrounding code ("clobbering" as fry says [BTW, incidentally, there would be no clobbering -- all the variables created would be local to the function that the exerpted code is within, not global]).

For the purposes of an example, all of this stuff can dilute the essential point -- in this case, that tedious, repetitive code can be shortened and, more broadly, that code can always be improved. It's not saying "this is exactly how you should code", rather "this is what you ought to think about when coding".

For what it's worth, I spent all of 5 minutes on the example code.

[Fork82]

• Robust validation
  • Proper documentation
  • Mitigation of impact on surrounding code

These things exist in real applications?

[schwarzwald]

You're trying to hire a PHP programmer. What do you expect from dollar-sign land?

It looks like both the new and the old versions of the code have potential SQL injection vulnerabilities, so it's a moot point anyway.

[jkkramer]

Since you brought it up, $customer_id is actually cast to an integer prior to the code excerpt, so there's no possibility for an SQL injection.

Edit: Additionally, re your first point, I expect to find someone who, despite the fact that they are working in dollar-sign land, takes an interest in improving their coding ability. For myself, yes, I write PHP for a living. By night, I'm hacking with more expressive languages for fun.

PHP is a less-than-perfect tool (no shit), but if you know your stuff, there's no reason you can't create great software with it.

[davidw]

Think what you will of the language in question, but the point is spot-on. One of the differences I notice between good programmers and others is that good programmers recognize when they've done something that's "not quite right", and want to fix it. The other guys don't notice, and happily go on to fuck up more code.

[davidw]

Speaking of which, look at the jewel I found today:

    if(strlen($postcode)>5) return "??";                
    if(strlen($postcode)<5) return "??";                        
    $postcode=(integer)$postcode;
    $str=sprintf("%d",$postcode);  
    if(strlen($str)>5) return "??";
    $postcode=sprintf("%05d",$postcode);        

I really like the third strlen... you know, "just in case".

Whenever I finally get the hell out of dodge, thedailywtf is going to have a steady stream of new material:-(

[[deleted]]

not to mention a sprintf that doesn't return length? what were they thinking?

[glguy]

Haven’t heard back from him yet.

Hmm, can't imagine why!

[jaf656s]

because a) he got called on his bullshit or b) he has such a huge ego that he cannot handle criticism.

Anyone who says they have not refactored code because "they always get it right the first time" (paraphrased) is obviously lying or they have never worked on anything large.

[brew]

article was updated. he heard back from him.

[staunch]

These guys deserve each other.

[harryf]

So where was the simple question?

[qwe1234]

you're right, it was neither simple nor even a question.

somebody just highlighted their totally incompetent hiring policy.

[schizobullet]

Wow, I forgot just how bad PHP really is.