- It sounds pretty simple to automate that away too: make it part of the merge hook to include the source branch name into the message.
We are engineers, everything is a problem waiting to be automated :)
- Invite engineers to solve it in a way that makes it cheap for them.
Most shops I've been at prefix their branch names with ticket numbers ("bug-X-" or "TCKT-Y-"), and then it's trivial to reference it back. Some will write scripts on top, which gets them even more motivated to solve your problem (and might add links into the tracking tools too, move the ticket to "In Review" when the PR is up, close it after it's merged...).
- But if nobody writes them, they don't have the habit of reading them either.
However, also make sure your PR descriptions are not "substantial" in the "there is a lot of it" sense, but only "substantial" in the "everything of substance is described, but not more" sense :)
- But there are really two types of things you need to describe in a change request:
- What and why needs changing
- What the code does after the change
One should really try hard to keep the first one answered in a change request description, or comments in the tool for code reviews. Don't you love running into comments in the code of the type "// This performs better than sorting-after-load as the service offers built-in sorting." because someone originally did "load(); in_memory_sort()" and today the code only does a "load(order_by=X)" (I mean, duh).
The resulting code should only have comments that explain the why for the end-state code.
But yes, questions to explain something in the end-state should always trigger changes in the code: make code more self-explanatory!
- What is the overall practice in the team? Does everybody write good descriptions and nobody reads them, or only a few of them write good descriptions and nobody reads them?
Because if it's the latter, there's your problem: even those who write good descriptions do not expect a change request to have one, so they don't bother looking.
- By investing in quality on your job, your job will be easier and take less time. In software, you won't be called in to resolve that incident over the Thanksgiving weekend, and you won't be asked to debug why your system broke after someone committed 10k line diff without any review and without tests.
Be selfish! But be smart! On top of getting the best result for you, this gets the best result for the business too! And businesses know it, and even if they don't reward it proportionally, they do reward it with bonuses and seniority promotions.
- It can be your problem if the company goes under and you lose your job: you might not be able to pay your mortgage or bills.
If you believe your manager is asking for unreasonable things in what you are an expert in despite you raising these concerns, and it's not clear their manager is in on it, please raise it to their manager!
"I am willing to continue working this way, but I just want to make sure the consequences it could have on the business are clear to everyone here."
- Obviously, unleash LLM code reviewer with the strictest possible prompt on the change :)
Then innocently say "LLM believes this is bad architecture and should be recreated from scratch."
- 2 decades ago, so well before any LLMs, our CEO did that with a couple of huge code changes: he hacked together a few things, and threw it over the wall to us (10K lines). I was happy I did not get assigned to deal with that mess, but getting that into production quality code took more than a month!
"But I did it in a few days, how can it take so long for you guys?" was not received well by the team.
Sure, every case is its own, and maybe here it made sense if the fix was small and testing for it was simple. Personally (also in a director-level role today), I'd rather lead by example and do the full story, including testing, and especially writing automated tests (with LLM's help or not), especially if it is small (I actually did that to fix misuse of mutexes ~12 months ago in one of our platform libraries, when everybody else was stuck when our multi-threaded code behaved as single-threaded code).
Even so, I prefer to sit with them and loudly ask questions that I'd be asking myself on the path to a fix: let them learn how I get to a solution is even more valuable, IMO.
- Pairing people together on a single task makes that task get done faster, with higher quality. However, when paired together, the people still pick up some of the same biases and hold the same assumptions and context, so it is really worse than having a single author + independent reviewer.
So:
single author, no review < pair programming, no review < single author + review < pair programming + review - With time and experience, reading code becomes much easier.
And well-written code is usually easy to read and understand too!
The purpose of a code review is, apart from ensuring correctness, to ensure that the code that gets merged is easy to understand! And to be honest, if it's easy to understand, it's easy to ensure correctness too!
The biggest challenge I had was to distinguish between explanations needed to understand the change, and explanations needed to understand the code after it was merged in. And making it clear in my code review questions that whatever question I have, I need code and comments in the code to answer them, not the author to explain it to me (I frequently have already figured out the why, but took me longer than needed): it's not because I did not get it, it's because it should be clearer (finding the right balance between asking explicitly, offering a suggestion, or pitting it as a question to prompt some thinking is non-trivial too).
- Right, we already had both: pre-review build & test runs, and pre-merge CI (this actually ran on a temp, merged branch).
- Doing code review as described (actually diving deep, testing etc) for 10 engineers producing code is likely not going to be feasible unless they are really slow.
In general, back in 2000s, a team I was on employed a simple rule to ensure reviews happen in a timely manner: once you ask for a review, you have an obligation to do 2 reviews (as we required 2 approvals on every change).
The biggest problem was when there wasn't stuff to review, so you carried "debt" over, and some never repaid it. But with a team of 15-30 people, it worked surprisingly well: no interrupts, quick response times.
It did require writing good change descriptions along with testing instructions. We also introduced diff size limits to encourage iterative development and small context when reviewing (as obviously not all 15-30 people had same deep knowledge of all the areas).
- Wow, that's a very arbitrary practice: do you remember roughly when was that?
I was in a team in 2006 where we did the regular, 2-approve-code-reviews-per-change-proposal (along with fully integrated CI/CD, some of it through signed email but not full diffs like Linux patchsets, but only "commands" what branch to merge where).
- I believe they meant you could create an executable that is accessible outside the container (maybe even as setuid root one), and depending on the path settings, it might be possible to get the user to run it on the host.
Imagine naming this executable "ls" or "echo" and someone having "." in their path (which is why you shouldn't): as long as you do "ls" in this directory, you've ran compromised code.
There are obviously other ways to get that executable to be run on the host, this just a simple example.
- There are better definitions. Monolith would be a full product (as we are talking Twilio, their main offering) in a single, tightly coupled codebase which does data passing mostly by directly calling appropriate code paths.
Service-oriented architecture is where you decouple parts of the full architecture into independent subsystems that communicate over opaque interfaces and without ability to break those boundaries.
Microservices architecture is a SOA taken in the direction of keeping them as small as possible, which can be very small!
- Even better, agreed!
- You'd still need one real measurement at least: this might get proportions right if background can be clearly separated, but the absolute size of an object can be worlds apart.
- I'd suggest a different verb like "detach" or "unlink".
Is this pairing on small tasks that get done in (less than) a day? Or do you also have longer-running implementations (a few days, a week, or maybe even more?) where you rotate the people who are on it (so one task might have had 5 or 10 people on it during the implementation)?
If the latter, that sounds very curious and I'd love to learn more about how is that working, how do people feel about it, what are the challenges you've seen, and what are the successes? It'd be great to see a write-up if you've had this running for a longer time!
If it's only the former (smaller tasks), then this is where biases and assumptions stay (within the scope of that task): I still believe this is improved with an independent reviewer.