1. 17
  1.  

  2. 25

    I used to do the things listed in this article, but very recently I’ve changed my mind.

    The answer to reviewing code you don’t understand is you say “I don’t understand this” and you send it back until the author makes you understand in the code.

    I’ve experienced too much pain from essentially rubber-stamping with a “I don’t understand this. I guess you know what you’re doing.” And then again. And again. And then I have to go and maintain that code and, guess what, I don’t understand it. I can’t fix it. I either have to have the original author help me, or I have to throw it out. This is not how a software engineering team can work in the long-term.

    More succinctly: any software engineering team is upper-bound architecturally by the single strongest team member (you only need one person to get the design right) and upper-bound code-wise by the single weakest/least experience team member. If you can’t understand the code now, you can bet dollars to donuts that any new team member or new hire isn’t going to either (the whole team must be able to read the code because you don’t know what the team churn is going to be). And that’s poison to your development velocity. The big mistake people make in code review is to think the team is bound by the strongest team member code-wise too and defer to their experience, rather than digging in their heels and say “I don’t understand this.”

    The solution to “I don’t understand this” is plain old code health. More functions with better names. More tests. Smaller diffs to review. Comments about the edge cases and gotchas that are being worked around but you wouldn’t know about. Not thinking that the code review is the place to convince the reviewer to accept the commit because no-one will ever go back to the review if they don’t understand the code as an artifact that stands by itself. If you don’t understand it as a reviewer in less than 5 minutes, you punt it back and say “You gotta do this better.” And that’s hard. It’s a hard thing to say. I’m beginning to come into conflict about it with other team members who are used to getting their ungrokkable code rubber stamped.

    But code that isn’t understandable is a failure of the author, not the reviewer.

    1. 7

      More succinctly: any software engineering team is upper-bound architecturally by the single strongest team member (you only need one person to get the design right) and upper-bound code-wise by the single weakest/least experience team member.

      Well put – hearing you type that out loud makes it incredibly apparent.

      Anywhoo, I think your conclusion isn’t unreasonable (sometimes you gotta be the jerk) but the real problem is upstream. It’s a huge waste when bad code makes it all the way to review and then and then needs to be written again; much better would be to head it off at the pass. Pairing up the weaker / more junior software engineers with the more experienced works well, but is easier said than done.

      1. 4

        hmm, you make a good point and I don’t disagree. Do you think the mandate on the author to write understandable code becomes weaker when the confusing part is the domain, and not the code itself? (Although I do acknowledge that expressive, well-structured and well-commented code should strive to bring complicated aspects of the problem domain into the picture, and not leave it up to assumed understanding.)

        1. 3

          I think your point is very much applicable. Sometimes it takes a very long time to fully understand the domain, and until you do, the code will suffer. But you have competing interests. For example, at some point, you need to ship something.

          1. 2

            Do you think the mandate on the author to write understandable code becomes weaker when the confusing part is the domain, and not the code itself?

            That’s a good question.

            In the very day-to-day, I don’t personally find that code reviews have a problem from the domain level. Usually I would expect/hope that there’s a design doc, or package doc, or something, that explains things. I don’t think we should expect software engineers to know how a carburetor works in order to create models for a car company, the onus is on the car company to provide the means to find out how the carburetor works.

            I think it gets much tricker when the domain is actually computer science based, as we kind of just all resolved that there are people that know how networks work and they write networking code, and there’s people who know how kernels work and they write kernel code etc etc. We don’t take the time to do the training and assume if someone wants to know about it, they’ll learn themselves. But in that instance, I would hope the reviewer is also a domain expert, but on small teams that probably isn’t viable.

            And like @burntsushi said, you gotta ship sometimes and trust people. But I think the pressure eases as the company grows.

            1. 1

              That makes sense. I think you’ve surfaced an assumption baked into the article which I wasn’t aware of, having only worked at small companies with lots of surface area. But I see how it comes across as particularly troublesome advice outside of that context

          2. 4

            I’m beginning to come into conflict about it with other team members

            How do you resolve those conflicts? In my experience, everyone who opens a PR review finds their code to be obvious and self-documenting. It’s not uncommon to meet developers lacking the self-awareness required to improve their code along the lines of your objections. For those developers, I usually focus on quantifiable metrics like “it doesn’t break anything”, “it’s performant”, and “it does what it’s meant to do”. Submitting feedback about code quality often seems to regress to a debate over first principles. The result is that you burn social capital with the entire team, especially when working on teams without a junior-senior hierarchy, where no one is a clear authority.

            1. 2

              Not well. I don’t have a good answer for you. If someone knows, tell me how. If I knew how to simply resolve the conflicts I would. My hope is that after a while the entire team begins to internalize writing for the lowest common denominator, and it just happens and/or the team backs up the reviewer when there is further conflict.

              But that’s a hope.

              1. 2

                t’s not uncommon to meet developers lacking the self-awareness required to improve their code along the lines of your objections. For those developers, I usually focus on quantifiable metrics like “it doesn’t break anything”, “it’s performant”, and “it does what it’s meant to do”. Submitting feedback about code quality often seems to regress to a debate over first principles.

                Require sign-off from at least one other developer before they can merge, and don’t budge on it – readability and understandability are the most important issues. In 5 years people will give precisely no shits that it ran fast 5 years ago, and 100% care that the code can be read and modified by usually completely different authors to meet changing business needs. It requires a culture shift. You may well need to remove intransigent developers to establish a healthier culture.

                The result is that you burn social capital with the entire team, especially when working on teams without a junior-senior hierarchy, where no one is a clear authority.

                This is a bit beyond the topic at hand, but I’ve never had a good experience in that kind of environment. If the buck doesn’t stop somewhere, you end up burning a lot of time arguing and the end result is often very muddled code. Even if it’s completely arbitrary, for a given project somebody should have a final say.

                1. 1

                  The result is that you burn social capital with the entire team, especially when working on teams without a junior-senior hierarchy, where no one is a clear authority.

                  This is a bit beyond the topic at hand, but I’ve never had a good experience in that kind of environment. If the buck doesn’t stop somewhere, you end up burning a lot of time arguing and the end result is often very muddled code. Even if it’s completely arbitrary, for a given project somebody should have a final say.

                  I’m not sure.

                  At very least, when no agreement is found, the authorities should document very carefully and clearly why they did take a certain decision. When this happens everything goes smooth.

                  In a few cases, I saw a really seasoned authority to change his mind while writing down this kind of document, and finally to choose the most junior dev proposal. And I’ve also seen a younger authority faking a LARGE project just because he took any objection as a personal attack. When the doom came (with literally hundreds of thousands euros wasted) he kindly left the company.

                  Also I’ve seen a team of 5 people working very well for a few years together despite daily debates. All the debates were respectful and technically rooted. I was junior back then, but my opinions were treated on pars with more senior colleagues. And we were always looking for syntheses, not compromises.

              2. 2

                I agree with the sentiment to an extent, but there’s something to be said for learning a language or domain’s idioms, and honestly some things just aren’t obvious at first sight.

                There’s “ungrokkable” code as you put it (god knows i’ve written my share of that) but there’s also code you don’t understand because you have had less exposure to certain idioms, so at first glance it is ungrokkable, until it no longer is.

                If the reviewer doesn’t know how to map over an array, no amount of them telling me they doesn’t understand will make me push to a new array inside a for-loop. I would rather spend the time sitting down with people and trying to level everyone up.

                To give a concrete personal example, there are still plenty of usages of spreading and de-structuring in JavaScript that trip me up when i read them quickly. But i’ll build up a tolerance to it, and soon they won’t.