68

I'm thinking of creating a cron job that checks out code, runs code formatters on it, and if anything changed, commits the changes and pushes them back.

Most projects that use autoformatters put them in a git hook, but doing it automatically every few hours would remove the burden for each dev to install the git hook.

I would still encourage everyone to write clean, well-formatted code, and maybe I can have the system automatically ping devs when code they wrote gets reformatted, so they know what to do in the future.

  • 105
    There's a logical error here. This method wouldn't encourage people to write well-formattted code; rather it would encourage people not to format and rely on the system instead! If your concern is that the code base is coherent, that's okay. It your goal is to train coders to write cleanly, it's a huge bug. – Kilian Foth Apr 12 '17 at 6:24
  • 52
    It's also with considering the effect this will have on features such as blame - espsecially if the formatter makes a lot of changes, if most lines in the file are marked as being changed by the formatter, you loose some of the value. – Neil P Apr 12 '17 at 8:38
  • 13
    I've had issues with an autoformatter not being updated properly and breaking my code. Something to keep in mind. – isaacg Apr 12 '17 at 9:01
  • 22
    If it is that important to you, you could fail the build when code in not formatted correctly. That is a bit harsh but a proper peer review would more or less do the same thing. – Erno Apr 12 '17 at 12:59
  • 18
    This is an excellent idea if your goal is to get people to hate you. It achieves very little, but will certainly cause unforeseen problems. – TonyK Apr 12 '17 at 14:08

13 Answers 13

129

Sounds nice, but I would prefer to have people responsible for committing code changes, not bots.

Besides, you want to make absolutely sure that those changes do not break anything. For example, we have a rule that orders properties and methods alphabetically. This can have an impact on functionality, for example with the order of data and methods in WSDL files of WCF contracts.

  • 50
    Also it kinda breaks the VCS. You won't be able to know who wrote a line easily with blame, and if there is a conflict, your VCS won't be able to know the tool's changes are just for style and should be discarded everytime. – Pierre.Sassoulas Apr 12 '17 at 10:13
  • 2
    A tangentially related topic is enforcing certain "save actions" in an IDE like making fields final if possible. It's an issue because (In Java at least) a lot of frameworks set them through reflection (e.g., Spring and Hibernate). – Captain Man Apr 12 '17 at 13:25
  • 20
    @JeffO git blame isn't just to find whose fault a bug is, it's to find the commit when something was added/removed, which can be useful for a number of reasons. – Pokechu22 Apr 12 '17 at 16:12
  • 2
    "we have a rule that orders properties and methods alphabetically" Boy, that sounds awful! You would have to constantly be hoping around all over the file – Alexander Apr 14 '17 at 21:47
  • 1
    Also for Java the combination of a style checker which looks for derivations from the agreed upon style (perhaps even making it a compilation warning), and an IDE configured for formatting the agreed upon style makes it very easy to detect and fix. It is important that code settles (for lack of a better word) when it actually changes functionality - not when a bot reformats it. – Thorbjørn Ravn Andersen Apr 15 '17 at 12:01
72

I would instead try to make it really easy for everyone in the team to apply automatic code formatting according to your team's standard directly inside your editor or IDE, to the current source code file (or selected parts of it). This gives your team members more control about how and when the formatting takes place, let them inspect the code before it is committed in the final form, and test it after the formatting took place, not before.

If all or most of your team members use the same editor, this should not be too hard. If everyone uses a different one, then your approach might be the second best solution, as long as the team supports this. However, I recommend you have extra safety measures installed, like nightly builds and automated tests which are run after each automatic code modification.

  • 7
    "A formatter at hand where you are 100% sure it does not break anything" - When have you ever run into a piece of code that you were, honestly, 100% sure would never break? – Zibbobz Apr 12 '17 at 13:29
  • 2
    @Zibbobz: any of the tools we are using to edit code, to compile and link it, and also the VCS, can have bugs, still that does not stop us trying to develop working programs ;-) But see my edit. – Doc Brown Apr 12 '17 at 14:07
  • I do approve of the edit - if only because an extra ounce of caution is worth a pound of debugging. ;) – Zibbobz Apr 12 '17 at 14:15
  • @Zibbobz Quite often! Note that being 100% sure of something doesn't mean it has a 100% chance of being true. – user253751 Apr 15 '17 at 11:45
  • @immibis: you may have missed that the comment referred to a sentence I removed from my answer some days ago. – Doc Brown Apr 15 '17 at 13:07
37

It's a bad idea, not just because it discourages people from writing decent code, but also because the reformatting will show up as code changes in your VCS (you do use one I hope), masking the historical flow of the code's development. Even worse, every code formatting action (indeed every change to the code at all) has the possibility to introduce bugs, whether it's manual or automated. So your formatter can now introduce bugs in your code that aren't going to go through code reviews, unit testing, integration testing, etc. until possibly months later.

  • 10
    I think if he's talking about having a bot check things in and out of a repository, VCS is a given? – StarWeaver Apr 12 '17 at 7:35
  • 11
    @StarWeaver you'd think. But I've worked in places where the "code repository" was a shielded directory only accessible by a piece of software running under its own user account, no version control at all except a weekly backup of the directory under a timestamped directory name. – jwenting Apr 12 '17 at 7:40
  • 14
    That's … I'm going to go have nightmares now >.> – StarWeaver Apr 12 '17 at 8:00
  • 7
    @StarWeaver why do you think I still remember that environment 14 years later ;) – jwenting Apr 12 '17 at 13:47
28

I would tend to believe it is a good idea (to automatically run code formatters), but that is just my opinion.

I won't run them periodically, but if possible before version control commits.

With git, a pre-commit hook doing that would be useful. In many C or C++ projects built with some Makefile, I am adding some indent target (which run suitably code formatters like indent or astyle) and expect contributors to run make indent regularly. BTW, you can even add some make rules to ensure that the git hooks have been installed (or to install them).

But really, it is more a social issue than a technical one. You want your team to commit clean and nicely formatted code, and that is a social rule of your project. (There is not always a technical answer to every social problem).

Version control is mostly some tool to help communication between human developers (including your own self a few months from now). Your software don't need VC or formatting, but your team does.

BTW, different communities and different programming languages have different views on code formatting. For example, Go has only one code formatting style, but C or C++ have a lot of them.

  • Right, but that means every developer needs to install the commit hook. – bigblind Apr 12 '17 at 6:31
  • 17
    Yes, but that is a social rule, and you need several of them. You cannot find a technical solution to every social problem. You need to convince people. Also, every developer need to install some compiler, and the version control system itself. – Basile Starynkevitch Apr 12 '17 at 6:32
  • Right, so you're saying the social rule of having to install a git hook, is better than an automated system that does it? (serious question, not meant as a rhetorical one, tone is hard to convey on the internet :)) – bigblind Apr 12 '17 at 6:35
  • 15
    Yes, I am saying that. – Basile Starynkevitch Apr 12 '17 at 6:37
17

I think it's a bad idea. Many of the answers already covered that it dirties the history by making it difficult to pin down who actually added a line and that it encourages people to just commit whatever and the format-bot will handle it.

A better approach would be to incorporate a format checker into your build tool. (In Java there is Checkstyle) Then, only allow people to merge their branches to the main branch if the build passes (including the formatting).

If you allow people to commit straight to the main branch (like in Subversion for example) then you will still need to make sure everyone has the discipline to commit only formatted code (or have the server only accept the commits once some checks have been run).

  • 2
    but do make sure the style checker is sane and doesn't enforce weird things that go counter to any accepted (and acceptable) practice (and yes, I've seen such). You can then also have the build server have the automatic build fail when the style checker throws (new) errors. – jwenting Apr 14 '17 at 6:13
  • 2
    I have seen many cases where automatic formatting produces unreadable code. Especially for lots of closing parens (it is sensible to leave some of them on separate lines, but the robot does not care). Another example is automatic splitting of long Java strings (they are long because they contain SQL queries, but the robot has no idea.). – 18446744073709551615 Apr 14 '17 at 17:28
  • @18446744073709551615 I'm not suggesting automatic formatting, I'm suggesting automatic checking. Your rules could allow those things. – Captain Man Apr 14 '17 at 21:27
16

In general, I think it is a bad idea. In principle it's a valid idea, but it can be problematic in reality. Having the code formatter break your code is a real possibility, and it only takes one formatting run to have your developers respond with (probably justified) hostility (e.g. "Your lousy code formatter broke the build, turn it off now!").

In the same vein as @BasileStarynkevitch's recommendation, we use git server-side post-receive hooks to send "advisory emails" about code style.

If I push a commit that contains style violations, the git origin server will send me an email that informs me that I broke the style guidelines and recommends that I fix my code. It does not, however, enforce this because there may be valid reasons for breaking the house style (long strings exceeding line length limit, for example).

If it's a systemic problem that is harming the codebase, it might be time to start bringing up code style issues in code reviews. Poor code style may mask bugs and make the code harder to read, so it can be a valid code review issue.

To add to the "social problem" aspect of things, it can be worthwhile encouraging people to fix cosmetic and stylistic defects as they find them. We have a standard commit message "Cosmetic." for code style fixes that other developers know do not contain breaking changes.

As @DocBrown says, another option is to enforce code style within your IDE. We use CodeMaid with Visual Studio to correct many common style errors. It will run upon saving the code files, meaning that bad-style code should never even make it into the repo... in theory :-).

  • I upvoted this one, because it directly addresses both sides (desire for better code + safety from breaking the build). After the code base is in really good shape, though, I'd consider "bad idea" to be somewhat strong wording for automating future changes. – donjuedo Apr 12 '17 at 14:19
10

Yes, I think it's a bad idea. Don't get me wrong, the reason to do it sounds great, but the result could still be horrible.

You will have merge conflicts when pulling a tracked branch, atleast I fear that would be the case, I might be wrong though.

I don't want to test it right now at work, but you should try it out yourself.

In fact you can just check out a recent commit. Make a new branch, commit something petty, cherry pick or merge with no autocommit.

Then run your script, pull and if your result is a horrible merge mess, then you should definitly not do this, at daylight.

Instead you could potentially put it into a nightly build or a weekly build.

But even a nightly might be a bad idea.

You could either run it weekly, when you are sure no merge conflicts will arise because everything is finished on monday.

Otherwise run it 1-2 times a year on holiday season, when merge conflicts will not occur.

But the solution might depend on your priority for code style.

I think that making a setup script that automatically creates the git repository and sets the hooks for the project would be better.

Or you might include the hook setup script in a folder for your devs within the project and simply check it into git itself.

7

Something I haven't seen mentioned is the fact that sometimes there are legitimate reasons to not format something according to a set of rules. Sometimes the clarity of the code is improved by going against a given guideline that 99% of the time makes sense. Humans need to make that call. In those cases, automatic code formatting would wind up making things less readable.

  • Totally agree. For example, aligning variable identifiers on the left, all the equal signs in a single column, and all the values to the right of equal signs. A formatter would depreciate readability in this case by reducing the tabs/spaces each to a single space. Of course formatter rules could be written to prevent this but then you need a developer assigned just to writing the code formatter. Seems like a bad idea all around. Adopt better in-house conventions and enforce during code review. – ThisClark Apr 15 '17 at 3:27
7

It’s an awful idea.

If one of my developer colleagues made gratuitous changes to source files, it wouldn’t pass a code review. It just makes life harder for everyone. Change the code that needs changing, nothing else. Pointless changes lead to merge conflicts, which can lead to errors, and just create pointless work.

If you want to do this on a regular basis, that’s just awful.

And then there is the question of what kind of changes the code formatter makes. I use automatic formatting in my editor, it works reasonably well, and I can improve things when the automatic formatting is less than perfect. If you use a code formatter that goes beyond that, you are not going to improve the code, you are going to make it worse.

And then there is the social problem. There are people who want to force everyone to use their code style, and there are more flexible people. Something like this would likely be suggested by the "grammer-nazi" (spelling intentional) type of developer who want to force their style on everyone else. Expect a backlash, and expect the flexible, normally easy-going developers to put their foot down.

4

You don't mention the VCS you use but depending on that another option is to have a server-side hook. A VCS like git supports that. You could install a sever-side hook that runs the formatter on the version being pushed and then compares the formatted file against the version that is being pushed. If they differ the developer did not use the correct formatting and the server could reject the push. This would force your devs to only push code with the desired formatting thus encouraging them to write clean code from the beginning, it would make the devs responsible for having tested the correctly formatted code and relieve your devs from having to manually install a client-side hook.

  • This is what Go does, for example, except using Mercurial. Pushing something that is not invariant under go fmt is automatically rejected. – Jörg W Mittag Apr 12 '17 at 8:39
1

It's a trade-off between a cleaner code format and more exact and easier to understand git history. Depends on nature of your project and how often do you dive in git history or blame to understand what's going on. If you're working on something new and don't have to maintain backwards compatibility, history usually doesn't play as important role.

1

This idea is similar to some other answers, but I cannot comment my suggestions.

One option is to set an alias (or hook, or whatever) to the commit function, that runs the code formatter on the to-be-committed code before it is committed.

It could have 2 (or more) results:

1) Show the user the proposed changes and ask for their approval to apply and commit the changes.

2) Ignore the proposed changes and commit the original code.

You could also add more flexibility to these options, like the ability to edit the changes proposed in option 1. Another idea (Depending on how hard you want to push these coding standards) is to have the system send you a report of some sort when option 2 is selected.

This may be a good balance of auto-checking all code like you want, while still allowing flexibility to the developers to suit their needs. It also allows the option to not 'auto reject' code with formatting differences as mentioned in other answers. With the 'I have reviewed and approve automatic formatting corrections; commit' option, it still keeps personal accountability for each developers work and doesn't mess with VCS.

0

I won't do it on the repository but I will do it on save if the tool supports it. Eclipse is one and in addition I would do the code cleanup including sorting.

The nice part is that it is part of the project so every developer will get it for their project.

As an added bonus merges would be significantly simplified since things will not be jumping around.

Code reviews will prevent any errant ones.

Another place I would do it is have it part of the build. In my case I have it such that maven builds will reformat the XML and clean up pom files and reformat the code.

That way when developers are ready to push it will all be cleaned up for their pull request.

protected by gnat Apr 13 '17 at 8:55

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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