5 ms·

The Boy Scouts have a rule: “Always leave the campground cleaner than you found it”. - Robert. C. Martin

by gitgud 8y ago

e40 8y ago
To this I would add: When in Rome, do as the Roman's do. When you are repairing other people's code, don't change it to your style, fix it in theirs.
- Jtsummers 8y ago
  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).
  - transpy 8y ago
    > 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.
  - gwbas1c 8y ago
    That's why a team should have code reviews; and everyone should keep an eye on what's changing. 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.
- sokoloff 8y ago
  Related, don't combine style changes and functional changes in a single commit.
  - Jtsummers 8y ago
    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.
- informatimago 8y ago
  Nope. See https://jamesclear.com/marginal-gains 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’.
  - lgessler 8y ago
    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.
    - Thiez 8y ago
      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.
    - hermitdev 8y ago
      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.
- deleted 8y ago
  [deleted]
- bluntfang 8y ago
  just have a robot handle your styles.
  - SatvikBeri 8y ago
    When I hear style I think, e.g., "Object-Oriented" vs "Functional" – not something you can easily automate.
emperorcezar 8y ago
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.

The Boy Scouts have a rule: “Always leave the campground cleaner than you found it”. - Robert. C. Martin

by gitgud 8y ago

The Boy Scouts have a rule: “Always leave the campground cleaner than you found it”. - Robert. C. Martin

e40 8y ago

To this I would add: When in Rome, do as the Roman's do. When you are repairing other people's code, don't change it to your style, fix it in theirs.

Jtsummers 8y ago
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).
- transpy 8y ago
  > 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.
- gwbas1c 8y ago
  That's why a team should have code reviews; and everyone should keep an eye on what's changing. 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.
sokoloff 8y ago
Related, don't combine style changes and functional changes in a single commit.
- Jtsummers 8y ago
  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.
informatimago 8y ago
Nope. See https://jamesclear.com/marginal-gains 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’.
- lgessler 8y ago
  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.
  - Thiez 8y ago
    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.
  - hermitdev 8y ago
    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.
deleted 8y ago
[deleted]
bluntfang 8y ago
just have a robot handle your styles.
- SatvikBeri 8y ago
  When I hear style I think, e.g., "Object-Oriented" vs "Functional" – not something you can easily automate.

emperorcezar 8y ago

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.