views:

5899

answers:

38

I've been working with a small group of people on a coding project for fun. It's an organized and fairly cohesive group. The people I work with all have various skill sets related to programming, but some of them use older or outright wrong methods, such as excessive global variables, poor naming conventions, and other things. While things work, the implementation is poor. What's a good way to politely ask or introduce them to use better methodology, without it coming across as questioning (or insulting) their experience and/or education?

EDIT: Wow, a lot of great answers. Thanks for the input so far. I'm not even sure if there is a truly correct answer to this.

+7  A: 

Suggest a better alternative in a non-confrontational way.

"Hey, I think this way will work too. What do you guys think?" [Gesture to obviously better code on your screen]

JosephStyons
A: 

Politely and firmly...

johnstok
+37  A: 

Introduce the idea of a code standard. The most important thing about a code standard is that it proposes the idea of consistency in the code base (ideally, all of the code should look like it was written by one person in one sitting) which will lead to more understandable and maintainable code.

Scott Dorman
Indeed. Something we have in our code reviews is that if the code isn't consistent with the standard, the reviewer stops reading and doesn't come back to the code until it complies.
Graham Lee
One of the better and simpler ways to enforce it.
Scott Dorman
I think it depends on the nature of the problems. Even with a coding standard there are still multiple ways of doing things, and maybe some of the flaws are separated from enforced standards.
EnderMB
Not only is it a good idea in general, but this avoids the problem of looking like you're singling any one out for writing particularly cruddy code.
Bob Somers
@Scott Durman: "all of the code should look like it was written by one person in one sitting"... LOL!!! ;-)
Galwegian
@Galwegian: First, if you're going to use my full name at least spell it correctly. :) Why is that statement funny to you? As I said, it's the ideal of a code standard. I never said it was completely achievable, but it gives a clear, well-defined goal to work towrds.
Scott Dorman
+7  A: 

By example. Show them the right way.

Take it slow. Don't thrash them for every little mistake right off the bat, just start with things that really matter.

Bill the Lizard
+3  A: 

If you have even a loose standard of coding, being able to point to that, or indicating that you can't follow the code because it's not the correct format may be worthwhile.

If you don't have a coding format, now would be a good time to get one in place. Something like the answers to this question may be helpful: http://stackoverflow.com/questions/4121/team-coding-styles

warren
+1  A: 

Probably a bit late after the effect, but that's where an agreed coding standard is a good thing.

KiwiBastard
+8  A: 

The code standard idea is a good one.

But consider not saying anything, especially since it is for fun, with, presumably, people you are friends with. It's just code...

Jeff Kotula
I like this point, as for this project, 'if it works, it works' will suffice, but I get the feeling that finding an appropriate way to deal with the issue will help a lot more than the initial instinct to point and say 'This is wrong'.
MadMAxJr
"It's just code"? It's just the product of your effort, your professional face, your contribution to the company or to humanity at large. Who cares if it's good or bad?I betcha your coworkers are furiously reading this topic, trying to figure out how to tell you your "just code" is garbage.
Arkadiy
It's just code? Helloooo maintenance nightmare.
MetalMikester
+8  A: 

They may think your style stinks too. Get the team together to discuss a consistent set of coding style guidelines. Agree to something. Whether that fits your style isn't the issue, settling on any style as long as it's consistent is what matters.

SumoRunner
Oh, this is certainly true! "Why are you using a class for holding a string while you can use low-level C routines to do string manipulations (including memory allocation/deallocation and manually adding a 0-terminator)?" And indeed, those programmers have often used their programming style to successfully complete large projects, strange but true.
Dimitri C.
+9  A: 

First, I'd be careful not to judge too quickly. It's easy to dismiss some code as bad, when there might be good reasons why it's so (eg: working with legacy code with weird conventions). But let's assume for the moment that they're really bad.

You could suggest establishing a coding standard, based on the team's input. But you really need to take their opinions into account then, not just impose your vision of what good code should be.

Another option is to bring technical books into the office (Code Complete, Effective C++, the Pragmatic Programmer...) and offer to lend it to others ("Hey, I'm finished with this, anyone would like to borrow it?")

Kena
+19  A: 

You have to explain why your way is better.

Explain why a function is better than cutting & pasting.

Explain why an array is better than $foo1, $foo2, $foo3.

Explain why global variables are dangerous, and that local variables will make life easier.

Simply whipping out a coding standard and saying "do this" is worthless because it doesn't explain to the programmer why it's a good thing.

Andy Lester
I think this applies to just about all possible commandments (not only programming related ones).
Dimitri C.
+8  A: 

If possible, make sure they understand that you're critizising their code, not them personally.

JesperE
+4  A: 

I always go with the line 'This is what I would do'. I don't try and lecture them and tell them their code is rubbish but just give an alternative viewpoint that can hopefully show them something that is obviously a bit neater.

Craig
+114  A: 

Introduce questions to make them realise that what they are doing is wrong. For example, ask these sort of questions:

Why did you decide to make that a global variable?

Why did you give it that name?

That's interesting. I usually do mine this way because [Insert reason why you are better]

Does that way work? I usually [Insert how you would make them look silly]

I think the ideal way of going about this is subtly asking them why they code a certain way. You may find that they believe that there are benefits to other methods. Unless I knew the reason for their coding style was due to misinformation I would never judge my way as better without good reason. The best way to go about this is to just ask them why they chose that way; be sure to sound interested in their reasoning, because that is what you need to attack, not their ability.

A coding standard will definitely help, but if it were the answer to every software project then we'd all be sipping cocktails on our private islands in paradise. In reality, we're all prone to problems and software projects still have a low success rate. I think the problem would mostly stem from individual ability rather than a problem with convention, which is why I'd suggest working through the problems as a group when a problem rears its ugly head.

Most importantly, do NOT immediately assume that your way is better. In reality, it probably is, but we're dealing with another persons opinion and to them there is only one solution. Never say that your way is the better way of doing it unless you want them to see you as a smug loser.

EnderMB
This is a good technique, and probably the most appropriate one in a professional environment. If your colleague answers questions like this flippantly instead of actually considering them, or has lame answers, odds are good that they'd ignore a code standard or other "authority" as well.
Greg D
I agree, but my gripe is mostly to deal with sensitive programmers. If you directly tell someone that their code is wrong then, naturally, they won't be very happy. It's silly, but working through and learning the problems with them will probably deliver the best outcome for everyone.
EnderMB
I think questions like "Why did you give it that name?" are close, but not quite right. That immediately gets me thinking defensively about my decisions. "That's interesting. I usually do mine this way because [Insert reason why you are better]" is much better because it gets me thinking about ways **other** than what I've already decided on. With the latter tone, I'm much more likely to see the light.
Bill the Lizard
In a way, they should be thinking defensively. If I merely show them my way of doing things they might just decide to stick with the method they know. A compromise would be good, saying how you would do it and then subtly adding why your method is faster/better/etc.
EnderMB
And what if there answer consistently is something like: "because it is to much work to change that" (often it indeed is because of the huge amount of existing code) or "because we've always did it this way and it worked fine"?
Dimitri C.
+2  A: 

Have the person(s) in question prepare a presentation to the rest of the group on the code for a representative module they have written, and let the Q&A take care of it (trust me, it will, and if it's a good group, it shouldn't even get ugly).

Jim
A: 

Privately inquire about some of the "bad" code segments with an eye toward the possibility that it is actually reasonable code, (no matter how predisposed you may be), or that there are perhaps extenuating circumstances. If you are still convinced that the code is just plain bad -- and that the source actually is this person -- just go away. One of several things may happen: 1) the person notices and takes some corrective action, 2) the person does nothing (is oblivious, or doesn't care as much as you do).

If #2 happens, or #1 does not result in sufficient improvement from your point of view, AND it is hurting the project, and/or impinging on you enough, then it may be time to start a campaign to establish/enforce standards within the team. That requires management buy-in, but is most effective when instigated from grass roots.

Good luck with that. I feel your pain brother.

Chris Noe
+3  A: 

Start a wiki on your network using some wiki software.

Start a category on your site called "best practices" or "coding standards" or something.

Point everyone to it. Allow for feedback.

When you do releases of the software, have the person whose job it is to put code into the build push back on developers, pointing them to the Wiki pages on it.

I've done this in my organization and it took several months for people to really get into the hang of using the Wiki but now it's this indispensable resource.

Schnapple
+2  A: 

A lot of the answers here relate to code formatting which these days is not particularly relevant, as most IDEs will reformat your code in the style you choose. What really matters is how the code works, and the poster is right to look at global variables, copy & paste code, and my pet peeve, naming conventions. There is such a thing as bad code and it has little to do with format.

The good part is that most of it is bad for a very good reason, and these reasons are generally quantifiable and explainable. So, in a non-confrontational way, explain the reasons. In many cases, you can even give the writer scenarios where the problems become obvious.

Nerdfest
+1  A: 

I frankly believe that someone's code is better when it's easier to change, debug, navigate, understand, configure, test and publish (whew).

That said I think it is impossible to tell someone his/her code is bad without having a first go at having him / her explaining what it does or how is anyone supposed to enhance it afterwards (like, creating new funcionality or debugging it).

Only then their mind snaps and anyone will be able to see that:

  • Global variables value changes are almost always untrackable
  • Huge functions are hard to read and understand
  • Patterns make your code easier to enhance (as long as you obay to their rules)
  • ( etc...)

Perhaps a session of pair programming should do the trick. As for enforcing coding standards - it helps but they are too far away from really defining what is good code.

rshimoda
+56  A: 

Start doing code reviews or pair programming.

If the team won't go for those, try weekly design reviews. Each week, meet for an hour and talk about a peice of code. If people seem defensive, pick old code that no one is emotionally attached to any more, at least at the beginning.

As @JesperE: said, focus on the code, not the coder.

When you see something you think should be different, but others don't see it the same way, then start by asking questions that lead to the deficiencies, instead of pointing them out. For example:

Globals: Do you think we'll ever want to have more than one of these? Do you think we will want to control access to this?

Mutable state: Do you think we'll want to manipulate this from another thread?

I also find it helpful to focus on my limitations, which can help people relax. For example:

long functions: My brain isn't big enough to hold all of this at once. How can we make smaller peices that I can handle?

bad names: I get confused easily enough when reading clear code; when names are misleading, there's no hope for me.

Ultimately, the goal is not for you to teach your team how to code better. It's to establish a culture of learning in your team. Where each person looks to the others for help in becoming a better programmer.

Jay Bazuzi
Seconded - if everyone in the team is having their code peer reviewed. the people you think have bad habits will not feel victimized
NotJarvis
If your colleagues respond well to such things, they're kinder than mine. When I try to pull comments like that, I'm generally told to deal with it. Or perhaps treated to a multi-page monologue about how their way is the only way. Even though .Net built in string-to-integer parsing, dangit.
Greg D
@Greg: Ouch. Tough crowd you got there!
Jay Bazuzi
Related to bad names: read about Stroop effect. Try reading the colors, not the words. Now think about how you name variables. If you don't find good names that actually describe what the variables are used for you will have a hard time reading and understanding your code when you come back to it later.
Igor Popov
+1  A: 

I do love code, and never had any course in my live about anything related to informatics I started very bad and started to learn from examples, but what I always remember and kept in my mind since I read the "Gang Of Four" book was:

"Everyone can write code that is understood by a machine, but not all can write code that is understood by a human being"

with this in mind, there is a lot to be done in the code ;)

balexandre
I found that to be true too. Nice reading:Things You Should Never Do, Part Iby Joel Spolskyhttp://joelonsoftware.com/articles/fog0000000069.htmlHe says it in his words: "It’s harder to read code than to write it."
mjustin
+1  A: 

You probably want to focus on the impact of the bad code, rather than what might be dismissed as just your subjective opinion of whether it's good or bad style.

JohnMcG
+1  A: 

Not that I'm really adding all that much to this, but I have to agree that the two most important things to consider in your approach to this are to explain your reasoning, and to allow the coder in question to explain their reasoning. Bad code doesn't come from nowhere (and, yes, "bad code" is certainly a term up for discussion - I'm somewhat assuming in this situation that you are in a position to define what constitutes good vs. bad code).

I've found that a questioning, educational approach works well with my team. I try to never say "do it like this" without any discussion or explanation as to why.

And while you should be somewhat sensitive about it, you can't sugar coat the issue. The ideal is that your team is thinking about the code they're writing, not just in terms of what the code is doing but in how it's written.

Lastly, I'd add that there are numerous books worth exploring on the topic - my favourite at this point is "Framework Design Guidelines" by Brad Abrams and Krystof Kwalina (et al), of the .NET BCL team at Microsoft. It does an amazing job of discussing and explaining the decisions that were made, and showcases places where the guidelines weren't followed internally and the fallout that resulted.

Jason
+1  A: 

In front of him just refactor his code and show the difference between the two versions. Definitely he will like that.

+1  A: 

I would suggest taking a positive approach to the issue. Instead of accusing your coworker(s) of using bad style, make some suggestions about style and commenting guidelines that your entire team could follow.

For example, if you guys are primarily a .NET shop, suggest adhering to Microsoft's C# style and commenting guidelines since this would put you more in line with the standard practices for that community.

You can also point out some of the examples to adhering to a unified code style--for example, if someone who isn't familiar with the code base looks through it, they don't have to decipher multiple styles. Think of it like this: if you were reading a book and it were easy to tell that each chapter had been written by a different person, might you be confused after a few chapters?

I think that the important thing is not to criticize your colleagues in a negative way. It's best to sell someone on the benefits of change and much easier than convincing them that they write crappy code.

Ed Altorfer
+1  A: 

I really liked EnderMB's answer, but I wanted to add to that:

Cultivate an environment where discussion of code quality is encouraged rather than seen as sensitive or taboo. For example, I've worked on an open source project (a Python library) where new code and bugfixes are frequently discussed with the group. Not only is it OK to say "hey, I think it's better to do it this way" but that's actually encouraged and part of the process we use for maintaining high quality code.

I realize not every environment is conducive to this kind of process, but it has really worked well for us. Every code commit doesn't have to be a committee meeting, but it should hopefully be perfectly acceptable for you to discuss questionable or non-optimal code and look for improvement. After all, better code is better for everyone on the team, and a major concept of teamwork is working together instead of as a loose group of individuals.

Jay
+1  A: 

I'm not the lead developer on my project and therefore can't impose coding standards but I have found that bad code usually causes an issue sooner rather than later, and when it does i'm there with a cleaner idea or solution.

By not interjecting at the time and taking a more natural approach i've gained more trust with the lead and he often turns to me for ideas and includes me on the architectural design and deployment strategy used for the project.

Nick
+3  A: 

Have code reviews, and start by reviewing YOUR code.

It will put people at ease with the whole code review process because you are beginning the process by reviewing your own code instead of theirs. Starting off with your code will also give them good examples of how to do things.

Giovanni Galbo
+1  A: 

I don a toga and open a can of socratic method.

The Socratic Method named after the Classical Greek philosopher Socrates, is a form of philosophical inquiry in which the questioner explores the implications of others' positions, to stimulate rational thinking and illuminate ideas. This dialectical method often involves an oppositional discussion in which the defense of one point of view is pitted against another; one participant may lead another to contradict himself in some way, strengthening the inquirer's own point.

Ed Guiness
The problem with the Socratic method is that no one has the patience for it, nor the willingness to follow along.
Patrick Szalapski
+6  A: 

There's some really good advice in Gerry Weinberg's book "The Psychology of Computer Programming" - his whole notion of "egoless programming" is all about how to help people accept criticism of their code as distinct from criticism of themselves.

andygeers
+2  A: 

Bad naming practices: Always inexcusable.

And yes, do no always assume that your way is better... It can be difficult, but objectivity must be maintained.

I've had an experience with a coder that had such horrible naming of functions, the code was worse than unreadable. The functions lied about what they did, the code was nonsensical. And they were protective/resistant to having someone else change their code. when confronted very politely, they admitted it was poorly named, but wanted to retain their ownership of the code and would go back and fix it up "at a later date." This is in the past now, but how do you deal with a situation where they error is ACKNOWLEDGED, but then protected? This went on for a long time and I had no idea how to break through that barrier.

Global variables: I myself am not THAT fond of global variables, but I know a few otherwise excellent programmers that like them A LOT. So much so that I've come to believe they are not actually all that bad in many situations, as they allow for clarity, ease of debugging. (please don't flame/downvote me :) ) It comes down to, I've seen a lot of very good, effective, bug free code that used global variables (not put in by me!) and great deal of buggy, impossible to read/maintain/fix code that meticulously used proper patterns. Maybe there IS a place (though shrinking perhaps) for global variables? I'm considering rethinking my position based on evidence.

David Frenkel
A: 

People writing bad code is just a symptom of ignorance (which is different from being dumb). Here's some tips for dealing with those people.

  • Peoples own experience leaves a stronger impression than something you will say.
  • Some people are not passionate about the code they produce and will not listen to anything you say
  • Paired Programming can help share ideas but switch who's driving or they'll just be checking email on their phone
  • Don't drown them with too much, I've found even Continuous Integration needed to be explained a few times to some older devs
  • Get them excited again and they will want to learn. It could be something as simple as programming robots for a day
  • TRUST YOUR TEAM, coding standards and tools that check them at build time are often never read or annoying.
  • Remove Code Ownership, on some projects you will see code silos or ant hills where people say thats my code and you can't change it, this is very bad and you can use paired programming to remove this.
Scott Cowan
I believe willful ignorance could be described as dumb? It's the same as not being willing to learn.
Adam Naylor
The example of this is a guy who's a partical physicist but can't get a girlfriend. He is obviously a smart guy but ignorant about women.
Scott Cowan
+1  A: 

Instead of having them write code, have them maintain their code.

Until they have to maintain their steaming pile of spaghetti, they will never understand how bad they are at coding.

JDrago
Even better: ask them to maintain code of other developers. This might also help to improve the communication between them...
mjustin
That's a lot less antagonistic, too :-)
JDrago
+1  A: 

I can't emphasize patience enough. I've seen this exact sort of thing completely backfire mostly because someone wanted the changes to happen NOW. Quite a few environments need the benefits of evolution, not revolution. And by forcing change today, it can make for a very unhappy environment for all.

Buy-in is key. And your approach needs to take into account the environment you are in.

It sounds like you're in an environment that has a lot of "individuality" to it. So... I wouldn't suggest a set of coding standards. It will come across that you want to take this "fun" project and turn it into a highly structured work project (oh great, what's next... functional documents?). Instead, as someone else said, you'll have to deal with it to a certain extent.

Stay patient and work toward educating others in your direction. Start with the edges (points where your code interacts with others) and when interacting with their code try to take it as an opportunity to discuss the interface they've created and ask them if it would be okay with them if it was changed (by you or them). And fully explain why you want the change ("it will help deal with changing subsystem attributes better" or whatever). Don't nit-pick and try to change everything you see as being wrong. Once you interact with others on the edge, they should start to see how it would benefit them at the core of their code (and if you get enough momentum, go deeper and truly start to discuss modern techniques and the benefits of coding standards). If they still don't see it... maybe you'll need to deal with that within yourself (especially on a "fun" project).

Patience. Evolution, not revolution.

Good luck.

shank
+1  A: 

Nobody likes to listen someone saying their work sucks, but any sane person would welcome mentoring and ways of avoiding unnecessary work.

One school of teaching even says that you should not point out mistakes, but focus what is done right. For instance, instead of pointing out incomprehensible code as bad, you should point out where their code is particularly easy to read. In the first case you are priming others to think and act like crappy programmers. In the later case you are priming for thinking like a skilled professional.

Bloodboiler
A: 

It is important to motivate and coach people and be show respect even if someone obviously does mistakes. But there should be the way not only to coach but also to state that mistake is mistake. Bad code should be done better. It is not optional. And employee should be aware which code is ok and which not ok from point of view of his supervisor. It is still supposed to be done with respect and motivate those who are accountable to improve.

Din
+1  A: 

Depends on the programmer. Some guys actually like to hear "that sucks" because they knew the code smelled but weren't sure why.

Other programmers need to be babied a little more. I find telling them something is bad is good; "that's not a good way to write code" followed by a bit of coaching "here, see if we do this it's more readable/less warnings/whatever". It's the constructive criticism that helps; if you can't put your money where your mouth is and actually do it better you're best not to comment, even if you know it's bad.

The only person that both approaches have failed on was a stubbon admin assistant who was writing enormous macros in VBscript and going about everything backwards. She actually had the gall to tell me that I didn't know anything about computer programming and that I could stand to learn from her 1337 sk1l50rz.

Adam Hawes
A: 

It totally depends on the culture you're writing in. In a free software project, you tell them they're writing bad code with positive suggestions, ways to improve it and feedback. You can also send them a patch to their code.

A friendly email never hurts, either.

mattl
A: 

I have a similar senario with the guys i work with.. They dont have the exposure to coding as much as i do but they are still usefull at coding.

Rather than me letting the do what they want and go back and edit the whole thing. I usually just sit them down and show them two ways of doing things. Thier way and My way, From this we discuss the pro's and cons of each method and therefore come to a better understanding and a better conclusion on how should we go about programming.

Here is the really suprizing part. Sometimes they will come up with questions that even i dont have answers to, and after research we all get a better concept of methodology and structure.

  1. Discuss.
  2. Show them Why
  3. Don't even think you are always right.. Sometimes even they will teach you something new.

Thats what i would do if i was you :D

Shahmir Javaid