9 ms·
No real insight is offered here, just a list of "givens": 1. Keep the changes small and focused 2. Ensure logical coherence of changes 3. Have automated
by edw519 8y ago
No real insight is offered here, just a list of "givens":
1. Keep the changes small and focused
2. Ensure logical coherence of changes
3. Have automated tests, and track coverage
4. Self-review changes before submitting for peer review
5. Automate what can be automated
6. Be positive, polite and respectful
Who could argue with these?
I've always believed there are 2 levels of code review, defined by their deltas:
1. Review against standards, defined by the delta between the code and shop standards, which should be well defined for everyone ahead of time. Things like formatting, white space, variable naming, conditionals, iterations, standard modules, etc. Every shop has (or should have) a standard way of doing a lot of these things. Much of this level of code review can be automated.
2. Review against expectations, defined by the delta between the code and the technical specs. Of course this requires technical specs which many shops don't do. If you write technical specs, and peer review the specs, agreeing that this is what is expected, then this level of code review becomes much more straight forward. This includes things like business rules, logic, data base updates, UX, interfaces and APIs with other apps, etc. This is much harder to automate, but offers a checklist to streamline the review and keep it objective. It also makes User Acceptance Testing easier down the line because code review has already eliminated a large subset of potential UAT rejections.
I'd love to hear how others handle these 2 levels of code review. Now that would be much more valuable than this list of feel good platitudes offered by the OP.
- docker_up 8y agoYes, I agree completely with your #1 completely. First thing before you do any sort of code review is that you NEED to have a well defined coding standard. This allows for rules of engagement. If you have no coding standard, then what happens is one person will say one thing, the other will say another, and the submitter is caught between two people giving contradictory comments based on opinion, not fact. Having a single set of rules of engagement levels the playing field completely and makes it easier for everyone.
- ionforce 8y agoPeople love to write write write code and don't give a shit about size or how it impacts reviewers.
- ponyous 8y ago> Who could argue with these? Maybe not argue, but I still find this link useful. I will link it to more junior engineers in my team. Believe it or not, not everybody does self-review before they ask for peer review.
- misterman0 8y ago>> Believe it or not, not everybody does self-review That's not hard to believe given the stream of shit that comes out of a feed, any feed. But I'm appalled by y'all's assumption that #3 is a given. Why do you all believe that? To me, including a test suite in your code base is a code smell.
- ponyous 8y agoWell depends, having 0 testing sucks too. I would argue 100% TDD can be a code smell, especially if you try to unit test everything. Use testing where it makes sense. Got some complex business logic in some function with several different edge cases? Cover them in test cases, it will also serve as documentation. If you have a problem with #3 you might as well have the problem with #5. You don't want to automate everything, only where it makes sense.
- misterman0 8y ago>> Use testing where it makes sense. I agree! >> Got some complex business logic So complex the only one who understands it is...who? A test?
- volkk 8y agoi'm not sure what you're arguing here. if you have a complicated codebase you write tests so that when you add other code, you have a bit more confidence that you're not breaking things. furthermore, if a new person joins a team, they can use old tests to understand what the hell is happening in the code otherwise you just have an unadulterated mess of code to read through. you're dealing with humans on a day to day basis that err very easily. the dumb mistakes i've made because there was a lack of tests are countless.
- sanketsaurav 8y agoHi, author of the post here. Thank you for your feedback. The framework that you put down, of code reviews being of these 2 levels, is valid. The goal of this blog post, however, was to touch on the basic hygiene which makes the process more effective -- including both these two levels of review. I would take a shot at writing a follow-up post to this, going deeper into the subjective aspects of peer code review. Thanks for reading!
- daveFNbuck 8y agoThese are not givens. They're things that have to be drilled into newer devs.
- pelasaco 8y agoboth points can be automated. Code review are more important for things that we aren't able to automate (yet)