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 work on a new venture at a large enterprise software company (3000+ programmers). In my group, we have a bunch of projects and people usually work on several projects over the course of a year.

I just started work on a project that has been previously maintained by a buddy of mine (consultant who's been with us for 3+ years) to add some features. I got into the code, and the quality was really quite poor. Whether it was on the UI frontend or the services backend, the code simply wasn't indented, there were hundreds of lines commented out for no apparent reason, documentation was basically non-existent, coding standards weren't applied consistently (e.g. mixing camelCase and under_scored_variables), variable names were unintelligible, datatype choices were wrong, etc. etc.

I'm very much a non-confrontational person, so I don't want to attack my coworker, but I also don't just wanto to go to my boss and complain about his performance. What are the kinds of things I could say to politely mention that the code is poorly structured?

EDIT:

I want to clarify that while I understand there is an element of "Everyone else's code sucks" to all programmers, when I see something like this (names are chosen on purpose and some details left out/changed in this example):

public void doCalculate(Object argument) {
  if (argument instanceof String) {
    String argument2 = (String) argument;
    if (argument2 == "DataBase") {
      // do something
    } else {
      long argument3;
      try {
        argument3 = Long.parseLong(argument2);
      } catch (Exception e) {
        argument3 = -1;
      }
      // do something completely unrelated
    }
  }
}

I think it's objectively fair to say that this is not a good idea. Furthermore, I'm not dealing with a newbie here (I'm only coding 3 years now). He's got maybe 20 years of experience on me. The advice you guys have given so far is great; just wanna make sure that we're not talking about a "fine line" here.

share|improve this question
7  
I don't think this is a duplicate: speaking to your boss and speaking to your coworker is entirely different. –  Tamás Szelei Jan 11 '12 at 17:38
3  
I feel it shouldn't matter if the other person is a co-worker or a boss, they're still both people and you should approach them the same way and with the same amount of respect. –  Rachel Jan 11 '12 at 17:42
6  
@Rachel: I think it depends on the boss. If they aren't very approachable and don't take kindly to criticism, there could be consequences. –  Bernard Jan 11 '12 at 18:18
2  
None of the defects you mentioned were especially egregious. Sounds like ordinary lackluster production code to me. They may be symptoms of deeper problems but those things aren't particularly horrifying by themselves. If you have a problem that is keeping you from making progress, then it is worth asking the original dev for help. Otherwise, not worth the hassle to get on someone's case about indentation and variable names and some inconsistencies. –  Angelo Jan 11 '12 at 19:29
4  
Are code reviews usual at your place? If not, you might want to suggest them. That's the time for the reviewers to point out these bad practices. –  Pete Wilson Jan 11 '12 at 21:01
show 10 more comments

12 Answers

Ask him to explain his code to you

Tell him you've never seen X programmed that way before, and ask him why he codes it that way. Show him the way you code it, and tell why you do it that way (best practices, better performance, less chance of errors, easier for other programmers to read/maintain, etc).

Be sure to prepare all your arguments in advance, and focus on why your method is best instead of why his method is worst. Afterwards, see if he still supports his method over yours.

If he is open to improvement, he will likely change his way of coding. If he still prefers to use his style of coding over yours, you are not likely to change his opinion.


This is the exact same answer I gave for the question How to tell your boss that his programming style is really bad?. I originally voted to close this question as a duplicate, however enough people thought it was different enough to warrent re-opening it. My answer is still the same though, regardless of if you're talking to a boss or a co-worker.

share|improve this answer
    
+1 I was going to write a similar answer before I saw this one come up. I'd also add that unless he has a lot of spare time or is instructed to do so, I doubt he'll fix his old code -- but it might convince him to do better in the future. I'd also wonder whether it was such a mess before he worked on it (if he wasn't the first)... –  Dmitri Jan 11 '12 at 19:16
1  
this seems pretty formal for talking to a coworker –  Ryathal Jan 11 '12 at 19:26
1  
@Ryathal It doesn't have to be format at all. For example, you could ask them what their naming convention is, or why they have two different ones. Then follow up with your own naming convention and why you use it. You could also ask them what the large commented out code does and if it's OK to remove it, and then explain that the old code will be in version control if they every want to access it. How you phrase the questions will likely be related to how comfortable you are with the person. –  Rachel Jan 11 '12 at 20:05
1  
I might also add to ask him about code he wrote a month+ ago. Even an author wont be able to grok his own code with bad variable names, bad indentation, etc. –  John Bubriski Jan 17 '12 at 19:19
    
You wouldn't believe the number of people who just code without thinking about WHY. "Why" stumps their own selves, and they don't bother to ask why others code in whatever way. Just follow like a rigid template and we're full of win! Next project! –  icelava Feb 6 '12 at 8:27
add comment

What if instead of approaching him personally, you approach the team and in a completely neutral way propose on "team" coming up with a coding standard/guidelines as a way for different team members to work more efficiently with each other's code.

Once that is in place, I think the code you have concerns with will bubble up to the top all on its own.

Peer code reviews also help in this area. Coding standards don't have much benefit if no one ever looks at the code. But peer code reviews have many other benefits, primary one knowledge transfer, so you should push to introduce them in either case.

I'm making an assumption that even if you have 3000+ engineers, you guys have MUCH smaller sub teams that work together as a unit

share|improve this answer
add comment

First, everybody thinks other people's code sucks.

Second, maybe he inherited the code from someone/someplace else OR this was a prototype never intended to be the code used for the actual project but of course that's what ended up being used.

Third, maybe his code really does suck but is it your job to badmouth previous developers? Unless you have built up a reputation that commands respect around the company I suspect if you do whine about the other person's code it is you that will end up looking bad and not your friend. The time for giving people grief about their code is when code reviews are held. At least then it is part of your job responsibilities.

I recommend that if you don't like his code then fix it to your liking. Then the next developer will come along and complain about your code and probably change it back to be more like what your friend did.

share|improve this answer
39  
Every Good programmer thinks their old code sucks, let alone the code from others. –  Brad Gilbert Jan 11 '12 at 19:24
4  
I think the OP is asking how bring the subject of code quality up with a colleague without offending him/her. An answer like this isn't very useful because it is basically saying don't say anything at all. I would much rather have someone approach me with some constructive criticism than stay silent, because there are a lot of things I don't know that I don't know, and I would appreciate someone taking the time to help me learn. Also, I'd rather try and teach someone how to make their code more readable once than deal with their mess for the next few years. –  Rachel Jan 12 '12 at 14:48
1  
@Rachel-Most people have worked with the guy who complains all the time and eventually nobody respects that person's views, even if valid. Most people have also thought they were being helpful by giving unsolicited advice only to be met with less than positive responses. Since providing critique of the original programmers code is not the posters job description (since he isn't the guy's mentor, lead or doing a code review) the odds are that nothing good will come from his saying anything. The righteous view is to say you want constructive criticism but when it comes on you; do you really? –  Dunk Jan 12 '12 at 15:52
1  
@Dunk My personal option is yes! I'm almost completely self-taught and I love it when people take the time to show me things I didn't know before. For example, my early projects are things like single-class files with horrible naming conventions and no version control. I didn't know any better. There's no harm in bringing up the subject in a non-threatening way while taking to someone and seeing how they react to it, as long as you're willing to drop the subject if the person is unwilling to change their ways. –  Rachel Jan 12 '12 at 17:30
1  
@Dunk and I disagree that "likely nothing good will come from his saying anything". If approached right, and if the person is willing to learn (I feel its almost impossible to be a good programmer without being willing to learn), the person will start coding better and it will make future upkeep of his/her projects a lot easier. If you say nothing, you're probably stuck with maintaining projects using his/her (lack of) coding standards in the future. –  Rachel Jan 12 '12 at 17:33
show 2 more comments

Your co-worker is probably not the root cause of the problem in your company. The reason that poor quality code gets into production is the lack of automatically measurable coding standards in your company. In this case, sunlight is the best medicine.

You should talk to your technical lead about implementing automated source code quality metrics in your group. The system should run at least weekly, e-mail everyone on the team a file-by-file list of coding standard violations, and send an executive summary to your boss. The summary should contain the number of violations per project.

In my experience, anything that gets measured and reported to a boss improves over time.

share|improve this answer
add comment

...the code simply wasn't indented, there were hundreds of lines commented out for no apparent reason, documentation was basically non-existent, coding standards weren't applied consistently (e.g. mixing camelCase and under_scored_variables), variable names were unintelligible, datatype choices were wrong...

As far as I can tell, above matches #12, #9, #5, #2 and probably #7 of Top 12 things likely to be overheard if you had a Klingon Programmer:

12. Specifications are for the weak and timid!
11. This machine is a piece of GAGH! I need dual Pentium processors if I am to do battle with this code!
10. You cannot really appreciate Dilbert unless you've read it in the original Klingon.
9. Indentation?! - I will show you how to indent when I indent your skull!
8. What is this talk of 'release'? Klingons do not make software 'releases.' Our software 'escapes,' leaving a bloody trail of designers and quality assurance people in its wake.
7. Klingon function calls do not have 'parameters' - they have 'arguments' - and they ALWAYS WIN THEM.
6. Debugging? Klingons do not debug. Our software does not coddle the weak.
5. I have challenged the entire quality assurance team to a Bat-Leth contest. They will not concern us again.
4. A TRUE Klingon Warrior does not comment his code!
3. By filing this PTR you have challenged the honor of my family. Prepare to die!
2. You question the worthiness of my code? I should kill you where you stand!
1. Our users will know fear and cower before our software! Ship it! Ship it and let them flee like the dogs they are!

http://i.stack.imgur.com/16MVS.png

It is quite likely that the only way to avoid confrontation is a warp drive into another space quadrant.

share|improve this answer
    
+1 Because Klingons are awesome –  Spoike Jan 12 '12 at 22:13
add comment

Get some background about what was going on within the company and the team when this project was written. Ask this dev for some feedback on how he thought the project went. Has his style changed? If given the opportunity, what would he do differently?

He may agree with your standards, but just didn't have the luxury of applying them during the creation of this app or just didn't know about them.

Either your group has no standards or code review policy or a serious problem with enforcement. You may be holding an opinion of one.

share|improve this answer
add comment

I'll admit it, I am sometimes that guy. When I am, I will have my reasons, usually time constraint, death-march projects, instructions such as "now, not perfect" etc. Very rarely, it's because of a bad day. I am happy to get asked about any issues, it gives me a chance to become a better developer. Provided a) You are reasonable about what will be changed (ain't that badly broke, agree to don't fix), and b) your not a nut job dictator who believes your way is the only way.

To approach me about it, do not ask "Why". Why is a fighting word, a challenge, and is ultimately destructive. If you find yourself using Why too much, change it to I. "Why did you call this x" becomes, "I would have used index, it's more descriptive than x."

The best way is to state what you would have done, or would have preferred. Explain that what you are seeing does not met certain standards, and that you would like to see it improved. My guess is that 99% of the time, the response will be "so would I, but........"

share|improve this answer
2  
+1 for the "Why" warning: most people go on the defensive as soon as you say "Why" –  AAT Jan 11 '12 at 23:17
    
Me too, I'm sometimes the guy producing poor quality code not because I don't care but because of constraints like this one time I was the only developer doing 3 developers' work. I did went back to improve the quality when project finally had budget to add new developers though. –  Alvin Jan 12 '12 at 2:11
add comment

Buy/loan him the book Clean Code, and tell him it really helped you, and you think he would enjoy it.

And...read the book yourself if you haven't! It really is quite good.

share|improve this answer
    
Why a -1? explanation? –  CaffGeek Nov 21 '12 at 17:28
add comment

It is unlikely that the author of the code will come back to fix his code for you out of pure good will, or that your employer will pay him to do that. So, either way, you are scr*wed.

If you believe that the mess the code is in will have a severe detrimental effect on your performance while working on it, be sure to save a backup of the original code somewhere so that you can prove that it was a mess when it was delivered to you. So, if your employer confronts you later about your poor performance, you can cover your ass.

Still, it may look to your employer like an excuse, so try to avoid even that happening if you can.

share|improve this answer
    
No need for a backup. That's what source control is for! If there is no source control being used, you've got bigger problems than just dealing with messy code. –  Bernard Jan 11 '12 at 18:20
1  
Also, once messy code has been put into production, any attempt to "clean up" or refactor it should require a full regression test. And that can be very expensive and time consuming. After all: "if it ain't broke (according to real defects filed by users and tracked in a trouble-ticket system), don't fix it!" –  FrustratedWithFormsDesigner Jan 11 '12 at 20:52
add comment

Think about what good mentioning him that he has a bad coding style would do you or him?

Perhaps it's better to try to improve your company's QA procedures.

share|improve this answer
    
QA in my experience doesn't involve code review. :( –  Karlson Jan 11 '12 at 19:52
1  
@Karl - QA is supposed to be involved in the entire process, assuring quality is met in all work products. Otherwise, what is the point of QA? –  Dunk Jan 11 '12 at 20:14
1  
@dunk It's supposed to but I don't know many places where it actually does. –  Karlson Jan 11 '12 at 20:18
add comment

You need to find out two things: What is your goal? How would you like to be approached yourself?

If your colleague, with 20+ years of programming behind him, is really, really open-minded, you can just go talk to him and point out specific examples, tell him why they looked wrong to you and discuss alternatives. Because he's really, really open-minded and keen to learn, he'll listen, a discussion will ensue, and he'll understand your points and you'll both live happily ever after.

In my experience, though, the colleague is most likely programming like that because he just doesn't care about style. It could be because he's really in the wrong job, or it could be because, in 20+ years of programming, he never found a reason to worry about it - maybe his code just works and he moves on.

If the latter is the case, you're just going to waste your and his time, and you may make a dent in your relationship if it turns into an argument. So what is your goal here?

Do you want him to clean up his code so that you can work on nicer code? Do you want him to learn and improve? Do you want to feel good about being a better coder? Do you want your boss to know that you're a better coder than him?

The first one is not going to happen and the third is already established, although you probably want him to recognize it as well - good friend, father figure?

If you really want him to learn and improve, then figure out how you would like to be approached in the reverse situation, and do the same. And figure out what would annoy you, and make sure you don't do that.

The forth one is easy: either you want a better salary, and then you just demand it during the next feedback meeting (it doesn't really matter how good you are in this industry) or you want more responsibility and then you just keep doing a good job until you get it.

share|improve this answer
    
I like your thought process here. Though you missed a step in your fourth paragraph, which is that I don't want bugs in the app. I don't care about my boss knowing, otherwise I'd approach her. There's a bit of 1 and 2 in my thinking, but mainly I know this will be very hard for multiple people to maintain going forward, and codebases change hands alot, so a good style makes everybody's job easier. –  daveslab Jan 24 '12 at 22:37
add comment

Suggest you boss that you have been seeing some crazy code and ask him to put all possible staff on some programming courses. I think he should be aware that his apps are being filled with crappy code.

share|improve this answer
2  
Please explain how behavior like mixed naming, sections commented out and no documentation can be corrected by a programming course. –  Simon Jan 12 '12 at 10:20
    
Its a subtle way of saying "here some people is making crap" and avoiding confrontation. And a good teacher can make a big difference on how people code because they are forced to see how someone, who knows what is doing, does stuff (and explains). If they dont know, someone should teach them and explain why and how do stuff, and i think is better for the office atmosphere that a teacher does that instead of a coworker. (Unless you are a well respected coworker who people go for answers, if not, people mostly doesnt want answers from the ones they see at a similar level) –  H27studio Jan 12 '12 at 10:28
    
Also, most people doesnt read books that companies give them, thats why i think a mandatory course is better. –  H27studio Jan 12 '12 at 10:29
1  
If you work at a company where people don't read books voluntarily and don't learn new tricks from each other, then you deserve the crappy code you'll get as a result ;-) –  nikie Jan 12 '12 at 13:17
    
I agree with that nikie :-) –  H27studio Jan 12 '12 at 13:44
add comment

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.