Coding Standard - haphazard application [closed]

Question

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.

Answer 1

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.

Answer 2

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.

Answer 3

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".

Answer 4

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:

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.

Answer 5

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".

Answer 6

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.

Answer 7

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.

Answer 8

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.

Answer 9

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?

Answer 10

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.

Answer 11

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.

Answer 12

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.