We don’t have time for code reviews • Hacker News Board

Re: We don’t have time for code reviews

#11

Post by masklinn » Wed, Oct 23, 2013, 1:55 PM UTC

benihana wrote: post #7

eloisant wrote: post #4

Earlier quoted context omitted.

I don't know, code review without running the code is a very very light variant, too light to call it code review I believe.

Code reviews are about psychology. When you know your teammates are going to be reviewing your code, you write it differently than you do when you know no one but you will ever look at it. Static analysis and regression tests are tools to make sure the code isn't broken.

> Code reviews are about psychology.

Code reviews are also about distance from the code. When it's your production it's easy to miss the forest for the trees. That's why books are generally better when beta readers or reviewers are involved.

Code reviews are also about spreading knowledge (about the subsystems and about choices made in implementation and the reason for them) and increasing the code's bus factor.

Re: We don’t have time for code reviews

#12

Post by benihana » Wed, Oct 23, 2013, 1:56 PM UTC

mabbo wrote: post #3

I once pushed un-reviewed code that contained alert("Fuck"); It made it to a single user who was testing it for me, on the other side of the planet. He spoke very little English, and for that I am grateful. That was my last push without review.

I think it's hilarious (and typical) that the moral of the story is "I don't push without code reviews" not "I don't use bad words in test code."

Re: We don’t have time for code reviews

#13

Post by edw519 » Wed, Oct 23, 2013, 2:10 PM UTC

Proper code review provides much more benefit than cost and is indispensible for quality software. Improper code review is a waste of time (or worse).

Proper code review is done:

  - by another programmer
  - by someone with some knowledge of the application
  - by someone with some knowledge of the environment
  - against some code standard
  - against standard requirements (APIs, database, etc.)
  - with a checklist
  - uniformly (no matter who is doing it)
  - until it's right

Improper code review is done:

  - by a non-programmer
  - fully automated
  - as a "rubber stamp" on an approval checklist
  - against the standard du jour
  - according to the whims of the reviewer
  - with a deadline for the next step
  - as a replacement for testing

Re: We don’t have time for code reviews

#14

Post by jcromartie » Wed, Oct 23, 2013, 2:11 PM UTC

benihana wrote: post #12

mabbo wrote: post #3

I once pushed un-reviewed code that contained alert("Fuck"); It made it to a single user who was testing it for me, on the other side of the planet. He spoke very little English, and for that I am grateful. That was my last push without review.

I think it's hilarious (and typical) that the moral of the story is "I don't push without code reviews" not "I don't use bad words in test code."

Professionalism is for people who wear shirts with collars and have nice cars and families. You know, the kind of people who fund the kind of place where you can use bad words in code...

Re: We don’t have time for code reviews

#15

Post by junto » Wed, Oct 23, 2013, 2:14 PM UTC

benihana wrote: post #12

mabbo wrote: post #3

I once pushed un-reviewed code that contained alert("Fuck"); It made it to a single user who was testing it for me, on the other side of the planet. He spoke very little English, and for that I am grateful. That was my last push without review.

I think it's hilarious (and typical) that the moral of the story is "I don't push without code reviews" not "I don't use bad words in test code."

   "I don't use bad words in test code."

This is actually really important. We had this happen with a customer whilst UAT testing. The project schedules were under pressure and as a result the customer went nuts (rightly so) and we had to make some concessions to customer as amends.

It isn't just curse / bad words. Equally as bad are comments like:

  "this was the customer's stupid idea not ours"

Also, if you really have to output test stuff in the browser use console.log('message') and not alert('message').

Then for production you can always do this at the top of your main JavaScript include:

  var console = {};
  console.log = function(){};

Re: We don’t have time for code reviews

#16

Post by joeblau » Wed, Oct 23, 2013, 2:14 PM UTC

mabbo wrote: post #3

I once pushed un-reviewed code that contained alert("Fuck"); It made it to a single user who was testing it for me, on the other side of the planet. He spoke very little English, and for that I am grateful. That was my last push without review.

My comments are more along the lines of alert("GOT HERE 1"); alert("GOT HERE 2"); alert("GOT HERE 3");

Not very helpful if someone else sees it, but also not profanity.

Re: We don’t have time for code reviews

#17

Post by jchendy » Wed, Oct 23, 2013, 2:15 PM UTC

edw519 wrote: post #13

Proper code review provides much more benefit than cost and is indispensible for quality software. Improper code review is a waste of time (or worse). Proper code review is done: - by another programmer - by someone with some knowledge of the application - by someone with some knowledge of the environment - against some code standard - against standard requirements (APIs, database, etc.) - with a checklist - uniforml…

Are code reviews by non-programmers actually common? How can that possibly work?

Re: We don’t have time for code reviews

#18

Post by junto » Wed, Oct 23, 2013, 2:17 PM UTC

mabbo wrote: post #3

I once pushed un-reviewed code that contained alert("Fuck"); It made it to a single user who was testing it for me, on the other side of the planet. He spoke very little English, and for that I am grateful. That was my last push without review.

As I said in another thread, always use console.log('message') and not alert('message').

Then for production you can always do this at the top of your main JavaScript include:

  var console = {};
  console.log = function(){};

Re: We don’t have time for code reviews

#19

Post by ohwp » Wed, Oct 23, 2013, 2:22 PM UTC

jchendy wrote: post #17

edw519 wrote: post #13

Proper code review provides much more benefit than cost and is indispensible for quality software. Improper code review is a waste of time (or worse). Proper code review is done: - by another programmer - by someone with some knowledge of the application - by someone with some knowledge of the environment - against some code standard - against standard requirements (APIs, database, etc.) - with a checklist - uniforml…

Are code reviews by non-programmers actually common? How can that possibly work?

I've seen 'code reviews' that were in fact functionality checks. So it's more like a 'check if this code works'.

And I can tell you: a web form capable of SQL-injections is still 'working code'. So it gets a positive 'code review'.

Re: We don’t have time for code reviews

#20

Post by myke_cameron » Wed, Oct 23, 2013, 2:24 PM UTC

junto wrote: post #18

mabbo wrote: post #3

I once pushed un-reviewed code that contained alert("Fuck"); It made it to a single user who was testing it for me, on the other side of the planet. He spoke very little English, and for that I am grateful. That was my last push without review.

As I said in another thread, always use console.log('message') and not alert('message') . Then for production you can always do this at the top of your main JavaScript include: var console = {}; console.log = function(){};

I think a lot of people like using alert() for debugging because it posses execution. Of course, so do break-points / "debugger" statements.

Git pre-commit hooks for alert are helpful since few and far between are the circumstances where an alert is actually wanted in production. While you're at it you can add one for whatever set of profane terms, if you roll like that.