I mentioned this idea to a very knowledgeable enterprise architect. I was mentioning how much I'd enjoyed being able to use the code review process in this way. His reply: As good of an idea that is, it breaks Agile. So long as the code meets the biz-provided spec, it must be accepted, and if there are other concerns with the code, to make a tech debt ticket to address it at a later date.

This, of course, horrified me.

>> it breaks Agile

It must be "nice" working with an enterprise architect who views agile as a strict, narrowly defined thing.

He's really sharp and has keen insight into how enterprise business works. His advice saves us a lot of time and money.

But if he wants you to open tech debt tickets, rather than spotting/fixing security issues immediately, this will not save the company time or money.

He doesn't. He was just describing to me the way the enterprise usually sees things. Our shop is more modern than he's used to working in.

His logic would apply to tests of all sorts. As long as the dev says the feature is done, that’s the end. And he’s wrong. As teams (or companies, or whatever unit) we get to decide our definition of “Done”. I suggest that definition include appropriate testing. And appropriate code review, which serves two purposes… catching defects and knowledge sharing.

In his world, testing the software belongs to a different team than the implementation. Differently managed, differently staffed. Not with engineers, but with testers. It's difficult to say the least to move from testing into engineering

And you can't just redefine Agile like that. It's the businesses' money, culture, and productive means. Not yours. If they want teetering software stacks with no thought given to maintainability, managed by non-tech-savvy staff, then that's what their money will buy.

We enjoy an environment catering to our needs because our orgs can afford to throw a lot more resources at better managers and better talent. As a result individual contributors can contribute not just tickets, but also to help improve the way we work. Not possible in heavily top-down org structures.

Ugh. I haven’t worked with an independent QA team in 15 years or so; testing is the responsibility of the dev team and there are test engineers on each team to facilitate that.

Maybe this is a difference of building a product vs contracting? We have to keep what we build running for years; there is no turnover to a client where can declare the system “done”.

As for redefining agile, I’ve done no such thing. The agile manifesto calls for working software. It calls for collaboration. Avoiding knowledge sharing amongst team members and throwing software over the fence to a remote test team are both counter to that goal (in my experience).

In my experience, splitting the testing team out of development materializes refactoring as costly.

"What parts of the system did this impact?"

"Well, this is a core piece of our ORM config, so... the entire data layer, so from a functional perspective this change impacts every part of the system"

"Then you're not changing it, QA can't rerun all their old tests, it would take months, most of those tests don't even work anymore".

End result: refactoring never happens. Get it right the first time.

Naturally. In an Agile environment it's more or less impossible to get it right the first time without seriously forward-thinking architecture, which takes time that could be spent pushing harder towards the deadline.

It's a business decision to organize software production this way. Refactoring and ease of software maintenance are quality of life issues for engineers. Not business concerns.

That's how a managerial bureaucracy approaches the problem of deadlines: The second it looks like it's working, mark it done and move to the next task, and deal with the bugs later - ideally the manager will move to another position by then due to their exceptional productivity in meeting the set goals, and the bugs will be someone else's problem to solve...

If they get solved at all.

Microsoft famously called that process “infinite defects methodology”, and decided it was a bad idea in 1989. "In general, the longer you wait before fixing a bug, the costlier (in time and money) it is to fix."

https://www.joelonsoftware.com/2000/08/09/the-joel-test-12-s...

Anyone with Architect in their title receives skepticism from me. I've turned down title changes that include it and I refuse to put it on my LinkedIn.

I think that attitude is a little short-sighted, and perhaps a little bit of "anyone that isn't a pure developer is incompetent" elitism.

For one thing, titles are just titles, and they stick around for historic reasons. For example, there really isn't a great reason for companies to call people DevOps Engineer, but it still happens.

I'll take the Solutions Architect role as an example. It's basically sales or customer-oriented developer or technical resource that works with customers to determine what a solution to a problem will look like. "Architect" is mostly meaningless in the sense that Solutions Architects don't really architect much of anything. Usually, they just come up with a plausible path forward and visualize that solution to all the stakeholders involved. This includes travel to customer sites, something that developers are basically never willing or expected to do.

They're the folks who deal with the god damn customers so the engineers don't have to. They have people skills, they're good at dealing with people. Yeah, I mean, considering the staff engineer on my team does not shower, there is value in that role.

Most of the value in the Solutions Architect is how they're able to work with customers to discover their needs on a more technical level rather than at a high level. Once the plan is determined, the solutions architects don't actually build the solution on their own, they're more like an interface or leader to the development team that builds the solution.

https://www.careerexplorer.com/careers/solution-architect/

I don't want to put words into your mouth, but maybe you're skeptical of "architects" because they aren't typically expert specialists in one small area. Maybe that's why you don't want to be associate with them. Understandable, perhaps, but don't be misguided into thinking that "architects" aren't skilled professionals who add value to the company.

On top of that, I believe they're often paid higher than developers ;-)

His world seemed completely and utterly divorced from mine. He was deeply connected with big enterprises and how they manage to meet business objectives regardless of not being software engineers themselves. In our world, we take for granted that our managers and leadership are technically proficient. In his, the "Engineering Manager" is a rarity, the people managing engineers are just ordinary managers.

Software architecture, in this world, is how you escape the rat race of soulless ticket punching without having to go into management. You use your skills and experience in an advisory role. Obviously there are better or worse architects, I've had the pleasure of working with really good ones. But it often feels like a title and field borne out of a need to retain top talent. (read, not push them away to competitors)

Even people designing buildings?

So, they find bugs and security vulnerabilities, help spot performance problems and keep the code maintainable -- as a service.

Still sounds like a pretty valuable service.

The case for code review is fundamentally economical: it saves the organization money by finding costly issues earlier when they are cheaper to fix without imposing a costlier burden for the review process itself.

As generic "knowledge transfer", or even increasing the bus factor, I would disagree. The best knowledge transfer experiences I've had have been dedicated meetings/workgroups dedicated to that purpose, and they tend to encompass larger scopes than a single commit/pull request. I've also seen the difference in devs who contribute to an area having previously only been reviewers of code in that area, vs. actually having a knowledge transfer session with that area's lead beforehand, and in the latter case they are more effective (actually even if they had never reviewed code in that area before, as was the case with interns or new hires). To me knowledge transfer is the nice possible side effect, but not the primary purpose.

I'll leave a third opinion from here https://static1.smartbear.co/smartbear/media/pdfs/best-kept-... which lists some direct and indirect benefits:

    Few developers and software project managers would argue that these are direct benefits of conducting code reviews:
    
    • Improved code quality
    • Fewer defects in code
    • Improved communication about code content
    • Education of junior programmers
    
    And these indirect benefits are byproducts of code review:
    • Shorter development/test cycles
    • Reduced impact on technical support
    • More customer satisfaction
    • More maintainable code

Maybe for you. For others it can have all or any of those other things as its primary purpose. One important thing for me is that it prevents or at least mitigates unilateral insider attacks by having two people required to change code.

Having a bus factor>1 is still a major prerequisite for doing that. Because if you have a junior developer review the changes of a senior developer, there are so many social engineering tricks to make the backdoor pass the review that it isn't even funny.

Personally I work on a 2 person team, the vast majority of my reviews either have no changes or "hey maybe this thing should be named something else so its more consistent".

For us the peer review is almost completely for knowledge transfer. Sure we know what each other is working on, but we still have to maintain each others code if something goes wrong and the other is not available.

So I agree with this 100%. Even in bigger organizations where the peer review was more formal, I feel like that is still the primary goal.

I do fail to see the benefit of this, especially looking at the price of it. Outside of maybe scripts? I can't really see how much they can actually help without the context of the larger application. Without that context can they really provide more benefit than AWS CodeGuru (or similar) could offer?

We do have detailed code reviews. People will complain about lines you write and expect a lot of consistency.

Simultaneously, architecture is complete mess, because architecture is much harder to see in code review.

> Say it with me: Code review is a knowledge transfer exercise.

It is, but (say it with me?): Things change.

Code review is not the sole knowledge transfer exercise, nor should it be forever. You could say similar things about "make format". Now that our formatting is automatic, we can discuss code at a higher level. If code reviews were standardized or even automated, we could discuss it at an even higher level.

In most of the projects I have reviewed for pullrequest.com, the engineering team is also doing its own reviews. We are "another set of eyes" as it were. Many of our larger reviews can end up becoming lengthy conversations between us and the team. There is definitely a lot of knowledge transfer going on. What's been truly rewarding is when someone on the team comes back to us and asks for advice on the best way to solve a particular problem.

Even without hard stats and evidence, I guarantee most people don't think of code review as mostly being about KT. The purpose(s) of code review commonly contain the ones you described, but it varies from team to team. The most common I'd guess is putting a 2nd pair of eyes on your code, checking code quality and finding issues.

Finding bugs is of questionable use because it's sort of limited to obvious bloopers. One usually needs to be deeply involved and familiar with the business logic to be able to spot a real bug of the type that will give you serious trouble - external services obviously will never have time for such commitment.

That said, I think there's still a lot of space where this service can be very useful: from improving your code style and an affordable way to have security audits, to just a support net for developers who might feel overwhelmed by a task sometimes, and could use some friendly advices from a seasoned dev. It being an external service can also make it less stressful and personal, which is great as some devs see code reviews as a criticism of their skills and go all defensive about it, creating tensions in teams (sounds silly, but I've seen a lot of it).

Not exactly. imo and just a perspective, but code review is one method of KT. Sending someone a document can also be a method of KT.

Code review for KT is one thing. Code review for finding bugs is another. Code review for following style guidelines is yet another.

I would like to use a baseline style guideline for JS is anyone aware of one that isn't too huge?

I'm a fan of using code formatters to define how you lay out code in a file.

Prettier is popular for that job:

https://prettier.io/

For detecting functional/idiomatic/behavioral issues, ESLint is my go-to:

https://eslint.org/

This shows my bias for automation over human enforcement.

Some companies doesn't have enough people to review the code, at least not with the required seniority. This way there is also knowledge transfer, but to the company. And with people not in your payroll that can provide objective comments since they are not afraid of telling the wrong person that their code sucks.

This. Code review is a terrible way to find bugs... At best you'll get the more Senior reviewer to spot library uses that are known to be workable but problematic (eg poor performance), coming from their experience. But straight up logic bugs are hard to spot.

The big thing that code review achieves is that it ensures a 2nd person understands the code that was written, and therefore it is possible for the reader to understand.

So 3 years down the road and you're looking at some counterintuitive piece of code, the reader isn't wondering *why on earth did he write it like this? Is it working around some cryptic edge-case bug in the framework or were they just stupid?"

> Code review is a knowledge transfer exercise

In many places, yes.

But: When you have a solo developer working on a component in a language that you aren't familiar with, team full of novices, component delivered by a contractor...

You're thinking of pair programming. That's different.

"but they have AI".. probably not. Probably the same linters and scanners that we all know + cheap working force reviewing code manually.

Hmm, what about education? I've learned a bunch through thoughtful reviews of my code by other devs.

That’s them transferring their knowledge to you :)

code review is a process that carries a lot of meaning today. the work cycle in most workplaces is not qualified to meet this meaning.

i thought code review was to assert dominance and to block as long as possible for yak shaving reasons; But I do like your version of it! Happy holidays~