Etiquette for refactoring other people's sourcecode?

Question

Our team of software developers consists of a bunch of experienced programmers with a variety of programming styles and preferences. We do not have standards for everything, just the bare necessities to prevent total chaos.

Recently, I bumped into some refactoring done by a colleague. My code looked somewhat like this:

public Person CreateNewPerson(string firstName, string lastName) {
    var person = new Person() {
        FirstName = firstName,
        LastName = lastName
    };
    return person;
}

Which was refactored to this:

public Person CreateNewPerson (string firstName, string lastName) {
    Person person = new Person ();
           person.FirstName = firstName;
           person.LastName = lastName;
    return person;
    }

Just because my colleague needed to update some other method in one of the classes I wrote, he also "refactored" the method above. For the record, he's one of those developers that despises syntactic sugar and uses a different bracket placement/identation scheme than the rest of us.

My question is: What is the (C#) programmer's etiquette for refactoring other people's sourcecode (both semantic and syntactic)?

Answer 1

An etiquette should always be done on the level of the team. So talk with your colleague about this and then talk with the complete team to define a rule.

Common rules may contain not to change the code, if it is only for beatifying and disputed coding styles. If someone has to maintain your class in the future, then it is usual, that he can change anything.

Define some base-rules, some anti-patterns (that always can be refactored by your coworkers) and so on.

Such rules don't have to be very strict, so the placement of braces or similar things don't need to be defined. But in that case, nobody should beatify code, someone else maintains. If you get into conflict about one thing, talk about it in the complete team, to create a new rule for this case.

Answer 2

IMHO this doesn't make the code any cleaner, just imposes the other guy's coding style on it. You should discuss it with your colleague(s) whether this type of "refactoring" is really necessary (and whether there are really no better ways for your colleague to spend his/her time :-)

Answer 3

Without coding-style guidelines/rules, he might not even be aware that the change is causing annoyance.

That said, the style is fairly non-standard and on a personal level, I'd be annoyed with a "refactor" that does not change the meaning of the code but rather serves only to stamp his coding style in your face. I'm not sure it even qualifies as a refactor. Pretty selfish.

Answer 4

Did he produce a majority of the code for the project in question? If that were the case, I could understand what he did. When entering into a project that's already well underway, I try to match the formatting that's already in use throughout the code.

That may not apply to your situation, of course. If whatever project has received generally equal contributions from all of you, maybe take Mnementh's advice and bring the issue to a head.

Answer 5

I believe the most important thing is to be able to disagree and commit. We all have our preferences, but changing non-important things back and forth waste everybody's time.

Answer 6

The most important aspect of this is consistency. Your team should decide on whether to use type inference and object initializers and write down some coding guidelines.

Answer 7

I would simply talk with your fellow developer about what you want to refactor and why. Only refactor the code if you agreed on something.

This will result in discussions where most probably both of you will learn something from each other.

Answer 8

I believe in collective code ownership i.e. that code belongs to the project, not to an individual engineer. So I have no problem with someone refactoring something I wrote as long as it complies with the project standards. If the project doesn't have coding standards, then the team should define some.

Answer 9

I would worry less about politeness and more about the economics. Every code change induces a huge number of costs:

• code changes must be tested by QA
• code changes must have test suites written for them by development
• code changes that affect user experience may need to be documented
• code changes can introduce bugs; those are obviously enormous costs
• and so on.

I would not dream of making a minor "aesthetics-only" change to any production-quality code, whether it was "mine" or not. The benefits of the change do not come anywhere even close to justifying the cost.

You might consider reminding your colleague that you're in the coding business not to produce beautiful code that you all find aesthetically pleasing, but rather to produce profit for your company in a weak economy. You're not artists, you're engineers, so act like engineers; all changes should be justified by a business purpose.

Answer 10

Ruthlessly adhere to the standard. If there is no standard then that is your problem, not that other people can and do change your code.

Also, before getting upset about code changes, you need to determine intent. Was it malicious, or an innocent change?

Answer 11

If I'm modifying a file that is owned by a colleague, I try to keep the changes consistent with their style. This way, even though half of our files prefix "m_" for fields and half prefix "_" (among other minor things), at least a single file/class is self-consistent.

Answer 12

I think in this specific example (C# that is), you should simply follow the guidelines provided by Microsoft. Consistency with the code standards of the .NET Framework makes for easy to read class and code structure.

The other thing is Visual Studio auto-corrects to various formatting rules as you type which would help. For example, when closing a bracket or ending a statement.

I personally think that cosmetic refactor looks ugly and decreases readability, whereas your code adhered to the .NET conventions.