One goal that I think often gets missed out of code review guides is education.
To me, code review is not just there to produce better software, but also better developers.
It's a great place to talk about tradeoffs, alternative approaches, reasoning behind changes, etc.
Maybe less useful in huge projects with many contractors you're never going to work with again, but I've seen the mentoring aspect have great benefits in small teams.
Couldn’t agree more. I am lucky to have worked with two individuals early in my career who would take the time to really help develop team members by way of thoughtful, detailed PR comments. I attribute much of my own growth due to this, and as such I always try to do the same for others. I care deeply about it and consider it to be one of the most important parts of the job. It saddens me that most people (IME) don’t feel the same.
On the other side of the spectrum - code reviews have never, not even one single time, given me any real opportunity to learn something of value. They have only ever been someone else shoving their preferences onto my coding style (for better or worse)
I was shocked at my first job to realize the code reviewer didn’t even read my code, merely glanced over and demanded style changes (which were not consistent from review to review in the slightest)
It really felt more like how you hand your advisor your thesis with obvious easy to fix errors so he doesn’t compulsively decide “well something must need improvement” and make you fix something difficult
I do a lot of mentoring and I’m not sure that code reviews are really the right place for it. Comments in a code review can be fine for small things like “oh you just rewrote a function that already exists over here- maybe you’d like to use that instead”, but those things don’t really matter much in the end.
Teaching people how to design applications is a lot more useful, but code reviews are really not the time and place. For one thing, making someone go back to the beginning when they have a working (if flawed) PR is going to demotivate people and strip away their confidence. It’s also going to really limit your ability to ship quickly. Even if you put those challenges aside, the review experience for GitHub PRs (which I’d guess is how most people are doing reviews) doesn’t allow you to comment on code that wasn’t part of the diff. This makes it really hard to discuss the broader impact of a change i a PR because you can’t add context inline about things someone missed.
After trying and failing to do good mentoring through code reviews I’ve mostly given up. I do use them as a chance to see where people might benefit from some mentoring, but I keep my reviews quick and lightweight. If I don’t see anything wrong with the PR I approve it. Mentoring happens during dedicated time I put on people’s calendars to focus just on mentoring outside of a PR.
Absolutely - I don't see code reviews as a replacement for mentoring, more as something to augment it, or direct future mentoring sessions.
I've had quite a few experiences where I've suggested a change in a code review, and found out that the author lacked knowledge in that area. That's then turned into a future topic to focus on in future mentoring sessions.
Agree about design too, I don't think code review is really the place for that, since it's often a bit late by then (plus it's hard to get a sense of the overall intent and architecture from code chagnes alone).
It's a shame it doesn't happen formally more often, but one place I worked in the past always had a separate design review (which needed at least one person to approve) before coding could even start. This was fairly informal, but caught a few major potential design problems early on.
Your point about the inherent conflict between educating juniors & having lots of contractors is an important one. The most messed up teams I worked for were 1/2 (junior) contractors and this was a big issue. You basically have a team of bad developers you don't really mentor & expect to get better. As to the why, you can talk to management. The senior engineers lost that fight.
Agreed. And, sadly, besides providing zero education, some code reviews even turn into spaces of abuse of political power when conducted by unprepared reviewers.
I've been unfortunate to work with one of those, some years ago. I decided to pretend I enjoyed the style as soon as I realized she suffered from a fragile sense of authority. In hindsight, that doesn't seem to have been a good decision.
It was clear she felt affronted by any deviations from the intended standard. But the feedback mostly comprised extremely vague, subjective, non-actionable ad hoc prescriptions. I carefully read through my reviews and the ones received by my colleagues, but was unable to devise any pattern. Ultimately I decided to simply code as fast as possible so that I could collect the feedback earlier and get rid of the damn thing.
That way I could kind of cope, but after some months I was really burned out and struggling to sustain any interest about my tasks.
Additionally, the Education is both ways - the reviewer is also getting educated along the way, especially from new developers who have a more fresh knowledge of things.
After 30+ years, I kind of reverse the pyramid, honestly. I look for readable, maintainable code, and try to make sure the writer is doing appropriate testing. But whether or not it works and is correct, that's not my job, that's the developer's job. I know developers that will spend days on a code review, which is a horrible waste of time. CI/CD and your deployment followup structure (canaries, monitors, gates, etc) should be catching any significant issues the vast majority of the time, and if it isn't, you need to spend time there.
My main concern as a reviewer is to make sure future people can understand the code when the inevitable modification comes along.
Things are different if the writer and reviewer are in a mentorship relationship, but even still, if you are only engaging with the code as a mentor when the PR hits, you're messing up the mentorship!
“ I look for readable, maintainable code, and try to make sure the writer is doing appropriate testing. But whether or not it works and is correct, that's not my job, that's the developer's job.”
And I personally refuse to work with this style of workflow anymore
Does the code do what it needs? That’s objective and it either does or doesn’t. I can manage that kind of review.
Does the code look pretty enough? This is subjective and guarantees i’ll waste hours of my life every month bc someone else felt anxious/neurotic/masochistic and wanted to dump a chore on me.
No thanks, if you have style guidelines i’ll follow them. I’m not your whipping boy that will ask how high when you say jump.
The fact that something is subjective does not mean that it is useless.
IMHO, writing maintainable code requires a lot of experience, which one cannot expect from all junior developers. Requiring style guidelines for everything is a bit pedantic, and does not even make sense when you stride in to new territories. The latter is increasingly becoming the only part of software development where I would like to go.
If all code and requirements would be so objective and so simple, then I'm afraid you'll soon be replaced by an AI agent :)
Readability is not "prettiness", it's minimizing the effort as much as feasible to understand what the code is doing. I don't care what lines the braces are on.
From what I've seen so far, people love debating code style.
The main problem is that those programmers mostly care about superficial syntax and don't actually care about code readability or design. They just go for the lowest hanging fruit and criticise variable naming or whitespace because it is easy; they don't talk about the object graph or the event lifecycle of the program, because that's hard.
A very common thing I see nowadays is a forced adoption of gofmt/black/whatever, hoping it would solve the obsession with the formatting. However, this just locks in a (often substandard) coding style and removes any kind of personality from the code. This is good if you are a manager trying to treat your employees as fungible units of work, but is bad for actually maintaining a codebase. Also, it doesn't stop obsession with style, it just forces a specific style on everyone which most of the time, everyone slightly hates.
I found your comment insightful until the "(often substandard)" part. This betrays the rest of your comment. You start out saying how arguing specific indentation style is superficial but then betray this with that comment. If it truly is superficial then it does not matter what you lock in as long as you lock it in.
As with many things it's not actually a technical problem. It's a social problem. And it can work. Where I am right now for example we do have formatting locked in and enforced by linters. And it's not an issue there's no debate about it in regular PRs at all. The code formatter adjusts it and that's that. If you want to change it, create a PR that changes the rules and put a cross section of developers on said PR to discuss it. Happens from time to time but not often and works well.
I say "often substandard".... because, honestly, it just is. For example, breaking long logging lines into multiple lines decreases readability, but the formatter doesn't care because it only sees the max. line length and that's it.
Same with breaking long function definitions: it doesn't consider what makes sense or what is readable, it just messes the code up. Similarly, some projects differentiate between single and double quotes (e.g. single quotes for data, double quotes for userstrings), and this just drives a tank over it.
Or completely disallow tabs, even when it's necessary for accessibility, like with screen readers (not even PEP8 enforces spaces, but black decides to without a configuration option)
Same with whitespace: it refuses you to add more than 1 (or 2, can't remember) empty lines inside a function, even when it would be more readable to space it out.
I heavily disagree with the assumption that consistency is better than readability. I believe that people can be civilised and not change each other's code just for the sake of formatting nitpicks, only when they actually edit the code.
The lack of debate over line-to-line code style (because it is enforced by a standard formatter) is one of the reasons I still enjoy using Go, and that's despite the fact that it doesn't use what was my preferred C-era brace style.
Just absolutely eliminating it as something to worry or argue about is something a lot more languages could benefit from, IMO.
And yes, while other languages have third-party formatting and linting tools that can enforce an agreed-upon standard, in the real world that just never works out as well (once a codebase grows beyond a couple of contributors) as having it be truly standardized in the language itself.
> From what I've seen so far, people love debating code style.
In my experience with people who seemed to confirm this, the opposite turned out to be true. I spent an inordinate amount of time configuring linting and formatting to suit their tastes, to minimize any formatting subjectivity in code review. Their reviews got a lot more substantive and valuable pretty much immediately because their formatting concerns were evidently close enough to solved that they could pay attention to what the code actually did.
I’ve had similar success adopting formatting that no one likes just because it’s opinionated and no one gets to dispute it because they couldn’t if they wanted to without diverting resources to a new tool. Now we’re all unhappy with the formatting and still happy to be reviewing actual substance.
When the bad formatting gets in the way, it’s trivial to ask the question “is this just a whitespace change?” and everyone groans for a second, agrees that it is, and gets on with life.
And when there’s any remaining style considerations (oh you like, or don’t like, reduce?) that’s a much smaller space of style concerns to negotiate and settle.
I think this is just accommodating anti-social behaviour, IMO. I think the correct choice of action is simply rooting the problem out by penalising comments about style on code review, so they focus on the substance of the code instead. Yes, this will require a period of uncomfortability while everyone gets used to each other's varied code styles, but it promotes a much more healthier atmosphere than the "suck it up" approach of unifying all code. That doesn't solve the problem, that just masks it by putting the code through a shredder so style gets lost.
Code should not have personality. If you're working in a massive code base and people can tell you wrote code, that's probably an indictment on your code more than whatever style the company has chosen to use. Homogeneous code bases are the goal.
Only if you are an employer who wants to treat programmers interchangeably..... as a programmer, you have zero reason to do that. If all the codebase is homogenous, you can be replaced way easier, which is probably not what you want.
I definitely agree that Black (which I love) is pretty overrated in the number of problems people think it will solve for them. But I just don't get this view
> removes any kind of personality from the code
What kind of personality comes from things like pefering " over '; having tabs over spaces; having n spaces between class methods?
For me, those things are essentially meaningless but nice to have consistency on. Code personality, and a large part of readability comes from things like:
- variable / class names
- functional vs oop problem solving
- type use
- custom data structures or not etc
None of those have are locked down or touched by a lint tool like Black. Use it or don't, but you'll have plenty of personality in your code either way.
That's not the interesting "personality". I think the formatting tools bring in something completely different: code layouts that you would never write by hand.
I don't like that. Code should be a transparent interface, you write something and it becomes part of the program. With code formatting it becomes, you input something into the source code, you transform it with the tool, and then it's done. We sort of lose the brain-hand-program connection.
As an example I randomly found this code snippet of rust imports. To reproduce this by hand would be like writing JSON by hand - I'm referring to the nesting, bracketing and sorting. This is the lack of personality: no longer writing code by hand, but with tools.
use core::fmt;
use std::{
alloc::{GlobalAlloc, System},
cell::UnsafeCell,
cmp,
fmt::Write,
marker::PhantomData,
sync::{
atomic::{AtomicBool, Ordering},
Mutex,
},
time::Duration,
};
use serde::{
de::{self, SeqAccess, Visitor},
Deserialize, Deserializer, Serialize,
};
"Consistency is the most important" seems to be an axiom...
"having n spaces between class methods" is definitely *not* meaningless. It affects code readability and black has very opionated (and often inappropriate) ideas about what counts as "readable" whitespace. When you raise that problem with people, they always say "well too bad, you should write shorter functions instead", which just introduces another layer of opinionatedness.
>A very common thing I see nowadays is a forced adoption of gofmt/black/whatever, hoping it would solve the obsession with the formatting. However, this just locks in a (often substandard) coding style and removes any kind of personality from the code. This is good if you are a manager trying to treat your employees as fungible units of work, but is bad for actually maintaining a codebase. Also, it doesn't stop obsession with style, it just forces a specific style on everyone which most of the time, everyone slightly hates.
This is so wrong. There is no reason to not have a standart for formatting code when working in a team.
There are good reasons for not having a standard code formatting style in a team. They might be ones you don't agree with, but saying there are no good reasons is IMO a bit disingenuous. With mature team members, it's perfectly possible to respect each other's style and if executed well, it can have advantages such as being able to very quickly identify who wrote something (who to pester about something), making readability decisions locally instead of globally and benefits like that.
Of course, that requires a team with emotional maturity, which is I get, not very common in the software industry.
> it just forces a specific style on everyone which most of the time, everyone slightly hates.
Just stop caring about useless stuff, gee… this is what the code review pyramid about, focus your efforts and care on important stuff, not irrelevant details.
Auto formatted code is the best thing that happened in the last few years. I don’t care that it doesn’t show the « personality of the dev who wrote it », I want consistent code.
> However, this just locks in a (often substandard) coding style and removes any kind of personality from the code. This is good if you are a manager trying to treat your employees as fungible units of work, but is bad for actually maintaining a codebase.
Is it bad, though? Having most of the code in the codebase look and read consistently feels like a good thing, even if that's not the style that you'd personally prefer. Especially if the formatter does its thing whenever you save a file.
I mean, you are right.... But as programmers, shouldn't you try to be less replaceable? Have unique skills or methods which can be valued, instead of being a generic hotswappable code monkey.
I don't really understand your logic - why is management right for removing personality? Of course, from a programmer's perspective, not from the manager's perspective.
I find this too concrete to be useful. I have a more principals / tradeoffs approach to code review:
Areas:
- Readability - how understandable the code is
- Maintainability - how the code enables the project to evolve
- Risk - security, regulatory and other vulnerabilities
- Correctness - whether the code does what you intended
- Robustness - how well the code handles unintended circumstances
- Performance - how resource efficient the code is
All of the above areas need to be considered within the context of the project and individual team members.
## Readability
Code is readable if it meets your expectations and surprises you only when there's something you don't yet know about the technology. We expect code to look like the code around it. To follow most, if not all of the technologies idioms. To use clear and informative names. To explain why deviance exists. To be as abstract as the concepts being abstracted are crystallised.
## Maintainability
Maintainability is the code's relationship with the wider team and time. You should be able to learn all you need to know to change, test and deploy code. You should be confident that any changes you make will not cause unintended changes elsewhere in the code. To make a change, you should have to touch as little code as the concepts being changed are crystallised. You should be able to debug an issue as easily as, the system the issue is in, is old. You should be able to track down and alter the changes that introduce a bug quickly. You should be able to learn why a change exists.
## Risk
Code is low risk if it accounts for, and where possible, mitigates the various risks to a project.
## Correctness
Correct code does as you expect and surprises you only when there something about the system you didn't yet understand. Validating correct code is as automated as the validations are frequent. Correct code is only as complicated as the concepts it models. Correct code is only as abstract as the concepts it models are crystallised.
## Robustness
Robust code handles unexpected circumstances safely, quickly and predictably. Validating robust code is as automated as the validations are frequent and as comprehensive as the failures are risky.
## Performance
Performant code is as resource efficient as the resources are expensive, but also as efficient as the development costs are cheap.
Ehhhhh yeah kinda? Yes, you should spend most of your time on "API Semantics" (what does it look like using this code?), and you should spend a lot less time on "how are the tests?"
but, for example,
Writing good tests massively contributes to good implementation details and API semantics; and test code is also code that needs to be reviewed under more-or-less the same criteria as the rest.
Also - documentation (or, legibility, if you're onboard with self-documenting code) can be more important than either implementation details, and even API semantics, as it can define whether the entire work is useable or maintainable.
I might say instead:
- What will it be like reviewing this code? (style, test coverage, etc - do I have to worry about spotting typos, and other stupid stuff?)
- What will be like debugging this code? (patterns, logging, etc - which might handled by a framework)
- What will it be like altering this code? (documentation/legibility, implementation details, etc - when the business needs change or grow)
- What will it be like using this code? (API semantics, API docs - when I go to build something on top of this)
And then yeah; the top should be entirely automated, and you should (generally) spend most of your time on the bottom.
My own "but" is shorter: the idea is fine, except code review also "should", per the prevailing wisdom, happen on small, focused changes. But that deep in the woods, style and testing is pretty much all you can talk about. There isn't much use in starting a discussion about API semantics on a commit that implements a stub of one of its endpoints or sth.
I might hold an unpopular view here but I do not like reviewing small focused changes.
When a feature is implemented I preferably want the entire thing before me when reviewing. Otherwise I find it hard to keep track of everything that has happened.
Not to mention that the small focused changes might be reviewed by different people leaving only the implementer in full knowledge of everything that was done.
Peer/Assembly programming help but if you do peer/assembly programming reviews are mostly a waste of time (especially for assembly programming).
At a former team, we went from spending quite a bit of time on code style comments and disagreements to spending no time at all on it, with the simple act of making the code linter a breaking step in our CI build, and deciding no review will start until the build is green.
We had to adjust our linter settings here and there - but it was still super efficient for everyone's time compared to what we had before...
If you are reviewing implementation semantics after someone has already coded a feature, I'd say it's too late. You might catch issues, but the loss of goodwill and wasted productivity will make everyone hate code review.
I think this pyramid is accurate but better framed as a "review pyramid". Catch semantic issues in an earlier phase (informal discussion or rfc-style proposal document).
That really depends on the size of the feature, but imho we should be reducing the need for meetings/reviews, not adding more. Forcing design reviews just slows people down. Jeffy B's "communication is terrible" are words to live by if you want fast moving engineers.
Sure, but the price is worth it. If we have a design session before you start coding, it definitely will make the feature go “late” by an amount of time proportional to the design session… which is less time than the required to fix the design after the PR is open.
“Fast moving engineers”: I can only imagine managers advocating for that. As an engineer, I can only but express disdain for such a mentality.
Readit News
To me, code review is not just there to produce better software, but also better developers.
It's a great place to talk about tradeoffs, alternative approaches, reasoning behind changes, etc.
Maybe less useful in huge projects with many contractors you're never going to work with again, but I've seen the mentoring aspect have great benefits in small teams.
I was shocked at my first job to realize the code reviewer didn’t even read my code, merely glanced over and demanded style changes (which were not consistent from review to review in the slightest)
It really felt more like how you hand your advisor your thesis with obvious easy to fix errors so he doesn’t compulsively decide “well something must need improvement” and make you fix something difficult
Teaching people how to design applications is a lot more useful, but code reviews are really not the time and place. For one thing, making someone go back to the beginning when they have a working (if flawed) PR is going to demotivate people and strip away their confidence. It’s also going to really limit your ability to ship quickly. Even if you put those challenges aside, the review experience for GitHub PRs (which I’d guess is how most people are doing reviews) doesn’t allow you to comment on code that wasn’t part of the diff. This makes it really hard to discuss the broader impact of a change i a PR because you can’t add context inline about things someone missed.
After trying and failing to do good mentoring through code reviews I’ve mostly given up. I do use them as a chance to see where people might benefit from some mentoring, but I keep my reviews quick and lightweight. If I don’t see anything wrong with the PR I approve it. Mentoring happens during dedicated time I put on people’s calendars to focus just on mentoring outside of a PR.
I've had quite a few experiences where I've suggested a change in a code review, and found out that the author lacked knowledge in that area. That's then turned into a future topic to focus on in future mentoring sessions.
Agree about design too, I don't think code review is really the place for that, since it's often a bit late by then (plus it's hard to get a sense of the overall intent and architecture from code chagnes alone).
It's a shame it doesn't happen formally more often, but one place I worked in the past always had a separate design review (which needed at least one person to approve) before coding could even start. This was fairly informal, but caught a few major potential design problems early on.
I've been unfortunate to work with one of those, some years ago. I decided to pretend I enjoyed the style as soon as I realized she suffered from a fragile sense of authority. In hindsight, that doesn't seem to have been a good decision.
It was clear she felt affronted by any deviations from the intended standard. But the feedback mostly comprised extremely vague, subjective, non-actionable ad hoc prescriptions. I carefully read through my reviews and the ones received by my colleagues, but was unable to devise any pattern. Ultimately I decided to simply code as fast as possible so that I could collect the feedback earlier and get rid of the damn thing.
That way I could kind of cope, but after some months I was really burned out and struggling to sustain any interest about my tasks.
My main concern as a reviewer is to make sure future people can understand the code when the inevitable modification comes along.
Things are different if the writer and reviewer are in a mentorship relationship, but even still, if you are only engaging with the code as a mentor when the PR hits, you're messing up the mentorship!
And I personally refuse to work with this style of workflow anymore
Does the code do what it needs? That’s objective and it either does or doesn’t. I can manage that kind of review.
Does the code look pretty enough? This is subjective and guarantees i’ll waste hours of my life every month bc someone else felt anxious/neurotic/masochistic and wanted to dump a chore on me.
No thanks, if you have style guidelines i’ll follow them. I’m not your whipping boy that will ask how high when you say jump.
The fact that something is subjective does not mean that it is useless.
IMHO, writing maintainable code requires a lot of experience, which one cannot expect from all junior developers. Requiring style guidelines for everything is a bit pedantic, and does not even make sense when you stride in to new territories. The latter is increasingly becoming the only part of software development where I would like to go.
If all code and requirements would be so objective and so simple, then I'm afraid you'll soon be replaced by an AI agent :)
‘…readable, maintainable [and tested]’, ie. if someone else needs to modify or remove this code in the future, will there be friction?
The main problem is that those programmers mostly care about superficial syntax and don't actually care about code readability or design. They just go for the lowest hanging fruit and criticise variable naming or whitespace because it is easy; they don't talk about the object graph or the event lifecycle of the program, because that's hard.
A very common thing I see nowadays is a forced adoption of gofmt/black/whatever, hoping it would solve the obsession with the formatting. However, this just locks in a (often substandard) coding style and removes any kind of personality from the code. This is good if you are a manager trying to treat your employees as fungible units of work, but is bad for actually maintaining a codebase. Also, it doesn't stop obsession with style, it just forces a specific style on everyone which most of the time, everyone slightly hates.
This is a really good article which summarises the problem way better than me. https://luminousmen.com/post/my-unpopular-opinion-about-blac...
As with many things it's not actually a technical problem. It's a social problem. And it can work. Where I am right now for example we do have formatting locked in and enforced by linters. And it's not an issue there's no debate about it in regular PRs at all. The code formatter adjusts it and that's that. If you want to change it, create a PR that changes the rules and put a cross section of developers on said PR to discuss it. Happens from time to time but not often and works well.
Or completely disallow tabs, even when it's necessary for accessibility, like with screen readers (not even PEP8 enforces spaces, but black decides to without a configuration option)
Same with whitespace: it refuses you to add more than 1 (or 2, can't remember) empty lines inside a function, even when it would be more readable to space it out.
I heavily disagree with the assumption that consistency is better than readability. I believe that people can be civilised and not change each other's code just for the sake of formatting nitpicks, only when they actually edit the code.
Just absolutely eliminating it as something to worry or argue about is something a lot more languages could benefit from, IMO.
And yes, while other languages have third-party formatting and linting tools that can enforce an agreed-upon standard, in the real world that just never works out as well (once a codebase grows beyond a couple of contributors) as having it be truly standardized in the language itself.
In my experience with people who seemed to confirm this, the opposite turned out to be true. I spent an inordinate amount of time configuring linting and formatting to suit their tastes, to minimize any formatting subjectivity in code review. Their reviews got a lot more substantive and valuable pretty much immediately because their formatting concerns were evidently close enough to solved that they could pay attention to what the code actually did.
I’ve had similar success adopting formatting that no one likes just because it’s opinionated and no one gets to dispute it because they couldn’t if they wanted to without diverting resources to a new tool. Now we’re all unhappy with the formatting and still happy to be reviewing actual substance.
When the bad formatting gets in the way, it’s trivial to ask the question “is this just a whitespace change?” and everyone groans for a second, agrees that it is, and gets on with life.
And when there’s any remaining style considerations (oh you like, or don’t like, reduce?) that’s a much smaller space of style concerns to negotiate and settle.
> removes any kind of personality from the code
What kind of personality comes from things like pefering " over '; having tabs over spaces; having n spaces between class methods?
For me, those things are essentially meaningless but nice to have consistency on. Code personality, and a large part of readability comes from things like: - variable / class names - functional vs oop problem solving - type use - custom data structures or not etc
None of those have are locked down or touched by a lint tool like Black. Use it or don't, but you'll have plenty of personality in your code either way.
I don't like that. Code should be a transparent interface, you write something and it becomes part of the program. With code formatting it becomes, you input something into the source code, you transform it with the tool, and then it's done. We sort of lose the brain-hand-program connection.
As an example I randomly found this code snippet of rust imports. To reproduce this by hand would be like writing JSON by hand - I'm referring to the nesting, bracketing and sorting. This is the lack of personality: no longer writing code by hand, but with tools.
"having n spaces between class methods" is definitely *not* meaningless. It affects code readability and black has very opionated (and often inappropriate) ideas about what counts as "readable" whitespace. When you raise that problem with people, they always say "well too bad, you should write shorter functions instead", which just introduces another layer of opinionatedness.
This is so wrong. There is no reason to not have a standart for formatting code when working in a team.
Of course, that requires a team with emotional maturity, which is I get, not very common in the software industry.
Just stop caring about useless stuff, gee… this is what the code review pyramid about, focus your efforts and care on important stuff, not irrelevant details.
Auto formatted code is the best thing that happened in the last few years. I don’t care that it doesn’t show the « personality of the dev who wrote it », I want consistent code.
> but is bad for actually maintaining a codebase
Absolutely not.
Is it bad, though? Having most of the code in the codebase look and read consistently feels like a good thing, even if that's not the style that you'd personally prefer. Especially if the formatter does its thing whenever you save a file.
Management is right in trying to remove “personality” imo.
I don't really understand your logic - why is management right for removing personality? Of course, from a programmer's perspective, not from the manager's perspective.
Areas:
All of the above areas need to be considered within the context of the project and individual team members.## Readability
Code is readable if it meets your expectations and surprises you only when there's something you don't yet know about the technology. We expect code to look like the code around it. To follow most, if not all of the technologies idioms. To use clear and informative names. To explain why deviance exists. To be as abstract as the concepts being abstracted are crystallised.
## Maintainability
Maintainability is the code's relationship with the wider team and time. You should be able to learn all you need to know to change, test and deploy code. You should be confident that any changes you make will not cause unintended changes elsewhere in the code. To make a change, you should have to touch as little code as the concepts being changed are crystallised. You should be able to debug an issue as easily as, the system the issue is in, is old. You should be able to track down and alter the changes that introduce a bug quickly. You should be able to learn why a change exists.
## Risk
Code is low risk if it accounts for, and where possible, mitigates the various risks to a project.
## Correctness
Correct code does as you expect and surprises you only when there something about the system you didn't yet understand. Validating correct code is as automated as the validations are frequent. Correct code is only as complicated as the concepts it models. Correct code is only as abstract as the concepts it models are crystallised.
## Robustness
Robust code handles unexpected circumstances safely, quickly and predictably. Validating robust code is as automated as the validations are frequent and as comprehensive as the failures are risky.
## Performance
Performant code is as resource efficient as the resources are expensive, but also as efficient as the development costs are cheap.
but, for example,
Writing good tests massively contributes to good implementation details and API semantics; and test code is also code that needs to be reviewed under more-or-less the same criteria as the rest.
Also - documentation (or, legibility, if you're onboard with self-documenting code) can be more important than either implementation details, and even API semantics, as it can define whether the entire work is useable or maintainable.
I might say instead:
- What will it be like reviewing this code? (style, test coverage, etc - do I have to worry about spotting typos, and other stupid stuff?)
- What will be like debugging this code? (patterns, logging, etc - which might handled by a framework)
- What will it be like altering this code? (documentation/legibility, implementation details, etc - when the business needs change or grow)
- What will it be like using this code? (API semantics, API docs - when I go to build something on top of this)
And then yeah; the top should be entirely automated, and you should (generally) spend most of your time on the bottom.
When a feature is implemented I preferably want the entire thing before me when reviewing. Otherwise I find it hard to keep track of everything that has happened.
Not to mention that the small focused changes might be reviewed by different people leaving only the implementer in full knowledge of everything that was done.
Peer/Assembly programming help but if you do peer/assembly programming reviews are mostly a waste of time (especially for assembly programming).
Although, the style guide should just be followed and fixed before you get to code review phase. That’s just a matter of professionalism
We had to adjust our linter settings here and there - but it was still super efficient for everyone's time compared to what we had before...
I can't recommend this more.
The Code Review Pyramid - https://news.ycombinator.com/item?id=30757206 - March 2022 (110 comments)
The Code Review Pyramid - https://news.ycombinator.com/item?id=30674159 - March 2022 (4 comments)
I think this pyramid is accurate but better framed as a "review pyramid". Catch semantic issues in an earlier phase (informal discussion or rfc-style proposal document).
Sure, but the price is worth it. If we have a design session before you start coding, it definitely will make the feature go “late” by an amount of time proportional to the design session… which is less time than the required to fix the design after the PR is open.
“Fast moving engineers”: I can only imagine managers advocating for that. As an engineer, I can only but express disdain for such a mentality.