You'd have to combine it with some risk measures of the code. An automated scan that flags high risk changes. Example, potentially infinite recursion...that would get flagged. Any changes that are boilerplate just get automatically hidden from the feed. Formatting only changes, hide em. Someone committed a 2000 line file that was not autogenerated? Push it into the feed. And so forth
When I hear/read “If we don’t review code before it’s integrated we’ll never review it.”, I have to wonder if they really see any value to the code review process. If the only way to make them do the process is to make it a blocking operation, then it sound to me like those doing and managing the work lack motivation and cost justification to do it just for the alleged value of it. This strikes me as "crisis-driven management" -- getting things done by making them a crisis. And not by demonstrating their value.
It’s keeping some of the value of code reviews, but not all - at least in my understanding of code reviews.
Imo code reviews should be synchronous: two devs sitting together discussing a piece of code. If, however, code review is only asynchronous, CodeFeed could indeed fully replace them.
The important aspects I see missing in asynchronous code reviews is the discussion, tue learning, the understanding a different approach.
If possible, I’d always opt for pair or mob programming. Besides other benefits it also has a built-in code review. Am I too naive? Would that miss other benefits of code reviews?
A code review should be done by a 3rd party. We used to have a policy that only senior engineers can make releases (in a CI/CD env.) and no developer is allowed to release their own code. This puts the onus on another team member, and if something goes wrong they and the code developer(s) get together and do a code review. Wasting 3 developer's time stands out, when it could have been avoided by the releasing developer having done a proper review. We also used to make them wear extravagant wigs while fixing the problem: we had a very open confrontation policy.
I love the idea and I see how this could improve workflows. I need to be very careful though not to think too much about it, as I would put away all my current stuff and start playing it. 😂
Yes, if I still worked with a team I'd do that. Currently working alone with trunk based model, so it has no relevance for me. But, knowing myself it would not stop me to forget what I'm doing and jump into experimenting on an open source project on github. Unfortunately right now I don't have the energy for that.
An incremental step might be to reframe what a code review is. Some go on mythic quests of "code quality" which really means, structure it the way I would structure it. To make it less of a battleground, it could be reframed as asking:
1. What is the intention of the change?
2. Does the code match the intention of the change?
3. Are there any bugs or unintended consequences of the change?
On a case-by-case basis, code review can go deeper into providing feedback, but the effort of the code reviewer should match the effort of the person who put the change-set up. A specific example is when in a mentorship position with the specific individual.
Matching effort must also factor in timeliness of code reviews. If it takes a week for you to review a change-set, and you have blocking feedback, then that should be seen as a failure of your responsibility to provide timely reviews.
On the other hand, if someone gives you a large change-set. Then it should be understood that it will take you longer to review. Again a principle of matching effort. Change-set size should be measured in the complexity of the change, and coupling becomes an important factor in this.
I don't think that pairing is a full replacement for code reviews (though it has other benefits). Even code reviews with the other dev sitting next to you are worse in my experience because you don't take yourself the same amount of time, patience and concentration/focus that you would apply while being alone. The only question is if the better review is worth the drawbacks.
Also, information like "the structural change you made is spreading to other systems" might be even hard to detect by other developers, especially external ones who are not part of the team (like you proposed), let alone AI. Maybe there is even some implicit intent of your change violated that you yourself haven't been aware of or didn't know others were unaware of. If people are not able to manually fake such a system, it looks to imagined for me... Not saying that I wouldn't like to use it if it existed ;) Especially as it allows to follow changes without participating in each review.
I'm wondering if it would be a more realistic approach to separate the main/trunk from the notion of releasable/done.
If everyone could integrate into mainline without a review (either directly or after automated ci-checks) but the system would still keep track of which commits have been reviewed, you should be able to remove part that is blocking development.
You could then bundle up commits from mainline into a review (or other devs would pick them themselves) and have a classical review of it (other dev taking their time etc). Only reviewed commits can be released and thus the underlying issues would only be done when all commits referencing it have been reviewed. Reflecting the actual progress. This would keep the benefits of reviews and would allow existing processes and compliance-foo to stay as they are while unblocking developers from waiting for code to arrive at mainline and removing incentives to create large branches etc. Additionally, tools for such a workflow exist (at least jetbrains has one where you can create reviews for arbitrary commits of the mainline).
I'm wondering which actual drawbacks would remain.
More I am reading this more I am being confused! Non-blocking code review 🤔 can someone boil this down to a few words and say what are we trying to achieve by this? Saving Time?
A good repository with good rules about commit comments would go a long way here. As well as a programmer who understands VCS very well, and how to be able to summarize changes.
Right, I agree. I just mean a VCS can tie a developer to a code change. How do es the system know whose code made the structural change? Every piece of code has to be committed by the developer who made the change. So that gives you the who, and the what can be gleaned from the commit messages if you have a good practice, i.e. The "x" class was changed to do "y" based on the reported issue "z". Then you can tie the who to the what and the why.
But much of the time, who cares? The code is what it is. We need to change it.
If someone has a lesson to learn from an earlier decision, fine, but the idea that we absolutely must know who to blame is high costs with few benefits.
I wouldn't use the information in this case as a source of blame. I might want to assign a task to the original developer because they may have more familiarity with it, and thus could make any necessary changes faster. Code is code is code, so anyone can fix it, but if it's a structural change it'd be nice to understand the developer's mindset to ascertain why they went a certain route.
As for me, that would not work in any way. My team appreciates code reviews, because they provide knowledge sharing and allow catching many misconceptions at very early stage. Still, the motivation to review merge requests tends to be low. Begging like "guys, let's merge it, those changes are dependencies for XYZ" is often required.
Loosening that necessity, that time constraint and therefore pressure, would deteriorate the idea of code reviews to minimum or even zero.
How do you solve the organizational incentives needed to implement a nonblocking code review? Once the code is merged, engineers can either pay off tech debt or launch higher profile items. The prioritization is handled by the PMs who themselves appreciate the tech debt less.
1. Right now the reviewing engineer can prioritize tech debt via his power to block a merge, so all tech debt feedback is being prioritized. By using an async framework like CodeFeed, it takes away his power.
2. Eng does have slack because that work is currently illegible to the PM, but if you make that work legible and then put it in the hands of a non eng...it can lead to hosing.
I experimented with this at a previous job as head of eng and found that engineers also would prioritize their own feature work over tech debt cleanup and without sufficient incentive to, would effectively pollute the codebase with debt to ship their deliverable faster.
1. sounds like authority without responsibility to me. Are everyone's incentives aligned on this--product, management, submitter, reviewer? If not, you'll see folks routing around this process at best, malicious compliance at work.
If you are one of the people with this power & don't want to give it up, my heart goes out to you.
2. again with incentive alignment. I'm working on a piece on sufficiency thinking, which seems foundational.
Yeah, incentive misalignment is the adverse condition I'd like to explore as in my experience it's been the norm :|. Is there any standard literature on it?
Right now I'm a 1 man CTO so it doesn't impact me just yet, but once I build up a team again, being prepared seems like a good idea
While pairing you can plan an idea that sounds like a go-forward plan to both of you, but it is easy to tie in some unintentional bias. Some developers choose to pair with each other because they share similar thoughts about the way things should be, in that in itself is inherent bias. Unless you have random pairing, you're always open to bias.
TDD is a great way to break bias, because it's purely factual. You can't call a class because the class doesn't exist, so you create the minimal class, then the test passes, so you write a test that reads from a method on the class, which doesn't exist, so...and on.
I find bias a lot more in planning and design. It still exists in coding, no matter what, but TDD makes it far more difficult.
You'd have to combine it with some risk measures of the code. An automated scan that flags high risk changes. Example, potentially infinite recursion...that would get flagged. Any changes that are boilerplate just get automatically hidden from the feed. Formatting only changes, hide em. Someone committed a 2000 line file that was not autogenerated? Push it into the feed. And so forth
🎯
When I hear/read “If we don’t review code before it’s integrated we’ll never review it.”, I have to wonder if they really see any value to the code review process. If the only way to make them do the process is to make it a blocking operation, then it sound to me like those doing and managing the work lack motivation and cost justification to do it just for the alleged value of it. This strikes me as "crisis-driven management" -- getting things done by making them a crisis. And not by demonstrating their value.
Nice idea.
It’s keeping some of the value of code reviews, but not all - at least in my understanding of code reviews.
Imo code reviews should be synchronous: two devs sitting together discussing a piece of code. If, however, code review is only asynchronous, CodeFeed could indeed fully replace them.
The important aspects I see missing in asynchronous code reviews is the discussion, tue learning, the understanding a different approach.
If possible, I’d always opt for pair or mob programming. Besides other benefits it also has a built-in code review. Am I too naive? Would that miss other benefits of code reviews?
A code review should be done by a 3rd party. We used to have a policy that only senior engineers can make releases (in a CI/CD env.) and no developer is allowed to release their own code. This puts the onus on another team member, and if something goes wrong they and the code developer(s) get together and do a code review. Wasting 3 developer's time stands out, when it could have been avoided by the releasing developer having done a proper review. We also used to make them wear extravagant wigs while fixing the problem: we had a very open confrontation policy.
I love the idea and I see how this could improve workflows. I need to be very careful though not to think too much about it, as I would put away all my current stuff and start playing it. 😂
You can play with this without writing a single line of code. Take last week's PRs, sort them this way & that, then write up The News.
Yes, if I still worked with a team I'd do that. Currently working alone with trunk based model, so it has no relevance for me. But, knowing myself it would not stop me to forget what I'm doing and jump into experimenting on an open source project on github. Unfortunately right now I don't have the energy for that.
An incremental step might be to reframe what a code review is. Some go on mythic quests of "code quality" which really means, structure it the way I would structure it. To make it less of a battleground, it could be reframed as asking:
1. What is the intention of the change?
2. Does the code match the intention of the change?
3. Are there any bugs or unintended consequences of the change?
On a case-by-case basis, code review can go deeper into providing feedback, but the effort of the code reviewer should match the effort of the person who put the change-set up. A specific example is when in a mentorship position with the specific individual.
Matching effort must also factor in timeliness of code reviews. If it takes a week for you to review a change-set, and you have blocking feedback, then that should be seen as a failure of your responsibility to provide timely reviews.
On the other hand, if someone gives you a large change-set. Then it should be understood that it will take you longer to review. Again a principle of matching effort. Change-set size should be measured in the complexity of the change, and coupling becomes an important factor in this.
I don't think that pairing is a full replacement for code reviews (though it has other benefits). Even code reviews with the other dev sitting next to you are worse in my experience because you don't take yourself the same amount of time, patience and concentration/focus that you would apply while being alone. The only question is if the better review is worth the drawbacks.
Also, information like "the structural change you made is spreading to other systems" might be even hard to detect by other developers, especially external ones who are not part of the team (like you proposed), let alone AI. Maybe there is even some implicit intent of your change violated that you yourself haven't been aware of or didn't know others were unaware of. If people are not able to manually fake such a system, it looks to imagined for me... Not saying that I wouldn't like to use it if it existed ;) Especially as it allows to follow changes without participating in each review.
I'm wondering if it would be a more realistic approach to separate the main/trunk from the notion of releasable/done.
If everyone could integrate into mainline without a review (either directly or after automated ci-checks) but the system would still keep track of which commits have been reviewed, you should be able to remove part that is blocking development.
You could then bundle up commits from mainline into a review (or other devs would pick them themselves) and have a classical review of it (other dev taking their time etc). Only reviewed commits can be released and thus the underlying issues would only be done when all commits referencing it have been reviewed. Reflecting the actual progress. This would keep the benefits of reviews and would allow existing processes and compliance-foo to stay as they are while unblocking developers from waiting for code to arrive at mainline and removing incentives to create large branches etc. Additionally, tools for such a workflow exist (at least jetbrains has one where you can create reviews for arbitrary commits of the mainline).
I'm wondering which actual drawbacks would remain.
More I am reading this more I am being confused! Non-blocking code review 🤔 can someone boil this down to a few words and say what are we trying to achieve by this? Saving Time?
A good repository with good rules about commit comments would go a long way here. As well as a programmer who understands VCS very well, and how to be able to summarize changes.
The real deal here is IMO Information like “The structure change you made last week is spreading to other systems.”
That would never be included in a commit msg and probably the other dev diesn't even know/recognize it.
Right, I agree. I just mean a VCS can tie a developer to a code change. How do es the system know whose code made the structural change? Every piece of code has to be committed by the developer who made the change. So that gives you the who, and the what can be gleaned from the commit messages if you have a good practice, i.e. The "x" class was changed to do "y" based on the reported issue "z". Then you can tie the who to the what and the why.
But much of the time, who cares? The code is what it is. We need to change it.
If someone has a lesson to learn from an earlier decision, fine, but the idea that we absolutely must know who to blame is high costs with few benefits.
I wouldn't use the information in this case as a source of blame. I might want to assign a task to the original developer because they may have more familiarity with it, and thus could make any necessary changes faster. Code is code is code, so anyone can fix it, but if it's a structural change it'd be nice to understand the developer's mindset to ascertain why they went a certain route.
As for me, that would not work in any way. My team appreciates code reviews, because they provide knowledge sharing and allow catching many misconceptions at very early stage. Still, the motivation to review merge requests tends to be low. Begging like "guys, let's merge it, those changes are dependencies for XYZ" is often required.
Loosening that necessity, that time constraint and therefore pressure, would deteriorate the idea of code reviews to minimum or even zero.
Maybe. Maybe not.
How do you solve the organizational incentives needed to implement a nonblocking code review? Once the code is merged, engineers can either pay off tech debt or launch higher profile items. The prioritization is handled by the PMs who themselves appreciate the tech debt less.
Difficult problem. Misaligned incentives.
1. How much of that work actually gets prioritized now? Would CodeFeed make the problem worse?
2. If the programmers have no slack time, everybody is already hosed (see also Kingman Formula).
3. This is, in fact, the topic of book 3 in the series. Which means I haven't written it yet. Which means I don't fully understand it.
Try some experiments & see what happens.
1. Right now the reviewing engineer can prioritize tech debt via his power to block a merge, so all tech debt feedback is being prioritized. By using an async framework like CodeFeed, it takes away his power.
2. Eng does have slack because that work is currently illegible to the PM, but if you make that work legible and then put it in the hands of a non eng...it can lead to hosing.
I experimented with this at a previous job as head of eng and found that engineers also would prioritize their own feature work over tech debt cleanup and without sufficient incentive to, would effectively pollute the codebase with debt to ship their deliverable faster.
1. sounds like authority without responsibility to me. Are everyone's incentives aligned on this--product, management, submitter, reviewer? If not, you'll see folks routing around this process at best, malicious compliance at work.
If you are one of the people with this power & don't want to give it up, my heart goes out to you.
2. again with incentive alignment. I'm working on a piece on sufficiency thinking, which seems foundational.
Yeah, incentive misalignment is the adverse condition I'd like to explore as in my experience it's been the norm :|. Is there any standard literature on it?
Right now I'm a 1 man CTO so it doesn't impact me just yet, but once I build up a team again, being prepared seems like a good idea
There's generally applicable material but not much past what I'm writing applied to software development.
While pairing you can plan an idea that sounds like a go-forward plan to both of you, but it is easy to tie in some unintentional bias. Some developers choose to pair with each other because they share similar thoughts about the way things should be, in that in itself is inherent bias. Unless you have random pairing, you're always open to bias.
TDD is a great way to break bias, because it's purely factual. You can't call a class because the class doesn't exist, so you create the minimal class, then the test passes, so you write a test that reads from a method on the class, which doesn't exist, so...and on.
I find bias a lot more in planning and design. It still exists in coding, no matter what, but TDD makes it far more difficult.