Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

I feel like code reviews comments should be of several types. 1. There is a bug in this code(most important) 2. This violates the norms on the project 3. This is less maintainable because it can cause a future bug(two pieces of code in different that need to be consistent) 4. I personally had trouble understanding this code, here's what took me a while to understand. Would you mind adding a comment or temp variable to speed this up. 5. Here's a cleaner way to do it(totally in writers court whether or not they implement this unless they're a jr. dev.

I make a lot less code review comments than I used to because I've seen too many bugs introduced by fairly innocuous looking code review changes and the amount of bikeshedding that can go into code reviews comments that will likely never materially effect the application.



>code review comments that will likely never materially effect the application

I think that is actually a good bar for a CR comment. Issue of maintainability affect the application. Bugs affect the application. A lack of general good software architecture has long term effects on the application.

All other comments I either tag on an "as an option, you could..." or force myself to shut down the "that's not how I would have done it!" voice in my head and just leave no comment.

On the receiving end, few things are more obnoxious than "deck chair rearranging" comments which have no bearing on performance, maintainability, style, or correctness, but just boil down to you satisfying the benign preferences of whoever is looking at your code.




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: