What is the politically correct way of refactoring other's code? [duplicate]

Question

I'm currently working in a geographically distributed team in a big company. Everybody is just focused on today's tasks and getting things done, however this means sometimes things have to be done the quick way, and that causes problems... you know, same old, same old.

I'm bumping into code with several smells such as: big functions pointless utility functions/methods (essentially just to save writing a word), overcomplicated algorithms, extremely big files that should be broken down into different files/classes (1,500+ lines), etc.

What would be the best way of improving code without making other developers feel bad/wrong about any proposed improvements?

Answer 1

Review and Annotate the Code

• DO review, preferably using a code review tool. ^{(Otherwise e-mails, but... meh).}

• DO justify your remarks and comments.

• DO NOT bitch about it.

Fix the Code

• DO fix it:
  • if they don't have time to do it themselves,
  • after having discussed it with the author, or formally told some members about your desire to go about fixing some things.
• DO test for things you fix ^{(preferably with unit and integration tests)}
• DO justify yourself.

• DO prioritize:

  • At first Start with a few minor things to get into the habit and show that you are careful;
  • But then DO absolutely fix the big things first rather than hundreds of small things.

• DO NOT fix unclear code paths that you do not fully comprehend.

• DO NOT fix untested code paths that you may break unknowingly.
• DO NOT fix code paths blindly. Simple fixes are simple, but mistyping a symbol or having a brainfart and inverting a boolean condition happens all the time. Test your stuff.
• DO NOT bitch about it, again.
• DO NOT annoy others and preach self-righteously.
• DO NOT fix irrelevant or inconsequential problems ^{(at first)}. If it doesn't matter too much, you're likely to have better things to do.
• DO NOT sacrifice consistency.

In essence, DO NOT become this annoying guy we all know who just changes whitespaces or brace styles based on a personal preference but without regard for the consistency with the rest of the codebase, and who even goes so far as to break builds while fixing minor code violations by introducing simple but deeply hidden bugs.

Communicate

• DO contact the authors.
• DO discuss your changes with others (not necessarily authors).
• DO request review for your own changes from others; this:
  • shows humility,
  • invites others to jump onto the bandwagon,
  • shows that this is a fluid process.
• DO establish rules and conventions with your co-workers to avoid this issue expanding and crippling the codebase over time.
• DO tackle the matter quickly.

Ask

DO ask. Rather than saying "I'm fixing this because X", ask directly "why did you do it this way, and how do you think we can improve it?" If they come up with something better, then start with that and work together on something even better. If they don't, then make a suggestion. If they come up with something better, or don't see why it was wrong, then explain and ask the second question again.

Make it a Team Effort

Your team is more likely to jump onto the bandwagon if they actually take part to this. If this is a large project with a somewhat aging codebase, it's likely that they also dislike the state of rot the code is in, but maybe never bothered to do something about it.

If you have the power (or support of the ones who do), it can be a good idea to organize small sprints dedicated to enhancing code quality. Setting up code analyzers to detect code smells and monitoring their steady decrease while you refactor is also fairly motivational for a team.

Of course, that might be a bit harder for remote teams, as it's a bit more difficult to convey enthusiasm about such things without actually working together.

---

It pretty much all comes down to that.

Semantics

Also, don't confuse "politically correct" with "diplomatic":

• The former is about beating around the bush to not offend people while saying something most likely to be negative.
• The latter is about tackling issues while not offending people because there might be some emotional context.

They're very different things.

Related Questions

• How Can I Tactfully Suggest Improvements to Others Badly Designed Code?

Answer 2

Part of the solution is changing the way people see the code. Any piece of code belongs to the whole team, not just the person who wrote it. If you start to think of the code base this way, you won't have to think about who wrote the original code, because it belongs to everyone.

Creating this sort of environment goes a long way to making feel people comfortable changing any part of the code.

Answer 3

Talk First!

There is no point in making changes that someone-else is just going to get upset about and undo. It's a waste of your political capital and your time. If you are the boss and can enforce new standards, then talk to your team about the new standards and implement them. If not, talk your co-workers and maybe your boss before making changes.

If you talk to each team member one-on-one, you can decide how much political capital you are willing to spend before lighting yourself on fire in front of the whole team. You may still want to have a team meeting as public acknowledgement of the private consensus you achieved through one-on-one meetings, and as a chance for people to comment on the new coding policies before implementing them.

With a group consensus, everyone is tasked with making the code better. It's not just you against the world. If agreement is unattainable, then you won't waste your time fighting the inevitable.

Answer 4

A codebase in a distributed team environment is a lot like Wikipedia; as they say, "If you don't want your posts to be ruthlessly edited by the rest of the world, don't submit it".

If there's a better way for a particular piece of code to be written, and you need it written in that way, then write it that way. If someone complains, listen to the complaint, but if it's primarily along the lines of "that's MY code", explain that he is a member of a team that is working on the codebase as a whole, and nobody really gets to claim ownership. Also point out that, legally speaking due to the paperwork he signed when he was hired, it's not "his" code, it's the company's, and you also work for that company.

Now, there can be legitimate gripes about changing the "interface" of code (the way code objects appear and interact with other code) without first consulting the team. If someone has a working copy they're making their own set of large changes to, heavily based on the older interface, and you come in and change that under their nose, they have a lot of refactoring to do. There's a simple solution to that; send a team e-mail saying "hey I need to change this code, is anyone working on something locally that would break the build if I changed it?". Wait a few minutes, and if anyone raises an objection, work it out in a more personal channel of communication. If not, refactor away.

Answer 5

It's tough to describe what to do in a situation like this. Most people who are good at refactoring already know the answer, and don't have to ask the question. I'm guessing you are still learning, and if that's the case then you're in for a longer, character-building journey.

Start small. Boil the frog, not the ocean.

You should not be refactoring just to refactor. You will certainly learn a lot faster by randomnly refactoring things, but your environment is not nurturing in that way. Instead you're 'changing my code for no good reason', and it'll only serve to get you branded as a divisive person - a jerk. I would wager you're already learning about this, and that's why you asked.

Refactor code that is actively in your way. "I changed this because I had to fix a bug/add a feature." Once that is going okay for you, once they understand that this is how you work, switch to refactoring code that's arguably in the way. "I changed this because it was easier to fix a bug/add a feature." This is when you start getting into the Campsite Rule, and explaining it to others, and why you think this is the way THEY should work (pretty please with sugar on top). Look for collaborators, people who'll let you fiddle with their code to make it shinier.

Just those three tasks will keep you busy for a good long while.

Some day, in the far flung future, you may get to explain the Broken Windows phenomenon, but I recommend you don't get your hopes up. Don't assume this team will ever get there, for your own sanity. Instead you should just plan to save that conversation for your next job, or even the one after that. Be an informed consumer during the interview process. It'll help you avoid this scenario, and they'll see you're serious about your craft.

Good luck!