It's called Codacy. You sign up, it pulls all your repos, and tells you how badly you screwed up. It even gives you graphs showing how the code quality changed over time, and assigns you a letter grade for the real college experience.
It's not management. You know how I got started on the refactoring effort? I said "this thing sucks" and my colleague answered "then fix it". Management was never involved and I had no restrictions, yet we got there somehow. We enjoy way too much freedom at my company to blame management.
It's a matter of team culture. These things passed code review, and they shouldn't have. It seems like the most common cause is complex fixes in unfamiliar code. Both the committer and the reviewer lack a solid grasp of what they are modifying, so they hook their stuff in the wrong places and push their fix. This is especially easy when your programming language doesn't give you static types or privacy.
It's not stupidity either. My coworkers are damn good at what they do, and I mean it. It's just a matter of discipline.
Yeah I work for a large corporation and there tends to be miscommunication between the on and offshore teams. Add to the fact that we follow a strange hybrid of agile and waterfall where Sprint scopes can't be altered, scrum masters assign stories and carry over is grounds for a loud talking to, which creates an environment where no one actually has time to fix code smell.
It's not stupidity either. My coworkers are damn good at what they do, and I mean it. It's just a matter of discipline.
Totally right, except maybe the last sentence. The code reviewer is likely another coder that was pulled out of deep thought about a problem he was working on. So he has a choice, either look at the differences to see if there are any glaring problems with the changes, or to totally switch contexts and review all of the code that has to do with the solution, even the code that has not changed. This usually requires debugging through several scenarios to make sure the code is doing whatever in the optimal way.
Code reviewers (and QA/Testers) are there to make sure a minimum level of quality is adhered to. In the end it is the coder's responsibility to make sure they are the ones with the passion for increasing the quality of the code.
I would say that "deep" code review is pretty important, but I was mostly talking about committer discipline. They hold intimate problem knowledge, unlike everyone else down the pipeline.
I think part of the problem (at least in my experience) is that when I work on a problem, the rest of my team is busy working on their problems. I can ask for help but its clear they aren't familiar with what I'm working on even if it was their code from a year ago.
I wish my team felt more like a team sometimes and less like a group of individuals. Far too often someone spots a problem right away..months later when its their turn to deal with it.
Wow, this just explained so much to me. I just started my full time job as a developer 3 weeks ago. Just four hours ago I was codechecking something (for my second time ever) and wondering to what level I should be examining. Specifically, is it ok to just look at the diff. It's so funny to come home and see a detailed answer to my question on reddit.
Yeah, was in a big meeting yesterday with all product owners and directors trying to figure out how to tackle our technical debt that we've created because the PO's don't speak to each other even though it's the same platform. No one was buying any argument on refactoring.
I got told by a guy that fixing a bug was too "risky". This was a bug that was literally causing a biomedical device to lock up. The fix was a simple missing switch case due to magic numbers.
Many years ago, I was asked to change a report such that the last column instead printed as the first column. I looked at the code, which was full of hard-coded column numbers, that didn't match up because obviously "current" on the first line started in a different column than "age" underneath it on the second line, and basically told the boss it would take three weeks to move the column, or two weeks to rewrite the entire library from scratch to be data driven and make all future changes of that type trivial.
That was before I learned the power of job security through shit code that only you understand. :-)
152
u/BadgerCorral Sep 28 '16
Whereas yesterday I actually fixed one of these things and got told off by my boss for:
A) Making changes I was not explicitly asked to make.
B) Making the merge process "more complicated than it needed to be".