Code reviews are one of the most useful parts of the development lifecycle. There are many different approaches to code reviews, and figuring out...
For further actions, you may consider blocking this person and/or reporting abuse
Hi Collin, tthis all depends on your development process. We dont have a dedicated QA team so rely on the developers and our account team for QA. We find that developers who are familiar with the code anticipate bugs better and catch them earlier. This means that by the time the account team see any work it is generally bug free and so saves them time. I think this can be the case with a QA team also, you want to foster an environment where your developers strive to not produce any bugs at QA stage, the team should work together to ensure the work is bug free before going to QA. C.R should be seen as a way for your developers to help improve each other and the system. That is, after all, their job.
It is not uncommon to get push back from senior developers on this, but if the rest of your team are behind it and drive it. It becomes part of the process and they won't have a choice. We integrate it into our definition of done, so no piece of work goes outside the dev team until it is fully reviewed, tested and unit tested.
In my team, we see the creation of good unit tests as part of our culture. At first it was hard to get on that discipline harness. Now, in our most complex library we have over 800 UT that run within 20 seconds (93% code coverage). The feedback on any change is great. It took some time but it is worth the work. Check out Michael Feathers' book "Working Effectively with Legacy Code"
Great to hear more people writing unit tests. :)
In my team, we have a git-hook set for pre-commit where we run lint and unit tests. If either of those fail or the coverage threshold was not met, then the dev will have to fix the issues before trying again to commit. We also use a tool called jest-coverage-ratchet, which updates our coverage threshold after a successful commit.
Our team and organization goal at Build.com has been to reach 80% branch coverage. I have personally set a goal for myself as well, or more like a rule that I will write enough unit tests to meet the 80% branch coverage for any file I touch when working on a particular feature/bug. For non-complex files, I usually try to meet 100%.
Positive feedback does benefit the code and the developer. If you see a pattern or refactor that makes the code better pointing it out to the dev helps highlight it.
As you have pointed out code reviews can be really rough emotionally for devs both new and experienced and the positive comments really help negate that.
That being said great article! I like the point about running the tests as part of the review.
I certainly would not discourage highlighting good solutions to reinforce good practise, however it is fairly common for code review comments to be mostly constructive. While I would not discourage positive feedback, I was using the point to lead onto how to foster a positive environment without developing an envronment where your developers expect constant positive praise in a code review, as that is just not productive.
Thanks for the positive feedback john. Implementing a strict CR process on small personal projects is hard but pays off in the long run. My advice is to lock your github repo master branch so you cannot push to it or merge without an approve CR on your PR. That will help you enforce goid practise.
As for reconsiling your technical differences, id suggest a few things. First off agree between you what a CR is going to include, then make a checklist. Dont go into too much detail. Some of the other comments have good suggestions for this. Ensure you are both manually and automatically testing the code, beyond that. Just do what you can. You will learn a lot from both having your code reviewed and feom reviewing others. Use it as a learning tool you both can benefit from.
I agree wholeheartedly with everything said in the article. This article is especially great to prepare you when you are about to introduce a Code Review process in your team.
For my last team, I've written a small online tool to be used for the mentioned checklist. It is very easy and efficient to handle and includes a "hierarchy" of things, so that only parts of the checklist need to be filled out that are relevant to the code under review.
You can have a look at it here:
guess-what.io/c
Here is an example checklist for code reviews:
guess-what.io/c/2DglcWlYmE6ITeHAlB...
I hope that others will find it helpful and maybe use it on a daily basis, too.
Thanks daniel, this sounds really great, will definitely check it out! Thanks for the feedback!
Mike, thanks for the article. I can't agree more, more eyes == less bugs.
To show some choice to the readers, I'd like to add that Gerrit is (IMHO) much superior for the code review process than GitHub pull requests. It is also free (both as in speech and as in beer), can be hosted internally, and there are already public SaaS allowing to use it, for example GerritHub.
Surely it has its twist for a git workflow it requires, and takes some time to get used to, but it really pays off IMO.
As for another set of tips for code review, take a look here (and immediately figure out what project I'm involved in :) )
Also, one must CI all things. Manual testing is flawed because it is 'manual', and humans are fallible. Have a decent unit test suit, a functional test suit and (if being a part of a bigger picture) an integration test suit - and run them automatically at least before merging, but better on each iteration, and let the CI vote on changes too.
Thanks Pavlo, I have not heard of Gerrit before. Will check it out! I like that Github has improved their code review stuff recently, coupled with all the other fab features it just seemlessly fits into many peoples current development processes, but its great to have alternatives! Just glad we are not back in the days where we had to do it on paper or word! Thanks for the other article too!
We struggle with the same thoughts. If budget and time were no issue we probably would have one, however based on the frequency (or infrequency) of serious bugs found in our application and the speed at which we are able to fix them currently it would not be cost effective to have even a single QA tester at the moment. Although we would still have our developers test their/each others code before going to QA.