13

While developing in a group atmosphere and adding or modifying features in some code base. Is it considered offensive or impolite to reformat the previous developers code to bring it up to current coding standards? I understand that standards have changed, and will probably continue to change, but would any of you get offended if someone came through and changed your code formatting?

To be clear, I'm not talking about changing any logic, just messing with tabs and spaces and such.

EDIT: I don't only do this for the sake of coding standards, it helps me read through their code and bring it up to date so I can fully understand the logic that has been implemented before I start modifying critical applications.

wfoster
  • 483
  • 6
    Enable automatic "format at save" for everybody. Everbody use the same agreed upon settings. After a while all code is normalized. –  Jul 08 '11 at 18:03
  • 1
    There can be a point where this goes to far. I had a coworker that reformatted everything added linebreaks not that were needed or even relevant as far as I was concerned. Personally unless it is unreadable or the code has become my primary responsibility I leave the formatting alone unless I am making other changes. – SoylentGray Jul 08 '11 at 18:52
  • 1
    If you are coding in c#, then stick to StyleCop. If in other languages, then try to pick a good, unbiased tool. – Job Jul 08 '11 at 22:20
  • 5
    Is this "I'm changing formatting... because I think it should look different".. or is this "I'm changing the formatting... too match the standards"... hugely different questions – WernerCD Jul 08 '11 at 22:22
  • @Thorbjorn but that kills history. – alternative Jul 08 '11 at 22:22
  • @mathepic, yes, but you will need to at one point anyway if they have incorrect formatting. –  Jul 08 '11 at 22:32
  • 1
    @Thorbjorn I wouldn't consider a branch that fixes formatting in every file, 1 file per commit, killing history. However, fixing it during the same commit is just bad. (I guess they could use something like git add to selectively commit parts, but my guess is that most people use the equivalent of svn commit or git commit -a) – alternative Jul 08 '11 at 22:43
  • @WernerCD I'm changing formatting to match the standards – wfoster Jul 08 '11 at 23:43
  • 1
    @mathepic, in that case then do a global reformat. Depends on your preferences. –  Jul 09 '11 at 06:23

10 Answers10

19

I think this is OK as long as the standards are agreed upon. One note of caution however; be aware if there is the potential the file is being modified by others at the same time. If you make their merge harder just because you were changing formatting you won't be very popular.

Jeremy Mullin
  • 306
  • 10
    The first sentence here is important. Make sure that you are actually following the agreed-upon standards, and not just making changes because you like them. – Thomas Owens Jul 08 '11 at 17:00
5

Yes, the code should belong to the project. Bringing the code up to standard will help reduce the project's technical deficit. If you are modifying it, you are currently responsible for it. For older code, the original developer may no longer be on the project or has new duties.

When you do this kind of change it is good to run the verification tests after your reformatting. If they pass, check the code in before doing your feature changes.

EDIT: In the context of this question, reformatting to standard is appropriate. In the absence of standards, I would recommend advocating for standards and not reformatting until there are standards for the format. Reformatting to personal taste/standards should not be done with code belonging to the project.

BillThor
  • 6,270
  • 2
    +1 for "check the code in before doing your feature changes" – bdoughan Jul 08 '11 at 17:01
  • 1
    +1 again for "check in formatting changes before doing your feature changes" and "it is good to run the verification tests after your reformatting". Ideally, we should run verification tests before each checkin. – leed25d Jul 08 '11 at 17:45
  • Actually, whether you reformat before or after the changes does not really matter. What matters is that aesthetical patches should be kept separate from functional patches --> if an aesthetical patch changed the functionality, it was not intended and can be deemed a bug; it makes functional patches easier to review (because smaller). – Matthieu M. Jul 08 '11 at 18:24
  • @Matthiew M: True, but in most cases they will be done first to improve maintainability before maintenance. Few developers have time to do so after the fact. Also, if the code needs to be upgraded to pass automated check-in tests, it must be reformatted first to maintain separation of aesthetic and functional patches. – BillThor Jul 10 '11 at 06:07
3

I believe it's always a good practice to refactor the code when you are modifying/adding to a particular file. That includes updating the style of the code to reflect proper naming conventions for variables/methods and coding styles.

Wayne Molina
  • 15,684
2

I do this all the time. Old code should be held to the same standards of new code and if you don't fix it up while you are working on it no one will. I think this counts under the Boy Scout Rule.

RKitty
  • 280
2

I think this is good practice and a necessary part of code maintenance.

I would recommend checking in the formatting changes in one commit to the version control system and the functional changes in a separate commit to help both yourself and others understand what took place.

semaj
  • 131
  • 1
    +1 for separate commits. Trying to figure out what code changes were made in a commit when the code has been reformatted at the same time is a PITA. Your diff tools are useless if every line in the file has changed. – Dave Kirby Jul 13 '11 at 22:27
2

I would not have any problem with it and would probably appreciate it... as long as the changes are not "religous". Please do not go through all of my classes and move the curly braces to the first line of the method. If the formatting is a legitimate "different strokes for different folks" type thing, then it is a bit annoying when someone goes in and imposes a format on the code you most frequently edit. However, if you become the primary editor of that particular module, then make whatever formatting changes you see fit.

Morgan Herlocker
  • 12,742
1

Yes. Please "fix" the code as you see fit. Just like the Pragmatic Programmers say in their book The Pragmatic Programmer, no broken windows. If the code is not up to par, I consider it a broken window.

mpenrow
  • 1,653
1

There are various repositories that will automatically do reformatting on check-in as well as small things like change CR/LF pairing on get depending on the platform getting the source.

There is a huge drawback to doing your own reformats in that your check in deltas will be obfuscated by tons of reformatting and if there's a regression problem it becomes more difficult to find the offending code blocks.

You might suggest to your lead that since the code base is old it should be brought in from the cold and reformatted to current standards all in one go, leading to a bright new future for code everywhere.

Patrick Hughes
  • 1,371
1

Since you are talking a purely "formatting" issue (meaning we aren't fixing bugs but making it look pretty to your own standard), I think it depends if the original person is still maintaining the code or not.

If the originator is still working on the project - it is rude. What might "look" right to you isn't what will "look" right to them and to modifying the code for the sake of formatting isn't polite. It can also waste lots of time.

I was working on a project once with a VERY possessive developer. Over the years, I have developed a very methodical way of formatting my code that I feel is easy to read, less prone to implicit mistakes, and self-documenting. This guy, on the other hand, favored using all implicit features with long lines which spread out 300 characters wide so you had to have a 30" monitor to read it because he believed line count was more important than readability. He spent half a day blowing through my code changing it to his "preferred standard"...while I was still developing in parallel! I came in the next morning to find two days worth of work formatted to his mess. It was rude and a waste of time.

Now if the developer is gone and you have a "better style" go for it.

Jordan Parmer
  • 203
  • 1
  • 7
0

Always autoformat the code if your IDE can do it.

  • Prevents manual formatting changes from cluttering your version history in the long run
  • Formatter profile has to be agreed upon amongst all the developers (pick the default?-)
  • Make formatting code and organizing imports a habit when saving a file

For example in eclipse, you can first run the formatter and organize imports for the whole code base. Then remember to ctrl+alt+f before you save.

jkj
  • 121