27

I almost always format my code before I commit to make sure it's done properly. Most of my team don't really care and don't always format their code properly (minor things that don't affect the code but affect readability when trying to maintain it).

I recently installed the VS power tools that has an option "Format on save", and made a change to a file that wasn't formatted prior. The development VP just came to me and reprimanded me for formatting since it shows up in the merging tool as having almost the entire file changed, instead of just a line or two (so he can't see exactly what I modified easily), and told me to disable the format on save in the future. While I understand that concern, I find it difficult sometimes to sort through the code that is unformatted, and IMO it should be formatted properly all the time anyways. Note that I'm not just reformatting things on a whim., but as I write code I'll either use the power tool or hit the key command to format the text to make it easier to read, and in SVN this shows up as a modification.

So I ask, is always formatting the code actually a bad thing? Are his concerns more valid than making sure the code is readable?

Improve this question
4
  • 8
    he's right, so why not get all the team to use the format-on-save tool too, then you'll all get nicely formatted code that is easy to read, and easy to view commit diffs.
    – gbjbaanb
    Commented Jun 13, 2011 at 15:25
  • 13
    Most good file compare tools have a filter for "unimportant differences" or "ignore whitespace". Some, like Beyond Compare, ship with prebuilt language-specific filters. Use it to your advantage if you have it.
    – Michael K
    Commented Jun 13, 2011 at 15:35
  • 7
    The formatting of the code is as important as the changes that were made. Readability has to be one of the highest priorities when you're on a team. Your VP should know that and be concerned about it.
    – Edgar Gonzalez
    Commented Jun 13, 2011 at 15:42
  • @Edgar: +1. The VP is being too picky. Readability first... and a whitespace ignore option means that this is no big deal. And it also means there is a bigger problem because the rest of the team don't care. The VP should be more concerned about that.
    – quickly_now
    Commented Jun 14, 2011 at 6:26

5 Answers 5

Reset to default
45

First off, your team needs to pick a formatting convention and stick with it. You need to come to an agreement and have everyone stick to it so you don't have people fighting over what things should look like. This should not just be something you do on your own.

As for your real question. Formatting code is not a bad thing. What is bad is making major formatting changes in the same commit as code changes. When your team comes to consensus about how things should be formatted, make one pass thru the code and format everything. Check that in by itself. The commit message will make it clear that the changes are just white space and not functional. Then when you need to make functional changes, they are in a different commit so they can be clearly seen.

Improve this answer
7
  • it still doesn't help if you want to compare changes from several revisions ago, but its better than code change + format changes in 1 go. Of course, this answer also applies to refactoring.
    – gbjbaanb
    Commented Jun 13, 2011 at 15:24
  • 1
    +1: In addition to this, it's good to use something like Stylecop or some other tool that autoformats and enforces style. Then, synchronize the settings between all of the team members so that the formatting is consistent across everyone and you don't necessarily have to remember what the "right" format rule is.
    – Ryan Hayes
    Commented Jun 13, 2011 at 15:30
  • 3
    If the OP was reprimanded for trying to format one document, something tells me he wouldn't be able to suggest using StyleCop.
    – Wayne Molina
    Commented Jun 13, 2011 at 15:33
  • 3
    @gbjbaanb: Yes. This is why it is best to make these kind of decisions at the start. The project I am on now has the Eclipse formatter settings checked into the repository so we know everyone has the same settings.
    – unholysampler
    Commented Jun 13, 2011 at 15:51
  • 1
    @quickly_now: This is why we have managers with veto rights. If people can't agree, they can make a decision.
    – unholysampler
    Commented Jun 14, 2011 at 12:13
32

No, formatting code is very important. However, commits should be done in two groups:

  1. Cosmetic changes - anything that makes the code more readable.
  2. The other changes - everything else that affects the code.

Use the commit message to signify that only cosmetics have been changed. These can be easily skipped over when searching for more substantial modifications.

Improve this answer
2
  • 3
    Additionally, it is also a good practice to decide on a certain formatting convention between your team. Don't just format code from other people without discussing this first.
    – Steven Jeuris
    Commented Jun 13, 2011 at 15:07
  • 1
    Yeah.. But you know, sometimes it's so tempting to format that damn mess "while you're at it". Also, trying to separate cosmetic changes from functional changes can be a pain if you use VS and it formats something automatically. Oh, and nobody will say that you are doing some stupid formatting while you have Very Important Tasks to do by looking at commit history
    – Dyppl
    Commented Jun 14, 2011 at 6:20
10

You both have a point, but you can both get what you want. Format the code first, check in that change only. Next, make your functional changes and check that in as a second step.

Improve this answer
2
  • 3
    I think this is the best solution for your current situation, but you should talk about it with your team. However, you do have a bigger problem, which is the lack of a coding standard.
    – Thomas Owens ♦
    Commented Jun 13, 2011 at 15:06
  • 2
    Agreed. I wonder if the OP's environment is one of those cowboy places where standards are eschewed to "crank things out quick".
    – Wayne Molina
    Commented Jun 13, 2011 at 15:23
4

I am a formatting nit-picker too, so here a few tips:

  • Required first step: get the team to agree on some basic formatting standard, such as tabs vs. spaces, brace positions, comment styles, etc. Now your formatting changes won't be a complete surprise to everyone, and you won't step on any toes.

  • Clean up the formatting only around the code you change. If you make changes to just one function, then clean up that function. At least over time you'll have better-looking code.

  • Do major formatting overhauls as a separate commit, with no other code changes. You should only do these when you're less likely to want to compare code after the change to before the change, since comparing across a diff like that can be annoying. I usually do cleanups as the first thing before major development on that code.

  • Get a good diff tool that can do language-dependent marking of significant changes and non-significant changes. My favorite diff too Beyond Compare marks actual code changes in one color and whitespace/comment only differences in another.

edit for one more tip:

  • It varies form language to language, but for most truly cosmetic changes to the code, you should be able to compare compiled binaries before and after a major cleanup to be absolutely sure you didn't muck it up.
Improve this answer
1
  • As long as you don't include VC tags in the binary (or build information).
    – Vatine
    Commented Jun 14, 2011 at 9:41
3

You should not be re-formatting and committing changes to other people's code unless:

  • you are the manager attempting to establish team coding standards
  • your manager has asked you to clean up the code to adhere to team coding standards
  • your are cleaning up code from a developer no longer on your team to adhere to team coding standards.

You'll notice in all cases I refer to team coding standards. I am a strong believer in reasonable, agreed-upon coding standards for the team. If you have them, then the original developer should go back and clean up his or her code to adhere to the team standards, you should not do that behind their back. If you do not have standards (and you should), then you should not be modifying another team member's code to adhere to your philosophies, especially behind their back. Remember, you are part of a team and while coding standards are important, so are the trust and respect between team members.

Improve this answer
5
  • "Behind their back": this goes back to the psychological issues of code ownership (or development turf war).
    – rwong
    Commented Jun 13, 2011 at 19:55
  • 2
    "Other people's code" is an interesting way of saying it. I work on my company's product, compiled from the code my company owns, that my team members and I work on. It isn't behind their back in any way to fix it to the standards while working on it. However, I do agree that the ideal solution is to make the original developer clean it up to the standard.
    – Caleb Huitt - cjhuitt
    Commented Jun 14, 2011 at 2:13
  • @Caleb: Gets hard if they just flat out refuse.
    – quickly_now
    Commented Jun 14, 2011 at 6:29
  • By "other people's code" I don't mean ownership, I mean something they wrote and believe they are still responsible for supporting. In the absence of coding standards, if I implement a class with 1,000 lines of code and you make changes to 2 lines to correct some behavior and reformat the whole file, I'm going to be very surprised when I open the file. As members of a team we shouldn't do that to each other. If you check that file in with a full reformatting and don't even give me a heads up, that is not very team friendly.
    – cdkMoose
    Commented Jun 14, 2011 at 14:01
  • In OPs original discussion, I read that to be an environment without coding standards (or not well enforced), that is why I answered as such. In that environment, one developer should not be imposing his standards on others.
    – cdkMoose
    Commented Jun 14, 2011 at 14:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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