55

I got this email from my manager this morning;

Hi ____,

Are you changing the spacing in the JS files that you modify in _____. Every JS file that you have updated has it’s spacing drastically changed which makes merging any ‘real’ changes into the release extremely difficult. If you are not doing this, we need to look at your IDE and stop it doing it.

Now yes I am reformatting the JS files that I use when I have a job that touches the file. There's a very quick extension in Brackets.io that does this for me, and meets the coding standard of the business. (So no I'm not wasting time doing this by hand.) (The only thing being changed by the indenter is obviously 'indentation', changing spaces to tabs, and normalizing the indentation across the document.) (The Only Changes Are Indents)

I'm doing this because the formatting across the document is all kinds of messed up, and with a single keystroke I can bring it all back to standard, and not get losses/confused when working through a document when the indentation suddenly changes.

My manager who sent the email is the person who created the coding standard for the business, forces us to comply with some banal/prosaic things in the coding standard, like "Curly braces must be on a seperate line from the function definition and before the function body, this is for legibility purposes" (Yes that's fine a common standard in some places)

Now if something so simple as curly braces being in the correct place, is important to the coding, am I right to think that I'm justified to respond with something akin to 'suck it up, it's your coding standard, and actually helps the flow of the code, it's only an issue because you've been doing it badly for so long.'

Or more tactfully, what is the workplace way of doing your job to the standard required by the business, while your manager is forcing you to do a sub-standard job. Being that it's "Your responsibility to ensure that all work conforms with the standards and policy documents provided to you, ... it is your responsibility to bring to light or avoid any circumstances that would have a negative outcome for the wider business" (Thank you staff manual)

FYI: Based in England, UK.

EDIT Just a note since it seems popular in answers, I tried the Submit formatting changes separately before beginning my actual work on the file, and this was met with an instant "Don't do that" So although this seems a popular opinion answer, it has not worked for my situation (even though I agree that it should be the simplest intermediate solution) Though this may work for anyone else in the same situation.

closed as off-topic by gnat, IDrinkandIKnowThings, Masked Man, Chris E, user42272 Nov 2 '16 at 0:29

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • 105
    It sounds like you don't understand why reformatting code in the middle of other people's changes is a bad thing. The answers you've got explain how to deal with the workplace issue here, but I suggest you do some reading / talk to your team about how you're causing them problems. – Philip Kendall Oct 31 '16 at 9:31
  • 73
    The decision to do a complete reformat is a strategic one, not something a new employee should be deciding. If the rest of the team, and especially the manager who wrote the coding standards, agree it should be done, do it as suggested in this answer. All you should be doing is making sure the narrow areas you need to edit conform to the standard. – Patricia Shanahan Oct 31 '16 at 9:45
  • 34
    It's a nightmare to review code when there is a mix of formatting and actual code changes. I have to look at every line in the PR and figure out, "formatting change? functional change?" which is a lot of extra work. – enderland Oct 31 '16 at 13:46
  • Comments are not for extended discussion; this conversation has been moved to chat. – Jane S Nov 5 '16 at 8:26

12 Answers 12

144

Don't mess with indentation.

I write this with a heavy heart because I, too, like to see everything neat and aligned. When you do, however, you override the changelog of a particular line, so a particularly complex piece of code that read fixed that nasty bug that happened when X was going on now reads indented code.

The only solution to implement a company-wide style is to have your CVS or build system do it with a commit hook or something like that. Automate it, in other words. Preserving the history of the codebase is much more important than having uniform style across it.


When I was first hired I did the same thing to some ancient Java files, and was told off by my manager much in the same way as you were. At the time I was told that someone else was working on that file, and they were getting a bunch of conflicts. From this I gather it's a common thing to do as a newb.


edit: Since comments are transient I thought I'd incorporate this excellent piece of advice from SJuan76:

Just to be sure: these changes are something to suggest to your boss, don't try to implement them yourself. And BTW, it would be nice to explain before something like "I did not realize the issue with CVS and I'll be more careful in the future"; if you just tell your boss "we could add a commit hook" you could look like someone who does not allow himself to be corrected.

  • 22
    Just to be sure: these changes are something to suggest to your boss, don't try to implement them yourself. And BTW, it would be nice to explain before something like "I did not realize the issue with CVS and I'll be more careful in the future"; if you just tell your boss "we could add a commit hook" you could look like someone who does not allow himself to be corrected. – SJuan76 Oct 31 '16 at 12:57
  • 11
    Good IDEs will either default to, or allow you to, use whitespace-ignorant annotate, so it's a somewhat weak excuse. But yeah, for ease of review, if you do need to reformat code, do it in a separate commit, then your actual changes in subsequent ones. – AKX Oct 31 '16 at 12:58
  • @AKX, it sill makes merges a lot harder and slower..... – Ian Oct 31 '16 at 13:18
  • 3
    @AKX I used to use the "ignore whitespace" feature in Eclipse, then found out that, while whitespace changed allright, I wasn't made aware of it. Then one day when I thought I had only formatted my new method, and checking to make sure in the diff viewer, my 2-line fix turned into a 1k+ LOC whitespace rearrangement. – rath Oct 31 '16 at 13:56
  • 1
    As QA person I love and hate formatting standards. If it is an enforced standard then it is great. It makes things much easier to read and follow, especially when there has been development over time by many different people. On the other hand there is nothing worse than a huge code check in that is 99% formatting and 1% actual changes. Sure there are different tools that will try to be intelligent about picking out the actual changes but those always have limitations. Please don't ever mix mass formatting changes and actually updates into a single commit. – Evan Steinbrenner Oct 31 '16 at 20:39
86

As your manager has told you, this is indeed bad practice. Yes, it's good that you're bringing the files up to the standard, but there is a better way.

First of all, apologise for the inconvenience. After that, you can (and should) plan a day where you will automatically re-indent all files in the project. Make sure you let everyone working in the same codebase know that this is going to happen, and you would like to get their input as to when. There's not a chance your co-workers will get merging conflicts with your changes: they will definitely get them. This is why you should do this in a single go, so that it doesn't have to happen every time (as is happening with your current workflow).

I have had the exact same situation in my workplace, and we re-indented all our javascript files in a single go after our sprint had ended, when no-one had any pending changes.

  • 14
    Even better, have a commit hook to do indentation on its own, so that everyone's code is indented the same by default. – rath Oct 31 '16 at 10:17
  • 4
    @LightnessRacesinOrbit actually in the UK, practice and practise are two different words. I'm also pretty sure in the US they exclusively use practice. – ediblecode Oct 31 '16 at 10:26
  • 6
    This is definitely the correct approach - changes to indentation or formatting should always be done in as few commits as possible, and after letting the team know in advance that it's going to happen so they can commit everything first. The only thing I would add, is to always check with manager(s)/co-worker(s) that this is even worth doing at all. Hey, I've noticed that the indentation is all over the place on this project - do you mind if I fix it all in one go? - then schedule a time convenient to everyone to fix it. If they don't want it done, at least you've made your view clear. – zelanix Oct 31 '16 at 11:17
  • 2
    Often there is NEVER a time when you don’t having many branches, due to customers with old versions on support contracts, therefore just accepting that the indenting can never be sorted out is the best option, unless the manger tells you to sort it out. – Ian Oct 31 '16 at 13:22
  • 3
    @Ian Or you accept that with merging, the burden is on the quy who came second, so if someone worked on a file while the indentation was fixed, he'll just have to merge the file properly. Which isn't that hard, considering that an automatic merge will get it right. – Peter Oct 31 '16 at 15:57
57

Obviously you're not following the coding standard for your team, hence the email. There are technical arguments to be made that defend your manager but ultimately, even if he were completely wrong he still told you directly that you need to stop changing the spacing. Orders don't get that much more explicit and at the end of the day the fact that you're being paid for your time means that people higher in the chain than you get to say how you spend that time. And that includes complying with an arbitrary coding standard.

how can I say the above, in a tactful way, that won't get me dismissed

There isn't. What you can do is ask your manager how spacing should be done in the coding standard and if your way is indeed more readable or matches the standard more closely you can make an argument for migrating the code base in an automated process. But that's it.

And keep in mind that "suck it up, I'm following the letter of the law" is a very, very poor attitude to have as an employee and incredibly adversarial. I'd suggest revisiting the way you look at workplace dynamics so it's less "me versus them" and more focused on "working well with others".

  • 5
    It sounds more like he is following the team coding standards, but other members of the team are (or were) not. If the documented standard is different from the de facto standard, then that is an issue for the team manager to resolve, but until that happens he is right to follow the documented standard when writing new code. – thelem Oct 31 '16 at 18:00
  • 1
    @thelem, But, there is a big difference between following the coding standards for code that is newly written and changing the formatting on the entire file which is being edited. Yes, the OP should use the coding standard for any new code (unless otherwise instructed). However, the suggestions in the answers to this question should be followed regarding making changes to the entirety of a file, or the entirety of the code base. – Makyen Oct 31 '16 at 19:05
  • 1
    @Makyen You and a fair few others seem to have the impression that you're on Software Engineering, but this is not a technical site. – Lilienthal Oct 31 '16 at 20:17
  • 3
    @Lilienthal, I'm not sure how you get that from my comment, or get that I am a reasonable person to single out for such a comment. I agree that on this site the issue should be more focused on following the directions provided by the boss. However, I stand by my statement that there is a big difference between following written standards for new work and going through correcting all work done previously to be compliant with those standards. My comment would be the same for other fields under the same conditions. – Makyen Oct 31 '16 at 20:30
  • 2
    +1 for also addressing the "suck it up" attitude, which while it's bad from anyone, is totally inappropriate from someone new to the team/in their probation period, where they should be focussing on how to fit into the team. – Gwyn Evans Oct 31 '16 at 23:19
40

I have experience with this, so I know where you're coming from (I was in the same position about 5 years ago with my current boss). In this case, you're making two errors in judgement:

  1. You're making a major code change without your manager's permission.
  2. You're doing it in the wrong way by interleaving code formatting changes with actual important changes.

While "just changing the code formatting" sounds like something very minor, it can have a big impact on file diffs in version control, and make code reviews immensely complex.

For example, if your code file is indented incorrectly (Joe used spaces instead of tabs for indentation, that heathen), and you correct it and check it in, the entire file is going to be highlighted in the diff:

Diff of a 11 line file after a "minor" formatting change

This makes code reviews nearly impossible (this is particularly important since you're new and in your probation period, your code will almost certainly be getting vetted by a senior dev). The file above is 11 lines long, and the whole thing is marked as changed because of 1 indentation change. If you or I were reviewing a file a few hundred lines long, it would be a nightmare to review. In addition to this, if I were making simultaneous changes to the same file, it would 100% cause a merge conflict and require effort to fix.

While in the long term making these changes will certainly be beneficial, my advice would be to code to the standard with your new code, and leave existing code as it is.

When bulk changes are required in the codebase at my current employer, these changes are planned weeks or months in advance so that devs can plan their commits around it. In general, these changes are scripted, tested and then executed on the entire codebase in one sweep so that there is only a single commit containing these changes.

Now if something so simple as curly braces being in the correct place, is so important to the coding, am I right to think that I'm justified to respond with something akin to 'suck it up, it's your coding standard, and actually helps the flow of the code, it's only an issue because you've been doing it badly for so long.'

Telling someone to "suck it up" and obey their own code standards is an easy way to get yourself disciplined. Within your probation period you certainly don't have the seniority to be making such assertions without having a background to why these inconsistencies exist. Don't do that. Ever.

  • 8
    This is all true - and pointing out that most diff tools for code reviews have an "ignore whitespace" feature doesn't mitigate it, unfortunately. Because people (your teammates) have their habits. (Plus, "ignore whitespace" doesn't help with the merge, which was mentioned specifically by the OP, and really, that's the most important thing as it is the most time consuming and error prone part of the process.) – davidbak Oct 31 '16 at 14:02
  • 2
    +1 for talking about when to do the changes properly -- with advance notice and in concert with the whole team – brian_o Oct 31 '16 at 15:14
  • This is so correct. Nothing is worse than having to fix dozens of merge-conflicts because of something like this. Or having git blame become totally useless... – Radu Murzea Oct 31 '16 at 22:05
  • A decent comparison tool should be able to ignore whitespace differences. Languages which syntactically belong to the C family like Java are white space ignorant so that a meaningful comparison should ignore them, too. (I don't know any which ignores newline changes, admittedly, but that was not the OP's problem.) If you compare Occam programs you should stop ignoring whitespace though... – Peter A. Schneider Nov 1 '16 at 17:25
  • Apparently tortoisegit has a variety of options concerning whitespace which kindof totally invalidates your point. See tortoisegit.org/docs/tortoisegit/tgit-dug-diff.html. @RaduMurzea These will (if the docs are right) affect blame and merge so that, in fact, changing indentation causes additional work (beyond a mouse click for the option) for exactly nobody. – Peter A. Schneider Nov 1 '16 at 17:39
29

If your changes create unnecessary work for your co-workers, then it doesn't matter one bit if these changes meet coding standards, because you are creating unnecessary work for your co-workers, and that is the ultimate sin.

What you can say in a tactful way is "I'm sorry and I won't do it again".

  • The big IF in the room is unnecessary. In my opinion the ultimate sin is to avoid actions which have a long-term benefit (like, properly formatted, readable code) for short-term benefits (like, one less revision in the version tree, one merge less for a colleague). The formatting may not be that important, but it is part of maintaining the code base which is generally seen as a crucial part of successful software development. I agree with the general feeling here that the OP should have asked; but I think he should not be discouraged but rather directed. – Peter A. Schneider Nov 1 '16 at 17:33
  • @PeterA.Schneider while I agree with your opinion on long-term benefits vs. short-term benefits, keep in mind when they're mutually exclusive and when they're not. I think this is an example of the latter, since there are ways to bring the code base up to the style standard without the merging conflicts. We get to have our cake and eat it too this time! – Lord Farquaad Jul 25 '17 at 21:22
19

I would ask the same of my team: keep commits atomic. If it's not a style-changing revision, your actual change got lost in the noise and you just created a heck of a lot of work for anyone who wants to merge/rollback/anything your revision.

Save it for some dedicated revision, and even then if it's a mass change, do it with the consent and preparation of your wider team.

16

There's a more serious wider issue here

This isn't just about the correctness of indentation and committing changes that impact everyone else.

The larger problem is that you've taken it upon yourself to impose a change without talking with your manager or the rest of the team.

While it's tempting to scatter-bomb changes into code/procedure because you feel it's a good idea, there's most probably an equally good reason why those changes aren't already in place.

You'll get a heck of a lot more respect for raising these as discussion points in weekly/daily meetings/stand-ups and proposing ideas there than taking it upon yourself to make changes.

  • 1
    +1 totally agreed. You're not at home at your computer tinkering away at some code anymore. You're now part of a team which is itself part of a department with many more moving parts that all have to collaborate towards successful deliveries/deployments. Dropping such a bomb in the repository is not going to help your popularity with your coworkers. Or your boss. Or the client (if there is one). – Radu Murzea Oct 31 '16 at 22:11
  • 1
    +1 Unless your task and your commit is "refactoring indentation", do not mess with indentation, no matter how bad it is. – Agent_L Nov 1 '16 at 14:00
4

Raise the issue that indentation is inconsistent with the coding standard

The fact that there are so many files that (apparently) require reformatting is itself an issue that needs to be raised. We have a couple of possibilities here:

  1. Your IDE settings are incorrect; perhaps you were looking at an out-of-date copy of the coding standards, or perhaps in your current project the team have unofficially adopted a different set of standards
  2. Some member(s) of your team have incorrect IDE settings. If so would be good to raise this with your manager to help stop the proliferation of incorrect indentation

I have been tempted with the 'suck it up, it's your coding standard' line of thinking myself in similar scenarios, but this assumes that the manager knows that the problem exists. Basically, raise the issue then let your manager manage the problem; this shows that you want to work as a team, and hopefully will provide the best outcome.

  • 1
    I feel that this is the correct answer. Talk to your manager (and team) about coding standards and code reviews, and have a sprint dedicated to bringing in correct standards. Using tools like Lint and Phabricator religiously actually end up saving money. But this is not something you should be driving yourself - it needs to be a team-wide effort driver by the manager. – PeteCon Oct 31 '16 at 15:19
2

From your point of view, they have coding standards yet don't want you to follow them. Because this makes the coding standards pointless, that makes them unprofessional.

From their point of view, you make multiple independent changes to a single file, and mix them into a single commit. Because that makes merging and tracking changes unnecessarily difficult, that makes you unprofessional.

This is a classic communication issue. They want merges to be easy, and you want to be allowed to follow the coding standards. Both of these are very reasonable requests. And you can fulfil both of them at the same time, in various ways, but neither of you bothered to think of one of these ways.

Very important to keep the following in mind in a business environment:

When the person I'm talking to sounds like a moron, either they are a moron, or I failed to understand their point of view - i.e. I'm the moron.

EDIT: Since I strongly disagree with some of the answers, I feel that I need to address another point:

Not fixing the whitespace issue, as advocated by some people here, is a ridiculous "solution". The only reason the OPs problem even exists is because in the past someone followed that advice. Besides the obvious point that there is a clear benefit which comes at a cheap cost if done right, there is a much bigger reason for fixing the whitespace issue: If you prevent your employees from improving anything, they will learn that they must not care about improving anything. Think of it from the management perspective: Do you want to fix issues that arise because your people don't fix problems, or do you want to fix issues that arise because your people do fix problems?

1

The problem here is not with the standard or your action.

The problem is with your tool chain.

Code merge/conflict tools that don't deal well with mass whitespace changes.

Attribution tools that tell you what was the reason a line of code was modified that don't deal well with whitespace changes.

Between the two, the cost of the change is much larger than the benefit.

If you want to make that kind of change, you should look at the cost to benefit ratio. And as it is not just you bearing the costs, the most reliable way would be to find a way to fix the tooling.

Fixing the tooling could be extremely hard. Changing source control providers or IDE integration tools or merge tools requires a lot of care and attention.

Simply saying "I don't care about merge conflicts or annotation" is not being useful. Rules are not to be followed blindly.

Clearly on your team those coding standard rules apply to newly created code, not something you should use to sweep existing code. If and only if you can find a way to sweep the existing code without costing annotation or causing merge conflict headaches would your action be clearly great.

0

Always limit your commits and pull request to as little changes as possible. It makes code review faster, and limits the risks of conflicts.

Put yourself in the code reviewer shoes and imagine receiving a 1000+ lines code review because the dev has reformatted the entire file, for a change that should have taken 20 lines.

Put yourself in the shoes of the next dev who is working on a feature branch, and gets conflicts on all their modified lines, because meanwhile you changed the formatting on a part that you were not expected to touch for your feature.

To that end, only fix the formatting of the lines you actually work on, if that is relevant. (Keep the indentation consistent with the surroundings). Try to avoid anything in your PR that is not directly related to your task.

0

Sorry, I had configured my IDE to automatically enforce the coding conventions, in order to avoid inadvertently committing changes not following it.

Seems we have a number with the wrong conventions and it has been enforcing them too much. I have completely disabled this feature now.

Should I take any other precaution regarding coding conventions for existing files?

Then I would bring up in some meeting (with your boss or the team) the option of automatically fixing the coding convention for existing files at a time (such as the end of the sprint, as suggested by Stephan Bijzitter).

You are stating

  • the base problem (some files had the wrong coding convention)
  • the source of these spacing changes (you had a code formatting)
  • your intention (comply with their code conventions)
  • and desire to solve the issue (by disabling it)

your boss will probably tell you to make your edits following whatever convention the original file has.

Ideally you will make a big formatting change to the codebase at some point. Making small changes while you edit is nice (eg. the lines you are already touching). But changing the indentation of the whole file is not only affecting other people with pending commits, also makes harder later to realise what you did change. IMHO those should have been two commits anyway.

Not the answer you're looking for? Browse other questions tagged or ask your own question.