the right person for code review is scarred

merlyn on 2005-02-03T14:20:08

The right person to do code review is not just someone who has programmed a lot of successful projects, but someone who has broken enough things and gotten the scars to prove it.

For example:

if (ref $this_thingy eq 'Foo::Bar') { ... }

is just obviously wrong to me and worth flagging, because I got burned once by having to subclass the Foo::Bar for part of the application. Similarly,

SELECT * FROM ...;

is very obviously bad SQL to me, because I managed to break an application when I accidentally deleted a column and then re-added it, changing the order of the columns.

Scars. Wear them proudly.

---

Scars wiki

davebaker on 2005-02-03T15:05:28

Good point!

I wonder if it would be worthwhile to have Perl programmers contribute their scar stories to a wiki or other sort of interactive web page ... a kind of "traps for the unwary" list for learners (which is everybody <g>).

depends on context

gav on 2005-02-03T15:22:41

This isn't necessarily anything wrong with using SELECT *, relying on the order of columns returned is the problem.

I remember reading somebody saying that you should always use an ORDER BY clause in SQL, which is useless in the cases where order doesn't matter.

Sometimes order does matter, sometimes it doesn't, it's about using the right tool for the job.

Re:depends on context

jwinter on 2005-02-03T15:47:29

This guy's absolutely right. You shouldn't be depending on the order of the columns returned, you should address the values by the columns. There's different ways to do that: named bind variables, returning each row as a hash keyed by its column, etc.

Re:depends on context

Ovid on 2005-02-03T16:14:11

I know that I've said results returned should always use "ORDER BY" and I was (again) being stupid. I was pissed off at a programmer failing to sort his results properly and when I get annoyed, I say stupid things.

In that case, the programmer eventually sorted the data, in Perl. That's usually stupid when the database can handle it. In our case it resulted in a bug whereby the data always had to be sorted but sometimes we were getting it before it had been sorted. The problem (and this is merlyn's point), is that logic should be applied as soon as it reasonable. If you only need name and rank, spell it out explicitly as in SELECT name, rank.... If you think you'll need all the columns, spell out all the columns. Then if someone renames "rank", your code will instantly break at the database level rather than some mysterious error deep in the application.

SELECT * ... is usually a hack for lazy programmers and it hampers the poor maintenance programmer who has no idea what type of data is being returned.

In a similar vein

VSarkiss on 2005-02-03T15:55:38

I used to tell my clients to be suspicious of any consultant who came in like "a knight in shining armor".

You want the knight whose armor is old, rusty, and has plenty of dents and dings. Shows they've fought real battles.