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.
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.
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.)
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.
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.
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
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.
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.
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.
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.
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.
On how to measure, a simple question I like to ask is:
When making a (typical) change or adding a (typical) feature to our software, is the amount of work required proportional to the scope of the change or feature?
I’m sure everyone can come up with a lot of exceptions to that rule but if typical changes requires a disproportional amount of work with regards to the scope, your architecture is bad. It may be good on certain metrics, but it fails at being viable.
In Clean Architecture, Uncle Bob alludes to the fact that if the “shape” of the architecture dictates the amount of work required rather than the scope of the features / changes, that is a sign of an inadequate architecture, which is pretty much the same thing.
alternating between saying “duh”, “hmm that’s good” and rolling my eyes
That’s refreshing. Been meeting too many programmers (of the sect SOLIDites) recently, who seem to view it as some sort of holy tome to be revered and held aloft, as unquestionably the one way true way to programmer enlightenment. Hallelujah.
The programming language you know best has been declining in popularity for a decade.
This is great news!
Everyone who bought enterprise on that language now has to pay a premium, since developers are obviously harder to get!
A large company, founded in 1997, built the initial version of their software in Perl. At the time Perl was a very popular programming language. Over the past 20 years Perl’s mindshare has shrunk: it’s harder to hire people with Perl knowledge, and there’s less 3rd party libraries being developed. So the company is stuck with a massive working application written in an unpopular language; the cost of switching to a new language is still too high.
See? I can charge 20%-200% higher on perl programming since that’s surely going to be cheaper than rewriting it!
Once you’ve identified a problem technology, you need to find a plausible replacement.
Wait why? Where problem is defined as Technologies that are shrinking in popularity; some Google searches for “$YOURTECH popularity” can help you determine this, as can Google Trends.? Absolutely not.
I had to skip back to see the issue: The language of the author switched midway between my (the programmer; who knows things and is interested in job listings), and my company (who is responsible for a project, for business, etc).
I the programmer, individual contributor do not want to switch technologies. In fact, looking for unpopular languages and at-risk companies is a good way to become a higher earner.
I the company? This is risky advice: Enterprise software that has sunk $2m in development is going to take 10 years before the 20% increase in developer salary is going to matter, and even at that point, you haven’t factored the cost of rewriting it.
Okay, but what about “without changing languages?” i.e. the “Format.js” library example? Yes, you can try the script the author suggested, but the manager is trying to justify the costs (perhaps in her head), and you’ll make everyone’s life easier if you simply justify the costs.
I the programmer, individual contributor do not want to switch technologies. In fact, looking for unpopular languages and at-risk companies is a good way to become a higher earner.
This is true… under certain conditions. Some technologies just fade into oblivion too fast to make extra money, some people aren’t good at marketing themselves or finding these niches. Some old technologies end up only used in niche geographies, and if you live in wrong place you’re out of luck.
It’s a viable strategy if you consciously set out to do it, and choose a tech stack with a conservative userbase that has deep pockets. Lots of people end up there by mistake, though, and don’t know how to get out of the hole.
Agreed. I read this trope a lot, the COBOL programmer making 1000$/hour or whatever, but I rarely meet any, probably because of that niche issue.
I however meet plenty of people who spent too much effort becoming framework experts and then really struggle to adapt as technology (or fashion) moves past them.
Institutional knowledge can seem dangerous because it doesn’t have the same value to your current employer that it will have with your next one.
Is there nothing else you have to offer? If you know Symphony but not Magneto, is there nothing you can leverage? I think this happens a lot less often than we believe, and it’s really more as itamarst suggests: a marketing problem.
I’d be surprised to meet an experienced COBOL consultant who thought the grass would’ve been greener had she done C instead, and should I meet them, I don’t think their issue looks very much like a library problem.
I’m not sure I buy the geography argument either: I commute internationally for work because I have a number of very niche skills, I like where I live, and plane travel is nothing compared to my day rate.
Marketing is a real problem though. If I could get you a 30% higher salary, would you give me 15% of it?
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.
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.
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.)
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.
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.
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
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.
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.
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.
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.
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.
Looks like Leprechauns…
https://leanpub.com/leprechauns
This should be read by every working software eng. So good.
On how to measure, a simple question I like to ask is:
When making a (typical) change or adding a (typical) feature to our software, is the amount of work required proportional to the scope of the change or feature?
I’m sure everyone can come up with a lot of exceptions to that rule but if typical changes requires a disproportional amount of work with regards to the scope, your architecture is bad. It may be good on certain metrics, but it fails at being viable.
In Clean Architecture, Uncle Bob alludes to the fact that if the “shape” of the architecture dictates the amount of work required rather than the scope of the features / changes, that is a sign of an inadequate architecture, which is pretty much the same thing.
Reading Uncle Bob’s Clean Architecture and alternating between saying “duh”, “hmm that’s good” and rolling my eyes.
That’s refreshing. Been meeting too many programmers (of the sect SOLIDites) recently, who seem to view it as some sort of holy tome to be revered and held aloft, as unquestionably the one way true way to programmer enlightenment. Hallelujah.
This is great news!
Everyone who bought enterprise on that language now has to pay a premium, since developers are obviously harder to get!
See? I can charge 20%-200% higher on perl programming since that’s surely going to be cheaper than rewriting it!
Wait why? Where problem is defined as Technologies that are shrinking in popularity; some Google searches for “$YOURTECH popularity” can help you determine this, as can Google Trends.? Absolutely not.
I had to skip back to see the issue: The language of the author switched midway between my (the programmer; who knows things and is interested in job listings), and my company (who is responsible for a project, for business, etc).
I the programmer, individual contributor do not want to switch technologies. In fact, looking for unpopular languages and at-risk companies is a good way to become a higher earner.
I the company? This is risky advice: Enterprise software that has sunk $2m in development is going to take 10 years before the 20% increase in developer salary is going to matter, and even at that point, you haven’t factored the cost of rewriting it.
Okay, but what about “without changing languages?” i.e. the “Format.js” library example? Yes, you can try the script the author suggested, but the manager is trying to justify the costs (perhaps in her head), and you’ll make everyone’s life easier if you simply justify the costs.
This is true… under certain conditions. Some technologies just fade into oblivion too fast to make extra money, some people aren’t good at marketing themselves or finding these niches. Some old technologies end up only used in niche geographies, and if you live in wrong place you’re out of luck.
It’s a viable strategy if you consciously set out to do it, and choose a tech stack with a conservative userbase that has deep pockets. Lots of people end up there by mistake, though, and don’t know how to get out of the hole.
Agreed. I read this trope a lot, the COBOL programmer making 1000$/hour or whatever, but I rarely meet any, probably because of that niche issue.
I however meet plenty of people who spent too much effort becoming framework experts and then really struggle to adapt as technology (or fashion) moves past them.
Institutional knowledge can seem dangerous because it doesn’t have the same value to your current employer that it will have with your next one.
Is there nothing else you have to offer? If you know Symphony but not Magneto, is there nothing you can leverage? I think this happens a lot less often than we believe, and it’s really more as itamarst suggests: a marketing problem.
I’d be surprised to meet an experienced COBOL consultant who thought the grass would’ve been greener had she done C instead, and should I meet them, I don’t think their issue looks very much like a library problem.
I’m not sure I buy the geography argument either: I commute internationally for work because I have a number of very niche skills, I like where I live, and plane travel is nothing compared to my day rate.
Marketing is a real problem though. If I could get you a 30% higher salary, would you give me 15% of it?