Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
35  
Why don't we want people feeling bad/wrong about doing bad/wrong things again? –  Telastyn Nov 12 '12 at 17:10
4  
Isn't this easy if you have regular code reviews? –  user16764 Nov 12 '12 at 17:10
7  
When egos are involved, it's always best to accentuate positives and give the benefit of the doubt. You want "Hey, I think we can do this better now that we're not under a time-crunch. Let's try this!" instead of "who the hell wrote this WTF-filled code? I'm going to nuke it from orbit". –  Steven Burnap Nov 12 '12 at 17:19
7  
2  
@Telastyn Oh, we want, but there is a fine line when people start to create even more problems –  dukeofgaming Nov 12 '12 at 18:33

5 Answers 5

up vote 37 down vote accepted

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

share|improve this answer
    
Could you elaborate on the "do not sacrifice consistency" and "do tackle the matters quickly" points, looks like you may have a story behind those –  dukeofgaming Nov 13 '12 at 4:15
    
@dukeofgaming: consistency I meant that if your team uses a particular code style, stick to it, whether you like it or not. If you want to change it, address the issue in meetings. If the file you are currently editing as a different style than the rest of the codebase, re-style it if there's no risk of adding changes and it's a small file or it's agreed with your team. If not, then leave it alone, and keep your changes consistent with this file's style. –  haylem Nov 13 '12 at 10:55
    
@dukeofgaming: about tackling it quickly, I just want to stress that I think it's a good idea to want to resolve these issues and that I think it needs to be done quickly. The more you wait, the more it creeps through the codebase, and the harder it gets to solve. Your technical debt increases, and the impact will be greater on the long run. Maintainance is a regular task. Like for a car, if you wait until you're forced to fix it, it's a lot more costly. Or like cleaning your house, the more you wait, the more things risk getting damaged by dirst/dust and the harder you'll need to scrub. –  haylem Nov 13 '12 at 10:58
    
+1 for "politically correct" vs "diplomatic" –  Andrew Finnell Nov 13 '12 at 14:05
    
Would love to hear how this all works out for you. Good luck. –  Bill Leeper Nov 13 '12 at 16:48

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.

share|improve this answer
    
Yes. When you have an environment like this, it makes it easy for people to fix code and spread good methodologies without having to directly confront anyone in front of the group. –  Steven Burnap Nov 12 '12 at 17:16
1  
Rings a bell, are you talking about "collective code-ownership"? extremeprogramming.org/rules/collective.html –  dukeofgaming Nov 12 '12 at 18:40
    
@dukeofgaming essentially, yes. –  Oleksi Nov 12 '12 at 18:42
1  
About pushing the cultural change: make sure you inform people that you would be very happy if anyone would take a moment to improve your code and to drop you a line about it. You cannot expect to get to modify "somebody else's code" yourself if you don't embrace them modifying "your code". Then, introduce code reviews: have every line of code seen by at least one other pair of eyes besides the programmer, preferably with the programmer introducing the code to the reviewer in person. People will do better when they know some real person will actually really see what they've done. –  ZeroOne Nov 12 '12 at 23:34
3  
Superstar developers might present a problem if you have them (or if you realize you're one of them). Also, related, I read another blog post from somewhere but cannot find it anymore, where the author adopted the attitude that if he got hit by a truck, any other programmer should be able to pick up from where he left within a week at most. That led him to write clearer and better code, so when he was trying to make himself replaceable, he actually became even more valuable for the company. –  ZeroOne Nov 12 '12 at 23:39

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.

share|improve this answer

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.

share|improve this answer

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!

share|improve this answer
    
+1: nice anwser, and good steps to approach this progressively. –  haylem Nov 13 '12 at 1:33
    
I'm not learning to refactor or collaborate, however, there seems to be no willingness to improve things and have gotten the "if it ain't broke don't fix it" attitude. If I try to "break" past this attitude I have to be careful about making people feel criticized. –  dukeofgaming Nov 13 '12 at 4:48
    
I used to say "Just because it works doesn't mean it isn't broken." quite a lot, maybe a little too often if I'm honest. I think refactoring is such a great tool because, while it is marvelous in a supportive environment, it can still work even in a moderately hostile one. I would challenge you to reconsider your statement that '[you are] not learning to refactor'. I'm sure you have the mechanics down just fine, but refactoring is not about moving code around; it's about moving it in a sequence of inoffensive steps. If you're offending people, you could still learn more about it. –  Jason Nov 15 '12 at 19:23

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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