Was nominal lead on a smallish project a couple months back, with 2 other folks. Worked with one before, one I hadn't worked with before.
We're all remote, and the 'new' guy started doing 'code review' on code before he had a running environment. We'd taken over another codebase, and he was pretty up-front about wanting to adopt/enforce certain stylistic standards, which... to me, I don't particularly care about as much (on the PHP side of things, PSR-2, in this case).
We're all remote, so we had a call - a couple actually - and I indicated I didn't care all that much, but he was free to make changes as he saw fit, as long as 1) he got a working environment up and running, 2) he was also spending time writing some tests around the code as he explored it, to have a firmer understanding of what was going on, and 3) he didn't break any existing tests (or new ones that were developed).
I swear that I don't think he'd actually had a working copy of the project, but was still committing code. Not a lot, but ... it broke tests. It broke stuff that was not 'tested' in an automated way yet, but was testable by running the app, and I think he'd just not bothered to test it at all.
But... hey - look at the formatting! Look - spaces, not tabs! - how useful when someone else is having to go back and debug your crap. I received a couple small lectures on variable naming and the importance of descriptive variable names.
function getListOfUsersForCompany()
{
$res = db::query('whatever query');
return $res;
}
Apparently, that $res is confusing and 'bad practice' because it's "hard to reason" about what's going on there. Literally 2 line methods with an internal placeholder variable (which also had some working functional tests around them) - those were being criticized by someone who had committed test-breaking code multiple times.
In all this, I kept having to figure out how much of my reaction was because my code was being criticized, vs what the criticisms were. I got accused of taking things 'personally', but I kept coming back to 'working tests broke with these changes', and in my mind, that this code was considered somehow 'better' because it more closely followed a stylistic 'standard' was bothersome. Working tests should a standard we strive for too, right?
I have worked with some folks who had a real eye for analysis - remote or colo pairing on problems has helped reason out issues that I know I've missed. And when I'm pairing or doing a requested review, cosmetic style tends to be the last thing I'll bring up, because it generally has the least impact on something. Much prefer to have someone spend time writing tests and/or docs and/or insightful comments vs reformatting. When I've refactored with someone to make something more testable, better naming/comments/docs tends to flow out of that refactoring anyway.
While I understand formatting is often not that big of burden, especially with modern IDEs, it's also not terribly high value on many projects. I'm primarily speaking on projects I've come in to where there's already extant code - in this case, we had taken on a very weird hybrid of 2 PHP frameworks and 2 JS frameworks (some screens used raw vue, some compiled vue, and some compiled angular).
Doesn’t sound to me like you are the problem. Sounds like your colleague has lost sight of the end goal and needs to refocus on what you’re supposed to be delivering.
(Unless the whole project was to reformat the code?)
I'd later learned through someone else who'd worked with 'new guy' that they'd had similar problems, and apparently he only really works well on his own. I'd been 'given' him on this project because the first person wasn't really sure if he (first person) was at fault, or if 'new guy' was. I was the test case that confirmed.
By no means was the purpose to just 'reformat' (far from it!) :)
I have wrestled with this a couple times before over the last ... 15+ years. When working with other folks, I'm normally not in 'full time w2' arrangements - it's usually shorter term contracting, so the dynamics are a bit different. That may also be why the style/formatting is usually less important to me. As with the project above, I'd seen many well-formatted projects with descriptive variable names that don't function. And... seen horrific spaghetti code that "just works" but people are afraid to touch it.
Finding that middle ground is a never-ending journey, but I feel I've got a decent sense of how to get pragmatic results, and it usually involves repeatable sample data, repeatable processes and tests and/or docs. Building those as a foundation will make stylistic changes a breeze, because the confidence to make "trivial" changes will be there. Coming in to new codebases with no tests, sample data or processes should be considered scary/risky, and when people don't seem to have an understanding of the risks involved, I get even more scared.
It took us 9 weeks on this project to get to a point where we made a first push out to production (and even then we ran in to a few small bumps). But we'd practiced with tests (only have dozens, not hundreds at this point) and pushing to staging, and as the client reviewed... we kept finding more and more bugs - things that had existed in the code for years that we were seemingly the first people to exercise in front of the client. I'm sure their confidence in us was bruised a bit - we apparently just kept showing bugs(!) - but it feels like we provided more of a base in 9 weeks than we inherited from the previous 4 years of teams (automated builds, sample data, automated unit tests, automated browser tests, etc).
Just did this a couple days ago and it's still very fresh/raw in my mind :)
Was nominal lead on a smallish project a couple months back, with 2 other folks. Worked with one before, one I hadn't worked with before.
We're all remote, and the 'new' guy started doing 'code review' on code before he had a running environment. We'd taken over another codebase, and he was pretty up-front about wanting to adopt/enforce certain stylistic standards, which... to me, I don't particularly care about as much (on the PHP side of things, PSR-2, in this case).
We're all remote, so we had a call - a couple actually - and I indicated I didn't care all that much, but he was free to make changes as he saw fit, as long as 1) he got a working environment up and running, 2) he was also spending time writing some tests around the code as he explored it, to have a firmer understanding of what was going on, and 3) he didn't break any existing tests (or new ones that were developed).
I swear that I don't think he'd actually had a working copy of the project, but was still committing code. Not a lot, but ... it broke tests. It broke stuff that was not 'tested' in an automated way yet, but was testable by running the app, and I think he'd just not bothered to test it at all.
But... hey - look at the formatting! Look - spaces, not tabs! - how useful when someone else is having to go back and debug your crap. I received a couple small lectures on variable naming and the importance of descriptive variable names.
function getListOfUsersForCompany() { $res = db::query('whatever query'); return $res; }
Apparently, that $res is confusing and 'bad practice' because it's "hard to reason" about what's going on there. Literally 2 line methods with an internal placeholder variable (which also had some working functional tests around them) - those were being criticized by someone who had committed test-breaking code multiple times.
In all this, I kept having to figure out how much of my reaction was because my code was being criticized, vs what the criticisms were. I got accused of taking things 'personally', but I kept coming back to 'working tests broke with these changes', and in my mind, that this code was considered somehow 'better' because it more closely followed a stylistic 'standard' was bothersome. Working tests should a standard we strive for too, right?
I have worked with some folks who had a real eye for analysis - remote or colo pairing on problems has helped reason out issues that I know I've missed. And when I'm pairing or doing a requested review, cosmetic style tends to be the last thing I'll bring up, because it generally has the least impact on something. Much prefer to have someone spend time writing tests and/or docs and/or insightful comments vs reformatting. When I've refactored with someone to make something more testable, better naming/comments/docs tends to flow out of that refactoring anyway.
While I understand formatting is often not that big of burden, especially with modern IDEs, it's also not terribly high value on many projects. I'm primarily speaking on projects I've come in to where there's already extant code - in this case, we had taken on a very weird hybrid of 2 PHP frameworks and 2 JS frameworks (some screens used raw vue, some compiled vue, and some compiled angular).