And this depends on who "owns" it. If you don't have communal ownership of code, don't make drastic changes without consultation. This is like rearranging someone's kitchen for them "because it's more efficient". It's not, because now they don't know their code anymore (I had this happen to me a few times, I know it needed work but the work done made it harder for me to make other changes later that the "helper" didn't know was planned).
If you'll be the one owning the code, do what you want. If it's not yours, check first.
If you do have communal ownership, coordinate and establish style guides, ideally using tools to establish conformance (rather than eye-balling it, this also leads to greater consistency).
> This is like rearranging someone's kitchen for them "because it's more efficient"
This randomly reminded me of the time I let my friend use my laptop, only to find out the next morning that he had changed all the colors and styles of my windows xp with a gimmicky "mac skin". So infuriating.
This is why I've been encouraging our junior developers (they're more responsive to these suggestions) to focus on small commits. Or rather, commits with a more focused intent.
As we've moved, here, from SVN style version control systems towards git we've encountered a few snags along the way. People used to SVN want to check out files, make a lot of changes, complete the whole change request in a single checkout (which could take a while), and then check it back in. The problem arises that they end up making tweaks (small refactors, style adjustments) that aren't really part of the change request. This creates confusion when trying to check the work back in and review it. It also means that positive changes (those tweaks usually are) get tossed out when the whole CR is rejected for some other reason.
Git makes it much easier to do small commits. Changed a file to use spaces instead of tabs (per style guide), check it in with just that change. Refactored a common set of lines into a new function? Check in the new function creation once. And then do one check in per true refactor (replacing the many lines with one function call).
This allows you to cherry pick much more easily, but also to isolate errors. Maybe that Extract Method refactor should only have been done to 10 of the 12 cases you did it in. Or what looked like a common magic number was only coincidentally the same. If you made all the changes in one commit, it's harder to revert just the broken part. And git bisect is useless if you have a handful of giant commits, rather than many smaller commits.
Nope. See https://jamesclear.com/marginal-gains
Code style, or worse, programming style (See for example FreeRDP vs. rDesktop), or even the choice of programming language, are amongst the little details that you’d thing don’t matter, but in the end of the day, they are the 1% worsening that accumulate direly.
So, the scouts’.
So, let's say that you work at an enterprise software company. Everyone writes in C#, and your team has a consistent style they stick to that is indisputably the best C# coding style.
Suddenly an SVP pulls you aside and tells you you need to extend a service that team SadStyle owns and integrate it with your own team's widgets. Team SadStyle has regrettably not yet been shown the way by a C# bodhisattva and has the following inarguably egregious C# style errors in its codebase:
1. Braces open on a newline instead of on the same line
2. Profligate use of `var` for every kind of variable declaration
3. Inconsistent use of tabs and spaces
4. Anonymous functions sprinkled into the code ad hoc
that are sometimes quite long
You're shocked at team SadStyle's code, but you need to get some work done, quick, since a deadline is coming. So what are you going to do?
If you answered "I'm going to go through and change every brace, variable declaration, and span of whitespace", I think you should maybe think more about whether this is a good use of your time.
Number 1 is the official c# style though... your bodhisattva was clearly a former Java programmer. Also I can't think of a reason for not using `var` everywhere, but I suppose that was your point.
Personally I try to follow the style of the existing code, but when something permamently becomes the responsibility of my team I won't hesitate to apply the style of my team to the whole thing. Most parts of the style can be applied automatically, and the parts that can't be will be applied when functional changes to that part of the code are required.
Your #1 & #2 fly in the face of commonly accepted C# style. #3 is unforgivable (Personally, I prefer tabs for indentation, spaces for alignment, but consistency is key). #4 depends on the function and how/when they're used. Anonymous functions are perfectly acceptable, but yes, they shouldn't be too large.
What I was hoping to signal by using sarcastic expressions like "indisputably the best C# coding style" and "inarguably egregious" is that the notion of an objectively best style is unsupportable. Coding style is to some extent fundamentally subjective, and someone should at least think very hard about why a style they disagree with might be unexpectedly good or just a matter of aesthetic taste before "enlightening" the codebases of others.
I've always received soooo much push back from this and I want to scream when devs wonder why the work they do in a project now takes 10x the devs it used to.
it’s kind of frustrating, cracking open old code for a change or bug fix and seeing some really gross helper function you have to use. the temptation to take 30 minutes and refactor is too irresistable sometimes.
I do agree with the basic idea of the article though.
maintenance/sustaining = touch the code as little as you can (helps with diffs)
greenfield = take the time to make it right for as long as you can
As I gain experience, I find myself more and more just gritting my teeth and using the nasty function as-is.
The business types don't care. What they care about is having something to sell.
The engineers who are even capable of understanding what I'm doing don't care because they've got their own deadlines on a totally unrelated part of the codebase. They probably won't even work here any more by the time someone has to touch this code again.
Remember that updating references is usually a global update. Now you have to test EVERYTHING that used that reference.
I've had so many projects bitten by an overzealous dev who decided to update the version of a library because newer = better and then we spent the next three weeks chasing bugs/differences between the versions.
Edit: Curious about the down votes. Would someone who disagrees care to illuminate me?
Not a downvoter, but when I saw your reply it made me think that you may need a better process around maintaining dependencies.
At least IMO, best practice is to continually evaluate and adopt updates to dependencies. I’d rather have a slow trickle of problems associated with continuous updates, than have a total firefight when you realize you have to upgrade, now, and you have neither solid plans and practice in place to do so, nor the time because you’re in the middle of yet another feature drop that is pushing tech debt to the breaking point.
Setting and forgetting dependencies is like saying you have a backup somewhere, but never actually testing if you can recover from it. That’s the kind of thing that could turn a routine outage into a crippling emergency.
I used to be a full time developer at a company where we only shipped new features. At some point, I wanted to go remote and part time. I've asked them if they'd be down if I worked in the shadows refactoring shitty code and replacing crappy modules. Which is what I'm doing right now.
For them, they pay me a fraction of a developer's price and get a constant improvement of the codebase.
For me, I work at my own pace, remote and part time. I live in a place where I don't need much money.
Hey, could I interview you about the negotiation process? about to release a book about it (https://codewithoutrules.com/3dayweekend/) and could always use more case studies.
That’s really interesting. Compared to what you could get on the open market, what’s “a fraction” mean? 1/2? 2/3?
It’s kind of hard for me to imagine moving out of the Bay Area, but I’m sure there are major cities outside California that one could easily live on 1/2 a software developer’s income.
I took 1/4th, but I was working for a very successful finance company in New York. If you don't count the yearly bonus, I took about 2/3rd.
But the calculation should not be about the fraction, it should be about what you need. If you make $100/hr and work 5hr/day, that's $110k with 5wk vacation. I make more than this and I moved to France, so I make a very decent living compared to the local standards.
I've done similar work, but what really gets me is new smelly code being produced faster than I can clean up. Have you experienced the same, and if so, how do you deal with that?
I can't say I have experimented this directly. I still give a lot of guidance to people whenever I see things that don't look right. But generally speaking the engineering team has gotten a lot better, and I'm mostly dealing with old legacy code noone's really looking at.
I think the right mind set if crappy code piles on is to think:
1. It's more work for me, so my employment is pretty safe
2. If I wasn't doing what I was doing, then the code would be even crappier. So all in all, this is my little contribution to a little cleaner world...
In my ideal environment, each time I would load a source file in my editor, it would be reformatted according to my own preferred formatting rules. And upon writing, it would be saved in some common "standard" format good for git diffs. This way each programmer can be presented the code in the best way he can read it, and the tools can work on a standard representation that they can best deal with.
I will note that some projects such as FreeRDP, already have an autoformat script that does the "saving" part (unfortunately, not entirely automatically, since the precommit-hook is not set to run it).
31 comments
[ 3.5 ms ] story [ 79.8 ms ] threadWhen you are repairing other people's code, don't change it to your style, fix it in theirs.
If you'll be the one owning the code, do what you want. If it's not yours, check first.
If you do have communal ownership, coordinate and establish style guides, ideally using tools to establish conformance (rather than eye-balling it, this also leads to greater consistency).
This randomly reminded me of the time I let my friend use my laptop, only to find out the next morning that he had changed all the colors and styles of my windows xp with a gimmicky "mac skin". So infuriating.
This way if someone decides to rearrange something, the stakeholders can hold up the review until they're happy.
I've certainly blocked code reviews because of planned changes.
That being said, you only "own" code if you own the business. If you think you "own" code, you might need to re-evaluate your ego.
As we've moved, here, from SVN style version control systems towards git we've encountered a few snags along the way. People used to SVN want to check out files, make a lot of changes, complete the whole change request in a single checkout (which could take a while), and then check it back in. The problem arises that they end up making tweaks (small refactors, style adjustments) that aren't really part of the change request. This creates confusion when trying to check the work back in and review it. It also means that positive changes (those tweaks usually are) get tossed out when the whole CR is rejected for some other reason.
Git makes it much easier to do small commits. Changed a file to use spaces instead of tabs (per style guide), check it in with just that change. Refactored a common set of lines into a new function? Check in the new function creation once. And then do one check in per true refactor (replacing the many lines with one function call).
This allows you to cherry pick much more easily, but also to isolate errors. Maybe that Extract Method refactor should only have been done to 10 of the 12 cases you did it in. Or what looked like a common magic number was only coincidentally the same. If you made all the changes in one commit, it's harder to revert just the broken part. And git bisect is useless if you have a handful of giant commits, rather than many smaller commits.
Suddenly an SVP pulls you aside and tells you you need to extend a service that team SadStyle owns and integrate it with your own team's widgets. Team SadStyle has regrettably not yet been shown the way by a C# bodhisattva and has the following inarguably egregious C# style errors in its codebase:
1. Braces open on a newline instead of on the same line
2. Profligate use of `var` for every kind of variable declaration
3. Inconsistent use of tabs and spaces
4. Anonymous functions sprinkled into the code ad hoc that are sometimes quite long
You're shocked at team SadStyle's code, but you need to get some work done, quick, since a deadline is coming. So what are you going to do?
If you answered "I'm going to go through and change every brace, variable declaration, and span of whitespace", I think you should maybe think more about whether this is a good use of your time.
Personally I try to follow the style of the existing code, but when something permamently becomes the responsibility of my team I won't hesitate to apply the style of my team to the whole thing. Most parts of the style can be applied automatically, and the parts that can't be will be applied when functional changes to that part of the code are required.
I do agree with the basic idea of the article though. maintenance/sustaining = touch the code as little as you can (helps with diffs) greenfield = take the time to make it right for as long as you can
The business types don't care. What they care about is having something to sell.
The engineers who are even capable of understanding what I'm doing don't care because they've got their own deadlines on a totally unrelated part of the codebase. They probably won't even work here any more by the time someone has to touch this code again.
I've had so many projects bitten by an overzealous dev who decided to update the version of a library because newer = better and then we spent the next three weeks chasing bugs/differences between the versions.
Edit: Curious about the down votes. Would someone who disagrees care to illuminate me?
At least IMO, best practice is to continually evaluate and adopt updates to dependencies. I’d rather have a slow trickle of problems associated with continuous updates, than have a total firefight when you realize you have to upgrade, now, and you have neither solid plans and practice in place to do so, nor the time because you’re in the middle of yet another feature drop that is pushing tech debt to the breaking point.
Setting and forgetting dependencies is like saying you have a backup somewhere, but never actually testing if you can recover from it. That’s the kind of thing that could turn a routine outage into a crippling emergency.
For them, they pay me a fraction of a developer's price and get a constant improvement of the codebase.
For me, I work at my own pace, remote and part time. I live in a place where I don't need much money.
It's awesome.
Ping me at itamar@codewithoutrules.com
It’s kind of hard for me to imagine moving out of the Bay Area, but I’m sure there are major cities outside California that one could easily live on 1/2 a software developer’s income.
But the calculation should not be about the fraction, it should be about what you need. If you make $100/hr and work 5hr/day, that's $110k with 5wk vacation. I make more than this and I moved to France, so I make a very decent living compared to the local standards.
I think the right mind set if crappy code piles on is to think:
1. It's more work for me, so my employment is pretty safe
2. If I wasn't doing what I was doing, then the code would be even crappier. So all in all, this is my little contribution to a little cleaner world...
I will note that some projects such as FreeRDP, already have an autoformat script that does the "saving" part (unfortunately, not entirely automatically, since the precommit-hook is not set to run it).