On trust in software development
blog.ploeh.dk
blog.ploeh.dk
- getting peer review on iffy code = 100%
- waiting for a PR to be approved for 6 hours, by 2 parties when it's typo fixes and adding some comments = ...
As a senior dev with over 20 years, I value code reviews from even junior devs. Heck it might spark a conversation that might mean everyone learn something, or get a refresher. It's also simply put: 2 brains are better than 1, particularly with codebase fatigue.
And by the way, in dynamic languages: even more so. Dynamic languages save you time in the writing side of things, which you pay back in the testing. Make no mistake, you are the compiler.
So my philosophy is if you feel this might benefit from another set of eyes, seek reviews. If you feel you're going to waste someone's time, don't.
But mission critical code, e.g. handling payment information, user privacy impact of any kind, business logic, should go through code reviews, on top of QA and UAT.
Trust people, don't trust code. And if you can't trust your people, you need new people(!)
I'd like to expand a little bit on this part:
> I value code reviews from even junior devs.
In fact it can be really valuable. If they don't understand it it means it is either too complicated or it might mean it is time to explain the necessary concepts to them.
When a review has found problems with my work, 90% of the time I've spotted them myself during that process.
This is arguably not a problem with the review process, but a problem of not being able to maintain an asynchronous workflow.
Git itself is made from the ground up to allow you to make a change, send it out, and then go focus on something else for the next few hours. It does require some amount of learning your tools (e.g., cherry-pick and rebase), and unfortunately most of the forges that people build on top of Git are quite bad at this. (Gerrit is the best one I know for this particular problem space; unfortunately, it has some other downsides.)
My tested and recommended process is working in pairs, rotating pairs regularly and sharing/rotating all tasks within pairs, equally. You have two people actively working together on each piece of code. They are supposed to both understand what is happening, care for quality and be vigilant to catch defects. This is also the right time to have actual input regarding quality or design improvements. And the code once it is done can immediately be committed and shipped so there is no awkward delay between code finished and deployment.
I also learned over the years pair programming is also a fantastic way to achieve some other goals like people socialising, getting to know each other professionally, learning from each other, newcomers getting bootstrapped, etc.
Pull requests degrade to nit picks that are irrelevant to code quality and code function to make the reviewer feel like they are contributing
Sorry to play devil's advocate, but pair programming is literally two people doing a job that has traditionally/historically been done by one person. There is no surprise this is going to cause raised eyebrows with management. Two devs sharing a keyboard is 100% going to be seen as two people fulfilling a single person's job to nearly anyone external to the role for obvious reasons.
I happen to hate it. Tried it at multiple companies and it still makes no sense to me. You are forcing two people to do a specific thing at the same time and pace. One of them has a meeting? Sorry can't do any work. Oh the other guy has a meeting after yours is done? Can't do any work. Oh one of them has a doctor's appointment? No coding.
I prefer "the best of both worlds". Workshop/whiteboard the solution. Go separate ways if possible working on different parts of the problem. A great split is someone doing the frontend and the other doing the backend if that's the kind of thing you are working on. Or find some other way to parallelize. Reconvene after some hours/when you are done with something agreed upon or if "stuck".
Not everyone likes 100% of the time social contact. Sometimes I just really need to be left alone in a corner, head down, no distractions.
What I do is that I include pair programming as part of interview process to let people experience the process a bit before they decide to join the team. Can't do much about people who are already in the team -- I try to be flexible about it but only to some extent.
In the end, being part of large software development project is a social endeavour, whether you like it or not. I get that some people prefer to work alone but I have to insist that social part cannot be somehow ignored.
What 100% pair programming does however is to dictate exactly one way of achieving that goal. Like with any form of dictatorship, I think there is more harm than good in it.
Pair programming in an interview would not be an issue for me for example. It is something that I do in some situations. Just two days ago, I had a PR comment from someone on how to do a part of what I was doing differently - and better. That was great but I wasn't sure how exactly it would play out and hadn't used that particular way of using the framework we're using. So I pinged him and we hammered it out together in 15 minutes of pair programming. Then he dropped off the call and kept working on his task while I cleaned up the mess of copy and pasted, commented out and `blah = newFunction()` type code we created in order to quickly figure out if "the other way" of doing things would work and if it actually was better.
Coming back to the interview situation, we'd probably get along fine in pair programming that together and you'd want to hire me. And then you tell me that this is how I will be required to work 100% of the time and I'll end the interview right there.
At some point this has to result with dictating how people need to behave, what is expected of them. Unfortunately, giving people just the goal and no process to get there does not work. What does work is giving people progressively more freedom after they have shown this can at least potentially lead to better results. Taking away freedoms does not work -- people become resentful, demotivated and stop working. That's why demoting people very rarely leads to good results.
Giving entire team complete freedom can work with highly selected individuals who are very committed and motivated to achieve the goal. But then the reason this can work at all is because they organise themselves and they honour their internally established constraints. But those circumstances are rare in large companies and building those teams is extremely difficult. I am not naive to think I can do it and even then I usually am given a team rather than given free hand to choose whatever people I want at whatever price I find necessary.
I empathize with people who don't like being constrained in how they do work, I don't like it myself. But this to some extent is required to get a large organisation working. The best solution is to do your job well and show it is in everybody's interest to give you more freedom.
> And then you tell me that this is how I will be required to work 100% of the time and I'll end the interview right there.
And I think it is perfectly fine. This is the point of interviewing process -- for me to find if I can work with you and for you to find if you want to work with me. For my end I try to explain and demonstrate how it would look in reality. It costs me much more to hire somebody that will then be unhappy with the process. On a more personal level I am trying as much as I can to not hire people that I would have to soon fire. I hate firing people. And more than that I feel responsible for causing problem to a person that I could have prevented by not hiring them in the first place.
So in a sense you breaking the interview is win win for both of us.
Hopefully there would have been screening before the interview where you would learn a bit about how we work and you would leave the process with less disruption to both of us.
I think dictating exactly how people need to work is counter productive if you work with highly skilled people. I like working with people that I can give a goal and we can talk about how to achieve it best together, try some things and see if it makes it better or not. E.g. we've tried pair programming in the team and most people didn't like it at all, just like myself. I am not going to force it on them. To your point, which is very valid, if someone did that to me after the fact, I'd quit.
Personally I don't think the solution is to force every new hire to 100% pair program, while I myself would then not pair program when I do code. That sends the wrong signal. Do as I say not as I do. And yes that does mean I'm constrained in who I can hire or work with. Not everyone will "make it" on my team. E.g. as people progress I will progressively expect them to communicate about being stuck by themselves instead of me asking if any help is needed. They either learn how to do that and work in a team or they are not made for mine. A Senior Developer I need to nanny every 2 hours is not a Senior Developer.
It doesn't mean we have zero process of course. I just don't believe in a process that enforces 100% pair programming ;)
Not doing pair programming doesn't mean the social part is ignored. It only means that the social part is done separately from the keyboard work.
I can't seem to code and engage in an ongoing human interaction at the same time. It has to be one or the other. I also really hate having someone looking over my shoulder while I'm typing.
Wow, this is me. A friend once analogized it to being like a light source. I am a laser, deeply penetrating a narrow spot, but leaving the larger field in the dark while I do so. Other people are like a floodlight, illuminating a large area, but not deeply penetrating any particular portion of it.
I always though of being like a laser as a kind of superpower, though, not a pathology.
There are, naturally, advantages and disadvantages to each way of thinking. A good team needs both lasers and floodlights.
People are definitely NOT taking twice as much time per-person to write a piece of code.
First of all, code review is already a cost tacked onto development to ensure quality. So one could say code reviews are a waste of time. Well... not necessarily. It is always cheaper to catch and fix an error closer to the source of the error. When the error happens in prod it may be costing your company millions, but the same error caught in review may mean a cost of somebody's 1h review and half a day to fix it and then re-review it.
So we are saying: it is better to spend a bit of extra effort upfront than let those quality problems spew uncontrolled into codebase and production environment.
Naively, pair programming means one person writes the code and another "watches" and catches errors. But that's not what is happening. People discuss the course of action so you usually already get better result with two people doing it. Then there is the fact that if one person would get stuck it is usually much faster to get unstuck with another person involved.
And you get other effects, too. For example, I find it impossible to procrastinate when working in pair with another person. It would be just disrespectful to the other person to be wasting time while we are both assigned and expected to produce results.
All this means that not only the quality is better than if one person worked on it, but also the work progresses much faster.
I would also suggest that in many businesses the most important factor is not development efficiency per se, but how fast can you deliver a given result. Meaning, if business figures out they want capability X, how fast you can provide it.
To get to X you create a project with tasks. Any project will have critical path and adding more resources to the project will not speed up the project at all. The only way to speed up the project is to either change the critical path to a shorter one or speed up the tasks on critical path.
There is normally very few ways to speed up development tasks. You can reassign them to your best/fastest devs. You can produce more technical debt. That's about it.
So working in pairs offers an invaluable ability to speed up the development process without creating more technical debt and without using your critical resources.
And yet another effect is scaling your development operations. Normally, adding more developers to the project creates more efficiency. Twice larger development team will not be twice more efficient. Working in pairs counteracts this problem a bit, ie. it lets the team be twice as large at the same level of efficiency/complexity. Any techniques that let you stave off the effects of growing organisation are very valuable.
True. But on the flip side, I've found that it also makes it impossible to sit and think deeply about an issue at hand. The tendency is to rush, so it doesn't look like you're not participating.
I swear maybe 50% of review comments are either to make the reviewer feel like they aren't a rubber stamp or just to signal that they actually read the PR.
Code review is fine and necessary. The problem is when a coworker can veto your own work, rather than just review it and offer helpful suggestsions. Pull requests are simply an artifact of certain version control systems that have somehow become pervasive to our entire software methodology. It doesn't have to be that way.
It's not a question of trusting in perfection. Of course everyone makes mistakes. But do you trust your coworker to not be overly pedantic and opinionated, to not get in your way and block your progress? You shouldn't necessarily trust that either.
If they were, I would raise it with my line manager and try to get the issue fed back to them.
What kind of workplace do you not trust your coleagues to be reasonable?
I don't trust myself nor anyone to not make mistakes or have blind-spots that pull requests and code review helps catch.
Here's an example from today of an issue caught code review, that could have been a bug that easily slipped through testing only to become a hard debug issue months or years later:
"You're assuming the order here, but it should be set explicitly".
Caught by the reviewer, and easily fixed in seconds.
But that could have been a really expensive bug to fix later.
And it wasn't really through a lack of competence, just an oversight because the data wasn't necessarily sorted at that point.
And the gating helps enforce and audit that code is reviewed, that everyone can go back and read the reviews to understand the journey the code went through. If there's later a bug in that area, there are now two different people I can know to ask first about that code.
Making code review optional is a good way to get it waved through with little oversight.
Having the people who okay the request share responsibility for fixing that code is a good way to motivate better code reviews as well as share knowledge.
Why would a coworker want to block your progress? Maybe I've never worked anywhere toxic enough to understand.
This reads a bit weird so I'm interpreting it as "what kind of workplace causes you to not trust your colleagues to be reasonable?"
Lots. There are many places where developers will play outright power games in order to posture and preserve their position. The PR is a big spot where this can shine through in black/white. As a manager, I really have to police GitHub/GitLab/Bitbucket to ensure that this isn't happening.
I've had juniors, equals, and seniors hold up months worth of work for arbitrary reasons, purposefully pushing past deadlines. Some even admit to it behind closed doors/in meetings. Toxic work places surely exist.
---
To be clear, I am 100% in support of peer reviewed work as a SWE (PR's, external auditing, etc)... I'm just expressing that I've experienced the check/balances of peer reviewed work weaponized, typically to feign importance or as an unfair attempt to advance someone's career. I feel like it's just the human condition that some people will just take advantage.
Isn't that a strong signal that the culture is bad?
Maybe not of the entire company, but at least of the team that's happening in.
It can be - but it is also isolated to pockets/individuals within a company which doesn't necessarily define the culture.
> Maybe not of the entire company
Absolutely agree, and that's not well captured in my comment on second read. It can be at the company level, at the team level, and all the way down to the individual level - obviously in any configuration/mix. Sometimes it's not on the company that they had a hypothetical bad hire as long as they take corrective action, just to give an example.
Me: "Code review is fine and necessary."
How did you turn "necessary" into "optional"?
This seems to be just more evidence that for some reason, some people can't seem to imagine code review without pull requests. Maybe you have to be older, and worked before git and GitHub existed? I don't know.
Let me spell it out more clearly: a pull request is when the reviewer can reject your commit and force you to meet the reviewer's demands. Whereas you can have mandatory code review without pull requests, where the mandate is just that someone has to examine your commit and comment on it.
> Why would a coworker want to block your progress? Maybe I've never worked anywhere toxic enough to understand.
Have you... heard of large corporations? ;-)
You seem to be against the concept of someone having the ability to veto your submission, and that is indeed what most people think of the purpose of code review is. If it doesn't pass muster, it doesn't get merged into the main branch.
I know. My original comment said, "The author keeps equating code review with pull requests and gated check-ins."
> You seem to be against the concept of someone having the ability to veto your submission
Correct.
> that is indeed what most people think of the purpose of code review is.
I don't know about "most" people, but the people who think that are wrong. :-)
> If it doesn't pass muster
According to whom, is the question.
I've had seen code reviews done by people who are not working on the code in question and never will, and this is dysfunctional and should never happen. If that's your case then I am sorry and yes, make code-review optional, non-blocking or even better cancel them all together.
But the proper code review is done by your peers working on the same part of code. When I put a blocking comment saying "this function called GetFoo now deletes database, submits payment and returns Bar, please rename it to match its function" this is not done because I read too much books and have weird ideas... This comment was written because I know that next week I will be adding new features and debugging production failures and having bad code will actually impede my progress.
Of course people may disagree what "good" is, so there should be some way to resolve conflicts. I've seen teams with very strong technical leader who had the final word. Or the one place which had "review meetings" for author/reviewer disagreements which anyone could attend and which would vote on resolutions. Either way, make sure you have a wiki page or something where you can write down what you decide on (for example: "no review defect should complain about punctuation in comments"), so that the same questions don't come up again and again.
(Btw, if you have problems with other people reviewing your code, this is where microservices shine. You have your own repo and service, they have their own repo and service. No one touches other's code and thus has no grounds to impose code review. Just make sure to have good monitoring solution so that if main app falls down you know who's service to blame.)
The users?
> But the proper code review is done by your peers working on the same part of code.
Of course. However, that's tangential to the issue of code vetoes.
> Of course people may disagree what "good" is, so there should be some way to resolve conflicts. I've seen teams with very strong technical leader who had the final word.
That's fine, but many conflicts can be avoided by not having vetoes in the first place. If the coder refuses to fix code that the reviewer considers not shippable, then they can go to the manager to resolve the conflict. The manager always has a veto, but otherwise there's no reason for coworkers to have a veto over each other.
> if you have problems with other people reviewing your code
I don't.
Author opens a PR. Reviewer starts reviewing. The pull request merging should be blocked until review is done, if anything to prevent mistakes. As review progresses, there is a comment which author refuses to fix. Since all parties must mark review as "complete", the PR is blocked cannot be merged until the comment can be resolved. Author and reviewer go to manager, which decides on one way or another. The manager's resolution is applied -- either reviewer resolves the comment, or author rewrites PR to fix the defect.
Was there any "veto" applied? It is not like there is a button labeled "veto" during review process, it is just that some comments take longer to resolve some others.
Or is your ideal system when reviews are non-binding and author can press "merge" button at any time despite having open review comments? In this case it is pretty horrible for reviewers - code review is a very hard job and I would be pretty disheartened if the author just walks away instead of resolving discussions. I think if your review is like that, you might as well just code reviews altogether as it is a total waste of time.
Going back to what I said in an earlier comment:
"This seems to be just more evidence that for some reason, some people can't seem to imagine code review without pull requests. Maybe you have to be older, and worked before git and GitHub existed? I don't know.
Let me spell it out more clearly: a pull request is when the reviewer can reject your commit and force you to meet the reviewer's demands. Whereas you can have mandatory code review without pull requests, where the mandate is just that someone has to examine your commit and comment on it."
So let's start over:
1) Author commits to a branch.
2) Someone else reviews that commit and gives feedback.
3) If necessary, as determined by author based on the feedback, author modifies code.
This is an async process. There's no blocking.
It was OK - definitely better than nothing, but with its own problems. Biggest defects were the worst - lets say someone made a new library function and inspection found out that function should not exist at all. By the time it's time to make a fix, code might already be used. The latency was not so great -- to save time, we often requested a review of entire patch series so the time from the first commit to the first comment was pretty long.
Later on for business reasons we had to stop working on that codebase and start all new code from scratch. Everyone on the team voted to switch to "pull review + code review before commit" and we never looked back.
Thinking back about this, the main reason to use post-inspection was that the tooling sucked. Back then we had SVN and branches were super heavy, the easiest way to share code was to email files around, and combining yours and someone else's unpublished changed was arcane process which could easily lead to data loss. Additionally, all sort of tooling, like advanced tests and staging environment required code on master. So making merge quick and easy was the only solution.
Luckily by the time we started again git was around (but not github, and we self-hosted everything anyway), so we improved the tooling itself. Turns out having lightweight branches is great! And being able to tell a colleague "PTAL at my branch" is also great! And we improved tooling so we could push any branch to staging or run extended tests on any commit! And we can review the code and fix it before having entire company use it! So the fact that a PR might take a day or two to land was not a big deal at all.
If you are still on the post-commit reviews, take a look at your processes. The new features are great, and you might be missing out on them.
(This assumes a medium sized team. If it's just 2 people working on a codebase, the benefits are much less and costs are relatively bigger)
- trust that your coworkers are competent code-wise - without this, you're gonna get frustrated that you're constantly having to clean up after them and catch problems they've created (whether in a PR gated process or later when you find the problem they already committed - note that this isn't solved by PR gating because someone else less competent might have approved the PR anyway)
- trust that your coworkers aren't going to be annoying with their reviews - you cover this one, you'll be constantly blocked if you can't trust that
When people complain about very large organizations and teams being too bureaucratic and annoying it's because their organization hasn't figured out a way of scaling that trust. There are coordination problems that become very hard: hiring quality bars for interviews; alignment on what to focus on in reviews; even just "my opinion on what code I can be trusted with" vs "the domain expert's opinion on how much I can be trusted with that code". So process is created because "trust" doesn't scale to huge networks of people. The author mentions TDD, that's just an automated form of process because honestly, you shouldn't even trust yourself with some things when it comes to code. ;) But in a huge org you can't even trust everyone else to be on the same page, and that causes friction if you want to go fast.
I trust that I'm not doing anything malicious, but I am also aware that I'm a human being and make mistakes. Sometimes really dumb ones. I like running it past another pair of eyes that aren't intimately familiar with my code to minimize the amount of dumb that makes it into the final product.
If someone leaves or has a bad meeting with a bus, at least one other person has some familiarity with that code.
PRs are good even if it's just within the small team as a way to peer review and have an extra pair of eyes.
It’s not to protect our default branches from our co-workers. It’s to protect them from ourselves. I work in a small team in a non-tech enterprise organisation, and each of our smaller code-bases tend to be mainly worked on by one person. Since the things are interconnected as stuff is today, I’ll sometimes add some react bits to our major front-end app that one of my coworkers have written 90% off, and sometimes he will add new controllers or features to one of the back-end services that I have build. In those cases, the strict default branch policies have never been an issue. Most of the time I’ll approve a Pull Request from my co-workers without reviewing anything, exactly because I trust them.
What I don’t trust is for myself to not cut-corners and not alter the default branch, or especially, to create a work-item on the DevOps board for what I think is a minor fix. I don’t trust myself to do that, because sometimes, I still turn the default branch rules off when I need to tinker because the red tape is annoying. Which is fine, until it isn’t, and the more times it isn’t the more I come to respect the red tape we’ve installed to protect us from ourselves.
I think the discussion around reviews becomes more informed once you're past that hurdle.
Also, as I've said before, architecture reviews yield 10x the value of reviewing programming logic.
It was more an organizational rather than peer level concern, but I've heard some good discussion of trust being the currency of the profession. Feeling like you are adequately regarded, even though it's likely few others really see into your part of the faxtory floor. Being able to trust the org to make good choices. The article focuses on peer relationships & risk & getting good output, but I think the social factors around trust more broadly are at least as important.
You can speed up the code review process by trusting people more on the basis of a longer tenure of high quality code and few bugs making it to production. Especially if they add screen shots or video snippets to the pull request to demonstrate the feature and to demonstrate edge cases.
Make it as asynchronous as possible and it scales to hardcore software engineering speed. And with time reduce the number of gatekeepers by assigning more authoritative titles with lower required numbers of approvals for things. That way we begin to promote the people who are demonstrating the most efficacy in shipping working code that gets the job done, rather than those people who are maxed out on talking a lot but don't ship any code at all. Or worse, the silent coasters who do nothing and say nothing lol. We see you now.
I don't understand the glee that software developers feel when they get to police their peers performance.
I honestly never see this in any other type of professionals. There's lots of other things that other professionals don't do that we promote, like micromanagement.
Given the joy you are expressing over being able to police others performance, I get why code reviews can sometimes be so despised: when you have people taking joy in pointing out others performance, like you appear to do, the code review is just another tool of passive aggressive bullies.
If you are writing a whole article on not trusting yourself, and never even mention the idea of engaging with someone dedicated to finding problems, then I can’t take you seriously.