how to perform code review among different types of developer?

Question

My develper team includes Android, iOS, PHP, Web Front, .Net developers, but every type of developer only 1 person, and they don't know other's programming languages very well.

We have setup Jenkins with some static analysis tools such as Sonarqube, but it's not enough to find out some logic mistakes.

Is there any suggestion on performing code review in this kind of team? Thanks a lot!

Answer 1

Ask ten different developers the purpose of "code review" and you will get eleven different answers. You can see that plainly amongst the answers and comments for this question. I agree with several of the comments thus far... but I also disagree with several others. Examples:

• A code review is not just for static code analysis.
• Testing is not necessarily where you will find most of the errors.

So before talking about how to perform code reviews my first suggestion is to get a better understanding of the purpose of a code review. I am just going to give my own perspective here, and I do not expect everyone to agree, but I have made it a passion of mine for several years to apply the "5 W's and an H" to code review (with some liberties and with a grateful nod to Aristotle -- see https://en.wikipedia.org/wiki/Five_Ws)

• what is code review?
• why do we code review?
• how do we code review?

And perhaps most importantly:

• how do we maximize the effectiveness of code review?

I have written several articles covering these questions (plus ancillary discussion in several other articles); thus you can surmise that, to me, there is no short answer here -- my comments here just scratch the surface.

In a nutshell: the purpose of code review is to improve the quality of the code we deliver.

Now testing (from unit tests through system tests as well as ad hoc testing) shares that same purpose, so you should not think of code review as a substitute for testing (of any kind). They work well together.

There are lots of other benefits of code review, too--this from a work in progress:

So the question at hand then becomes: how do we do code review with the intent of improving code quality?

My preference is to generally have at least three reviewers, consisting of the author of the code plus two others. No code is perfect. No matter how many eyes review it, it will still not be perfect. Think of a bell curve. Granted, this is a rather imperfect analogy. For one thing, we should really be talking about a one-sided distribution; two-sided is meaningless here. :sigh: But the point of this illustration is that the curve is asymptotic to the axis--it will never reach zero. Yet for the vast majority of software that is OK. If we can find 95% of the issues (two standard deviations) or 99.7% of the issues (three standard deviations) then we could call that sufficient.

I find that two reviewers beyond the author can achieve that high-nineties coverage. Applying more people severely diminishes returns; it is a waste of resources. Less people often does not catch enough. (I will say anecdotally that in a recent pull request I reviewed, two reviewers before me basically said "looks good" and then I, as yet one more reviewer, went and added 50 comments on issues that I noticed. :wink:)

So how can you do an effective code review? Ultimately your task is to answer the question: do the changes accurately and completely cover the requirements?

Here is my curated list of checkpoints to facilitate the task:

1. Understand the requirements of the issue before you dig into the code, including reading the issue and other pertinent background material.
2. If there are UI changes, was a screenshot included?
3. Compare requirements against unit tests (every requirement should be tested)
4. Compare unit tests against the code under test (all code should be tested)
5. Review the unit tests as an ensemble (to look for weaknesses, ambiguities, or gaps)
6. Is each unit test named well? (unambiguous, accurate, complete)
7. Does each unit test evaluate only one thing?
8. Do the unit tests have strong coverage? (equivalence class and boundary value analysis)
9. Is each unit test as simple as possible? (ideally cyclomatic complexity === 1)
10. Does the code under test follow best practices? (SOLID principles, no magic numbers, etc.)
11. Can you get by with less code? (is there a simpler way?)
12. Look beyond the changes, from the low-level (if FooBar is changed to FooBaz, are there instances of Foo-Bar as well?) to the high-level (was the README updated correspondingly?)
13. Is the code self-documenting? (that is the code itself, minimizing comments)

It is important to also add that a code review should NOT just be reading the code--it should include exercising the code as well. Automated tests, no matter how fastidious you are, are simply not sufficient to vet a UI of any complexity.

That was meant to be a rabbit-hole but was so cute I could not resist--think of it as office-sharing by a rabbit and a chipmunk...

But to directly address your question about developers who do not share common languages, I hope I have conveyed that that does not matter too much; there is much any seasoned developer can do.

Whew, I think I better stop now before I run out of bits, but here are some links from my Zen of Code Review series for further reading:

• Part 1: Pre-Review Comments
• Part 2: Authoring Best Practices
• Part 3: The Reviewer's Tale
• Part 4: Review as if you Own the Code
• The Sequel: Better Code Reviews with GIT

Answer 2

My suggestion would be to cross-train your developers so that they are able to read the code that gets written by one or two others. This means learning the basic syntax of the language and the major features of both the language and the framework you are using.

Then, your developers are at a level where they can review each other's work. For the future, you might want to go one step further to the level where everyone is able to contribute to at least two areas. They can still have their primary focus on one area, but should be able to step in if there is a real need for additional help in another area (e.g. due to an unexpected high amount of work there or the primary developer not being available for whatever reason).

Answer 3

Integration testing is where you find most non-trivial logic mistakes

• integration testing tests the code doing something useful
• unit testing mostly proves it can run and does something the developer expected, not if the developer expected the correct things
• sonar cube mostly tells you if it runs safely, within a very debatable definition of safe
• code review mostly tells you if they made an unmaintainable mess getting there

The developer exercising their own code while developing should find most of the "oops" kind of mistakes like off by one and boolean conditions, after those most mistakes are the developer misunderstanding the problem, so a unit test, or even a code review is not going to catch them as the test will be written with the same flawed premise and the code will likely do the wrong thing cleanly and succinctly.

If you don't do integration testing then your dogfood team or alpha testers serve the same purpose.

Answer 4

Step 1: Every developer checks their own code thoroughly before it is submitted.

Step 2: You pick whoever is most knowledgeable to review code. When they have problems because of different languages, the original developer can explain things.