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

41–50 of 67 posts

Re: We don’t have time for code reviews

#41

Post by zwieback »

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…

Tests==code. If your test code is not reviewed then you don't know if your tests are any good. Neither code reviews nor TDD are silver bullets.

Re: We don’t have time for code reviews

#42

Post by ProAm »

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…

I've found code reviews are beneficial to help you catch simple bugs, you don't always have to internalize the purpose of the code just make sure the person dotted their i's, crossed their t's and freed all their memory pointers... Or just help junior guys write more efficient code...

Re: We don’t have time for code reviews

#43

Post by atsaloli »

martinvanaken wrote: post #35
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…

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.

Re: We don’t have time for code reviews

#44

Post by hox »

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…

The cost you speak of is directly trading software quality for "speedier" delivery. Heck, this was even posted just yesterday: http://www.sec.gov/litigation/admin/2013/34-70694.pdf. One of the big problems was there was no code review being performed.

Also think of code review as a fantastic learning opportunity for the developers you are reviewing. Practicing an art can only get you so far as an expert, but being critiqued by others can help refine the rough edges you may otherwise ignore throughout your career.

Code reviews don't have to happen in massive context, either; why not follow a practice that every pull request (or whatever your SCM calls it) goes through at least one reviewer before being merged to master? And ensure your pull requests are frequent and preferably 100 lines or less!

A little bit goes a REALLY long way. Don't give in to the fallacy that your code isn't either worth it or is good enough to not be reviewed.

Re: We don’t have time for code reviews

#45

Post by jt2190 »

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 how to change team behavior like this, especially when it's a team member (peer) trying to make the change.

I've seen many teams struggle because there isn't a agreement over what practices to follow.

[1] See edw's comment on proper code reivews: https://news.ycombinator.com/item?id=6598804

Re: We don’t have time for code reviews

#46

Post by DougWebb »

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…

Test coverage is pretty orthogonal to code review. You might wind up with a large amount of fully tested freshly refactored code that does not meet requirements or edge cases the tests missed. I'd recommend at least reviewing the test code in your case.

Besides that, it sounds like a nice codebase to work in. I've had very good test coverage for libraries and backend services with well-defined APIs I've written, but a lot of my career has been spent doing UI work and I've never found an adequate testing tool that can deal with both the complexity and randomness of human interaction and the rate of change of UI presentation.

You can't really regression test UIs, because most UI development intentionally changes the things regression tests look for, so you wind up spending a lot of time updating the regression tests and trying to keep up with UI changes that are being made in a tight code/review/tweak iteration loop.

Re: We don’t have time for code reviews

#47

Post by martinvanaken »

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…

I agree with you, and this is the reason why having small features and reviews for all of them helps. Groking a colleague's code is much simple if it is a small piece, and when you are reviewing his code regularly (and the other way around).

Our typical PR is 1-2 days of work, so I'm reviewing several by week, something by days.

You are of course right that it should not fall into "you should do it my way". Now, when my colleague said "I would have done it differently", I always ask how and why. I will probably not change my code if it is good (or even good enough), but I would have learned something, or got another point of view.

Re: We don’t have time for code reviews

#48

Post by hox »

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 avoid a practice because they don't know the best way about executing a practice. If it's unit testing, show how one should go about determining edge cases and describe useful unit testing methodologies (mocks, stubs, etc).

3. Make it easy as possible. If it's unit testing, integrate a CI server directly to your SCM. If it's code review, adopt a practice that fits inline with your existing workflow and don't make it a ceremony - make it asynchronous.

4. Understand that it will be gradual, but every effort moving forward should push towards integrating the new practice. You can't suddenly have 100% test coverage after introducing unit testing to an existing project, but all code that gets included can include new tests, and old refactoring can have tests included to slowly build up the test coverage.

Re: We don’t have time for code reviews

#49

Post by edw519 »

martinvanaken wrote: post #35
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…

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 names

7. Function names must be Verb-Adjective-Noun

8. Variable names must be Adjective-Noun

9. Improper data type

10. Repeated (in one or more programs) code for which reusable code should be written. (Any future maintenance must only be done in one place.)

11. Fresh code rolled instead of using existing reusable code meant for that purpose.

12. Partially complete data base updates

13. Improper data base locking

14. Non-standard UI widgets

15. Version control problems (didn't start with current production version)

16. Not all changes documented with ticket#

17. Case/switch statements without fall-through clause (case 1)

18. Improperly structured if...then statements

19. if...then statements too long (according to published standard)

20. Improper indenting/spacing (according to published standard)

21. Comments don't match code

22. Comments inaccurate

23. Calling/using obsolete/deprecated code

24. Not backward compatible

25. Obviously works under only certain conditions (which will be caught in testing)

26. New code inserted in wrong place (if...then, case, function, etc.)

27. Obviously missing data base updates

28. Obviously missing API calls

29. What else?

Re: We don’t have time for code reviews

#50

Post by josephlord »

clubhi wrote: post #28

Pull requests are insufficient for code review. You need to run the code. I often find the code I want to review after following a path in a debugger.

Yes the diffs often don't show enough context and the errors may occur outside of the diff so you should really review the whole changed function even if you don't need the debugger.

They are always attached to a branch though so you can check it out to review (and run/test as appropriate).

Post reply on HN