Developer using Agile

[VectraMan]

Code reviews are common and make sense to me

I've always heard that, but I've never seen it actually happening in my 20 years of doing this professionally. YMMV.

As with the source control issue, if you need to be checking up on your developers you've got much bigger problems.

[eek]

Don't know whether it was classed as an agile environment, but at one client I worked with, one guy worked over the weekend re-writing my code because I'd used the ANSI JOIN syntax rather than the Oracle (+) notation.

The reason given was that this was complicated enough, so having a shorter query (in terms of characters used) would be better for performance....

The great thing about Toad and Sql developer is that you can see what is actually run. I seem to remember that the join syntax is both more readable and less costly.....

[TheFaQQer]

The great thing about Toad and Sql developer is that you can see what is actually run. I seem to remember that the join syntax is both more readable and less costly.....

Performance impact is pretty negligible. But given that the query was 50+ lines long, across multiple tables, and parsing XML in there as well, I thought that making it readable might be a bonus. I pointed out the Tom Kyte article on how much better it was, but that made no difference.

He billed two days overtime to work the weekend to rewrite it in his style, and the client then congratulated him on how hard he'd been working and made him "Contractor of the Week" (complete with little trophy and everything)

I managed a couple more months (couldn't give notice) before I turned down the extension and ran from the place as fast as I could.

[d000hg]

I've always heard that, but I've never seen it actually happening in my 20 years of doing this professionally. YMMV.

As with the source control issue, if you need to be checking up on your developers you've got much bigger problems.

Checks should always be in place. The most thorough of testing can fail to catch something a 5-min coding review might miss. Unless your entire team is incredible people things will get stuffed up. And most people who think they're incredible aren't

It's part of basic QC in my view.

[VectraMan]

Checks should always be in place. The most thorough of testing can fail to catch something a 5-min coding review might miss. Unless your entire team is incredible people things will get stuffed up. And most people who think they're incredible aren't

It's part of basic QC in my view.

But how much meaningful detail can someone go through in a 5 min coding review? If a developer has spent a week on a big complicated system, then a 5 min review, or even a much longer review isn't going to identify corner cases, race conditions, or other ways it can possibly fail to any meaningful degree. Which means it just ends up being a box ticking exercise, or ends up being about stupid things like names of variables.

For it to really work it would take so much time you'd need two people doing the job of one, which probably has its merits too, but most companies don't have that much money to burn.

[SpontaneousOrder]

But how much meaningful detail can someone go through in a 5 min coding review? If a developer has spent a week on a big complicated system, then a 5 min review, or even a much longer review isn't going to identify corner cases, race conditions, or other ways it can possibly fail to any meaningful degree. Which means it just ends up being a box ticking exercise, or ends up being about stupid things like names of variables.

For it to really work it would take so much time you'd need two people doing the job of one, which probably has its merits too, but most companies don't have that much money to burn.

In ten years I've never worked anywhere that didn't do code reviews (apart from the first 18 months where I was the only employee).

With that being said, especially in an 'Agile' team, people should really be pairing - there are very few kinds of work that benefit from 'Agile' in the first place, which also wouldn't benefit from pairing up (like exploratory coding or suchlike).

When pairing is going on I personally don't think there should then be any further formal code review. Especially because, leading bad to the OP's question, you should be pairing at a fairly granular level (user story max) and the pairing should rotate around the team so that everyone is constantly working with everyone else.
Then the team should also be rotated around the different areas of the system being built so that there are no silos of expertise.

If you do this then coding style shouldn't be an issue. If you're really an agile team then it normally makes little sense to be precious about your code anyway because someone else will most probably be changing it later anyway.
Secondly, who cares about coding style? I know a lot do but I personally think they shouldn't. Braces on the same / new line - fine. A common unit test naming convention might be nice, but not really important in my opinion.

Different styles mean more learning opportunities. And in any good team the better styles will be adopted in favour of the inferior ones by osmosis.
Again this is where pairing helps - if your way is objectively better, then you should be able to explain why. It's hard to argue with someone who's given you a good reason for their preference.

But how much meaningful detail can someone go through in a 5 min coding review? If a developer has spent a week on a big complicated system, then a 5 min review,

Assuming the OP is working in a typical Scrum flavour of 'Agile' (although even for other paradigms) then he wouldn't be getting a week's worth of code reviewed. You really want to be checking something in coded/tested/reviewed/done every day. Certainly no where near a weeks worth of work. I prefer to go for twice per day (lets say a 1 day story split into 2 technical tasks - maybe 1 is business logic & the other is DB access).

Like I say though, pair up and rotate pairs and don't bother with formal reviews (unless there's some kind of regulatory pressures in that respect).

[woohoo]

In ten years I've never worked anywhere that didn't do code reviews (apart from the first 18 months where I was the only employee).

With that being said, especially in an 'Agile' team, people should really be pairing - there are very few kinds of work that benefit from 'Agile' in the first place, which also wouldn't benefit from pairing up (like exploratory coding or suchlike).

Is it just me but does the thought of pairing with someone send shivers up your spine. I really couldn't think of anything worse, I like to spend a little time thinking about the problem in my head not sure how that would work with pair programming.

[eek]

Is it just me but does the thought of pairing with someone send shivers up your spine. I really couldn't think of anything worse, I like to spend a little time thinking about the problem in my head not sure how that would work with pair programming.

What I've never got is why would you pay twice as much to get something done...

I can see the justification for doing it occasionally to bring someone up to speed but continually?

[woohoo]

What I've never got is why would you pay twice as much to get something done...

I can see the justification for doing it occasionally to bring someone up to speed but continually?

Yeah occasionally its quite good to work with someone on something, if its new to you or they have a lot of knowledge in this area. But all the time, no way. I'm sure I read an article that rubbished the productivity claims of pair programming though im sure people in favour of PP would be able to produce articles to support their views.

[d000hg]

What I've never got is why would you pay twice as much to get something done...

I can see the justification for doing it occasionally to bring someone up to speed but continually?

Maybe it doesn't cost twice as much. A typical developer only spends something like 1-2 hours writing code on a given day, or something like that.