24

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?

7
  • 11
    There are no dumb questions, only dumb answers. Hell, any question can be appropriate during a code review. If someone asks me "how does f internally work?" and I don't know it, I tell them exactly this, and expect them to accept this.
    – Doc Brown
    Sep 19, 2023 at 11:17
  • 1
    Regarding point 2: Usually you should have a separate process to define which 3rd party libraries are allowed in the code-bases, since there are many points to consider (license, maintenance, security etc.) Sep 19, 2023 at 11:30
  • 1
    Perhaps you are more concerned about human factors than technical ones.
    – Pablo H
    Sep 19, 2023 at 13:00
  • 2
    One thing you can do (depending on the size of the team) is make sub-teams of people who have that specialty. For instance we had a subteam of "people who understand the CI/CD setup" and a ways of working agreement that all PRs that touched the CI/CD had to be reviewed by one of those people. If you do that though, make sure it's not some BS cool kids club: we gradually grew membership of that sub team from 3 people up to 6 as we did knowledge transfers. Sep 19, 2023 at 13:42
  • 1
    I value "obvious questions" or feedback as an important way to understand how and where to make a codebase more readable and accessible for future maintenance. Anyone, a non-domain expert or a junior developer, is a potential contributor.
    – StuperUser
    Sep 19, 2023 at 16:39

7 Answers 7

73

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.

8
  • 13
    Worth mentioning that code has no single ownership. There's no "my code and your code". It's reasonable that any team member ask the code writer about the algorithm. It's in the benefit of all the team that everyone understands the code they work on. The key is on never approaching code reviews as we were a sort of Caesar (thump up/down) who decides the fate of every single line of code others write.
    – Laiv
    Sep 19, 2023 at 6:40
  • 13
    "Worth mentioning that code has no single ownership. " Worth mentioning that this is very team-specific. Some teams/organizations practice "strong code ownership", where one person is responsible for each part of the code, others practice "collective code ownership". Personally, I believe in collective ownership, but find out first how your team does it.
    – sleske
    Sep 19, 2023 at 7:53
  • 10
    @sleske: You're not wrong that it happens in reality, but if "bus factor" means anything to you it should be clear that "one person is responsible for each part of the code" is a massive liability and overall bad idea. It should be advocated against if this is the current status of the team.
    – Flater
    Sep 19, 2023 at 12:41
  • 4
    Strong ownership... has to be interesting to know what happens when the strong owner leaves or get fired.
    – Laiv
    Sep 19, 2023 at 16:11
  • @Rik D I think a bit of my concern is what would happen in github, like endless conversations. It doesn't happen often, but it might happen. Sep 20, 2023 at 2:22
38

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.

2
  • 10
    This. Kid or Senior: You can always find bugs or mishaps in some code, even if you have no clue what it's doing on a domain level. Might not be ideal, but better than nothin.
    – Martin Ba
    Sep 19, 2023 at 13:23
  • @MartinBa fully agree. An in-depth review is better than cursory review, but a cursory review is still a lot better than no review, and more eyes on code are always better. Some people say "but why do I need a review from someone who doesn't understand the problem?" The answer is: "to see why the problem is hard to understand". Even responses about required updates/additions to in-code documentation or comments are valuable, regardless of what some people think. Non-expert reviewers are expert at finding hard-to-understand or badly-documented-and-completely-opaque stuff.
    – user213769
    Sep 21, 2023 at 11:48
14

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.

3
  • 1
    I like this answer. Maybe one bit where I disagree is on You should not expect developers to know about the internal behavior of all third party libraries they use. So asking about this is might not be very useful. I think here you're right, but some people want to know anyway so I wonder if that's a place for code review as well (assuming the code author or anyone else knows the answer). Sep 20, 2023 at 6:03
  • I just wanted to add an emphatic "Hear! Hear!". Code must be commented such that a non-domain expert can read and follow. Sure the base of the comments might be: "Using the bog-standard XXX algorithm ...".
    – Kingsley
    Sep 21, 2023 at 1:22
  • 1
    @user8469759 - most of the time, I'd consider questions about third-party internals to be off-topic for the code review. It doesn't mean they're wrong to ask about it, but I'd be looking to take that conversation elsewhere, letting the code review remain focused on the code being reviewed. Sep 21, 2023 at 2:25
11

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.

3
  • 1
    Likewise... as an architect, I prefer to get my code reviewed by developers who are smart but less experienced, because if they don't understand the architect's work, they'll ask questions because they want to learn from the architect. And that works out great, because nine times out of ten, they learn something that makes them more useful to me... and the tenth time, they've found something that I got wrong. Sep 21, 2023 at 2:21
  • my experience is quite the opposite. people who are not experts tend to tell only typos and grammar mistakes. quite rarely i get useful feedback from them :( Sep 21, 2023 at 8:49
  • 1
    @BЈовић - that's why in my comment, I specified "smart but less experienced". The value comes from having a junior developer who's keen to learn from his more experienced peers, because they'll ask those questions. If they're not interested in learning, I'd agree that their reviews are all but useless. Sep 26, 2023 at 4:40
9

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.

7

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.

4

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.