Code review from domain non expert

Question

My team has a mixture of specialties, there's some overlap however. When some commits are done from person A (who is expert in some domain) and person B (who is not expert in that domain) I wonder if it is appropriate to ask in depth questions about the code or is not a good practice or to what extent we should allow to have conversation in code review about the code itself.

Imagine the following situations:

1. An algorithm which is only known by A has been checked in. Can B ask clarifications in the code review?
2. A third party library has been used for a specific function f and some code using it has been checked in by A. Can B ask why is f useful? how about more details about f such as "how does it internally work?".

Are questions like these appropriate in code review?

Answer 1

Code reviews are not only useful to find potential errors, but also to transfer knowledge between developers. It is totally appropriate to ask domain specific questions, ask for reasons why problems were solved the way they are, etc. Explaining something often also leads to new insights, so it’s really an opportunity to learn in multiple ways.

If the knowledge gap between the writer of the code and the reviewer is big, a synchronous review where both look at the code together on a single screen works better then an asynchronous review using some tool.

Answer 2

When I was a kid (21 or so) I looked over some programmer’s shoulder, pointed at the screen, and said “there’s a bug”. He protested loudly, I had no idea what the code was doing etc. My mate heard it, took a look, and said “no, he is right, there’s a bug”. And there was a bug.

You don’t need to be a domain expert to see if something is wrong. You won’t find all problems, but you will find plenty.

In the case of an algorithm that only A understands, it must clearly be accompanied by documentation to let someone else take over if A is hit by a bus or wins the lottery. And a non-domain expert can check that.

Answer 3

An algorithm which is only known by A has been checked in. Can B ask clarifications in the code review?

The external behavior of the algorithm should be documented with comments it it is not obvious from just reading the signature. If the internal implementation is complex it should likely also be documented, possibly by referencing wikipedia, some research paper, or other source. So B should not just ask for clarifications, but ask for these clarifications to be added to the code.

It is possible, and even likely, that developer C will be asked to do something with the code many years after it was written, long after both A and B has left the company. So ensuring readability is very important.

A third party library has been used for a specific function f and some code using it has been checked in by A. Can B ask why is f useful? how about more details about f such as "how does it internally work?".

You should not expect developers to know about the internal behavior of all third party libraries they use. So if asking about this you might want to make clear that an answer is not expected. The external behavior of the library should be documented, so hopefully B should be able to check why it is useful himself. But if there is anything unclear, ask for clarifications.

Keep in mind that it is really difficult to determine if an unfamiliar algorithm is implemented correctly just by reading code. This should instead be proven by unit tests whenever possible. So the code review might instead focus on checking if the code is readable and understandable, code standard is followed, if there are typos or other simple mistakes etc.

Also keep in mind what knowledge is expected from your developers. If you are writing a graphics engine it is reasonable to expect knowledge about matrices, vectors etc. But if the domain is narrow you should likely require a higher standard of documentation, since you might not be able to find a replacement if your domain expert leaves. This is especially important when using code from researchers, since there might only be a handful of experts in their particular field in the world.

Answer 4

I actually prefer it when people who aren't domain experts participate in code reviews. In my experiences, those code reviews tend to give the most meaningful results.

As one example, I once was part of a review for a change that added support for a new piece of hardware that had a rather complicated interface. I was familiar with this particular hardware, but New Guy was also participating and knew practically nothing about it. The code's author and I traded comments back and forth about how the handshake mechanism was implemented, whether such-and-such part of the data packet was optional, etc. New Guy, on the other hand, had no idea what any of this code was actually supposed to be doing. Despite that, he ended up finding multiple low-level coding issues (indexing errors, unnecessary memory allocations, etc) that the rest of us wouldn't have noticed because we were focused on the higher-level functionality. New Guy's lack of domain expertise meant he was not distracted by the big shiny new technology and was able to look at the code from a completely different point of view.

We've had many such cases where reviewers with different domain expertise make very significant contributions. They've noticed code that's similar to something in their domain (which the rest of us are unfamiliar with) and that can be combined and refactored into a separate, reusable module. Our electrical engineer (who normally does the low-level assembly stuff and works upwards) can identify when code written by our programmers (who handle the high-level design and work downwards) would result in assembly that our chip architecture would not be able to handle efficiently.

Different points of view - including different domain expertise - are one of the most valuable things to have in a code review. You want your code examined from all angles. You tend to forget a lot about your own code once you've been away from it for several months. If someone without domain knowledge finds your code confusing, then you'll likely also find it confusing 6 months from now. Even if their questions don't result in any code changes, the fact that the questions and their answers are now logged in your code review system make this a valuable artifact for anyone trying to understand the code down the road.

Answer 5

Have you defined the purpose of a code review? In another answer here on Software Engineering, I wrote about reasons why you'd want to conduct a code review.

If the purpose of the code review is to find defects, weaknesses, vulnerabilities, or opportunities for improvement, then you would likely need someone who is knowledgeable in the domain and technology to provide the code review. Someone who is less knowledgable may be able to find areas to improve the readability and understandability of the code through their questions, but would be less likely to find errors or potential problems.

If the purpose of the code review is to develop a shared understanding of the system and cross-train on technologies or design decisions in various modules or components, then it's valuable to include people who may not be as knowledgable and use the code review as an opportunity to transfer knowledge. In these cases, questions for clarifications or how things work or why things were done in a certain way are far more useful.

You may also want to consider the method of a code review. A knowledgable person reviewing someone else's code for problems or opportunities for improvement could be done asynchronously. However, knowledge transfer is often best done synchronously, so you may want to set up time to have the people involved walk through the code together.

Answer 6

The best test of the readability of the code is how well the clueless understand it. If only the one who wrote it can follow it they've created a time wasting trap for everyone who comes along later.

Yes, we need to prioritize what we spend time on. But the closest thing we have to a silver bullet is readability.

If it's buggy, but readable, I can fix it.
If it's slow, but readable, I can fix it.
If it's a memory hog, but readable, I can fix it.
If it's not readable I have to go ask people, "what was this supposed to do?"

So aim to make it readable. Even for the non-domain expert.

Answer 7

A third party library has been used for a specific function f and some code using it has been checked in by A. Can B ask why is f useful?

Absolutely. Part of the purpose of a code review is to challenge technical decisions. Is it really justified to add yet another third party dependency? Perhaps there's something in the standard library (or one of the other existing dependencies) that would do the job.