Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

We don’t have time for code reviews

blog.8thcolor.com

Post reply on HN
Print view View original

51–60 of 67 posts

Re: We don’t have time for code reviews

#51

Post by jt2190 »

mberning wrote: post #34

When talking about code reviews I am constantly reminded of the following 1. How hard it is to grok code that I've never seen before 2. How hard it is to grok my own code from 6 months ago Sometimes it may take me a day or more to fully get my head around a new piece of code I am working with. I could not give a thorough AND fair review of another person's code without internalizing it to a satisfactory degree. There…

    > Sometimes it may take me a day or more to fully get 
    > my head around a new piece of code I am working with.
Are you trying to perform a design review, or a code review? Code reviews (in my mind, at least) concern themselves with the low-level mechanics of the code ("Handle this exception properly","there's a library method for this logic","follow the team's coding standards", etc.) whereas design reviews deal with the "how is this thing supposed to work?". When reviewing code, I avoid dealing with design issues. If I see design issues, I ask the developer for a design review.

Re: We don’t have time for code reviews

#52

Post by martinvanaken »

edw519 wrote: post #49
martinvanaken wrote: post #35

Earlier quoted context omitted.

Hi, OP here. Interesting list. I think I would approve most of your points, but never worked (or created) such a "proper" environments in my various teams. Could you share the kind of checklist you use? Or give an idea of the kind of "checks" you have there? Thanks!

Hi, OP. Thanks for the interesting post. Off the top of my head: 1. Variables declared or assigned, but never referenced again 2. Improper variable scope (local/global) 3. Reserved words (by language, framework, industry, app) used as variables 4. Improperly reused variable names 5. Improperly named variables (1 or 2 characters, contained in another variable name) 6. Too difficult to understand variable or function n…

Great list (especially "off the top of your head"). Thanks a lot for sharing. Several of those can actually be implemented in an automated tool (styles, short variables, spacing, unused variables even). What's your take on automated tools? (Disclaimer: my company built one - pulreview.com).

Re: We don’t have time for code reviews

#53

Post by puppybits »

hox wrote: post #48
jt2190 wrote: post #45

Personally I agree with the author that proper [1] code reviews are valuable, however, I think he's skipping over a really critical point: > We started doing Unit Testing at some point, and it > quickly became “good practice/mandatory” in our team (I > think good practices need to be applied by everyone in a > team, requiring some kind of “collective enforcement”). I'd love it if the author would elaborate more on ho…

My advice: 1. As an advocate for a practice, ensure you use that practice in all that you do. Advocate for it in every team meeting. Try to stress why it is important, and how it can directly make things better, instead of just "making our code better." Provide research and numbers from prior projects that show how useful such a practice can be. 2. Provide good examples and guides to get the practice started. Some av…

Why is much more important than what to do. The best benefits of code reviews are: 1. Developer educate each other about the code base and how to be better developers. This means quick, more stable features which means the company can start making money sooner. 2. Slows tech debit. Poorly written code stays out of the product. More up time means more overt unity to make money.

Re: We don’t have time for code reviews

#54

Post by RougeFemme »

paulbjensen wrote: post #31

You think that's bad, I once worked in a startup where the Product Manager told me he didn't have time for tests.

Not just start-ups; larger companies, too. The process may dictate testing, but if, for example, there's an urgent production bug or some enhancement that "needs" to be pushed quickly, testing often falls by the wayside.

Re: We don’t have time for code reviews

#55

Post by RougeFemme »

tonyplee wrote: post #39

I care more about full functional test coverage than code review. The tests must be automated, regression and functional coverage not just unit test. Test results/logs must output in HTML store in DB backend (now) or excel (did that in 1997) for ease of analysis. Test failures are highlight in red, passes are in green. The developers should write new test code for every new feature and run all the existing test code…

Code review, done properly, should provide more than just a critique of coding style. It should should also catch certain types of bugs before they get to functional testing. Yes, you can catch those same bugs in functional tests, but in the environments where I've worked, there's a lot more overhead (read: time, money) involved with bugs found in functional test than in code review. Ita can be pretty frustrating to be that far along (functional test) and find a bug that could've easily been found in code review.

Re: We don’t have time for code reviews

#56

Post by martinvanaken »

jt2190 wrote: post #45

Personally I agree with the author that proper [1] code reviews are valuable, however, I think he's skipping over a really critical point: > We started doing Unit Testing at some point, and it > quickly became “good practice/mandatory” in our team (I > think good practices need to be applied by everyone in a > team, requiring some kind of “collective enforcement”). I'd love it if the author would elaborate more on ho…

Hi, author here. I agree this may not be easy - but something can always be done. I've been in several situation.

As a team leader facing junior developers, I did simply set-up rules. I explained them, but I had the power to enforce them by myself. I coded with them, and they started coding like me - until they were confident enough to challenge me. I apologize to the "autonomous self organizing teams" evangelists, but in some situations, giving some direction may be the most efficient way to progress.

As a peer, you just need to find one other person in your team that is willing to play along. Although I understand the value in explaining something, doing it is for me much more convincing. If you really think something is a good practice, don't try convincing me if you are not already applying it yourself.

If you are in an Agile/sprint oriented team, just propose to test it for one sprint, then it will pass the retrospective test or it will not - no one should object to a one sprint experience, especially in an agile team. Do the same with your colleagues ideas, even if you find them silly. It shows goodwill, and you can be surprised at some time.

This is actually the way me arrived to our current workflow at 8th color (http://blog.8thcolor.com/2013/09/how-our-own-workflow-is-dri...) - successive retrospectives.

Finally I would not involve non coding management in the discussion if possible, as it will quickly devolve into "what would it cost". Better to handle this inside the technical team.

Hope it helps, and remember, it's the first step that cost. Find one willing colleague and start!

Martin

Re: We don’t have time for code reviews

#57

Post by dlitz »

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.

To avoid this sort of thing, I have an editor macro that puts a comment like this at the end of the current line:

  alert("Fuck");  // DEBUG FIXME(dlitz)
You can't see it here, but the comment also includes a few trailing spaces.

This way:

- "FIXME" gets gets highlighted in my text editor

- the whitespace triggers a git warning when committing

- I can configure a pre-push hook that refuses to push commits to origin that contain "DEBUG FIXME"

- I ever do accidentally push it, it's clear that the code was just there for debugging, and that I'm the person to talk to about it.

I also use "git add -p" to do a hunk-by-hunk review of my changes before committing them.

None of this is very hard or time-consuming, and I can't remember the last time that I accidentally pushed debugging code since I started doing this.

Re: We don’t have time for code reviews

#58

Post by martinvanaken »

atsaloli wrote: post #43
martinvanaken wrote: post #35

Earlier quoted context omitted.

Hi, OP here. Interesting list. I think I would approve most of your points, but never worked (or created) such a "proper" environments in my various teams. Could you share the kind of checklist you use? Or give an idea of the kind of "checks" you have there? Thanks!

See six steps to success section of my write-up of Dr. Holzmann's talk on how JPL builds reliable software. http://www.verticalsysadmin.com/making_robust_software/ The clever idea is to make your coding standard mechanically verifiable.

Thanks, nice list, summarize a good part of the best practices in a short form. Will quote.

Re: We don’t have time for code reviews

#59

Post by martinvanaken »

nradov wrote: post #26
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…

I also include junior developers who don't know much about the application or environment in code reviews. It's a good way for them to learn the code base and see what type of defects the senior developers point out.

+1 to that one. Code Review should be Peer Review - it is not about a Senior reviewing a Junior it is about a developer reviewing the work of another one. The junior's questions may actually be as useful as advices (notably by forcing you to explain your choices).

Re: We don’t have time for code reviews

#60

Post by wting »

mberning wrote: post #34

When talking about code reviews I am constantly reminded of the following 1. How hard it is to grok code that I've never seen before 2. How hard it is to grok my own code from 6 months ago Sometimes it may take me a day or more to fully get my head around a new piece of code I am working with. I could not give a thorough AND fair review of another person's code without internalizing it to a satisfactory degree. There…

There's also a few benefits of code review that you missed:

1. Reduces bus factor.

2. Increase readability.

Coding in a company is a communication problem. How you solve a problem may be different than someone else. Outside of simply catching more mistakes, it's possible that you may learn something new or the other person can improve their code.

Ideally there should be a team / company coding style simply to ease communication overhead and reduce bikeshedding.

Post reply on HN