33

What do you when you're working with someone who tends to write stylistically bad code? The code I'm talking about is usually technically correct, reasonably structured, and may even be algorithmically elegant, but it just looks ugly. We've got:

  • Mixture of different naming conventions and titles (underscore_style and camelCase and UpperCamel and CAPS all applied more or less at random to different variables in the same function)
  • Bizarre and inconsistent spacing, e.g. Functioncall (arg1 ,arg2,arg3 );
  • Lots of misspelled words in comments and variable names

We have a good code review system where I work, so we do get to look over and fix the worst stuff. However, it feels really petty to send a code review that consists of 50 lines of "Add a space here. Spell 'itarator' correctly. Change this capitalization. etc."

How would you encourage this person to be more careful and consistent with these kinds of details?

8
  • 3
    "Pretty printers" help. Also, does your firm have a style guide? Dec 3, 2010 at 16:01
  • 1
    what about coworkers who don't have any grammar? ;)
    – Muad'Dib
    Dec 3, 2010 at 16:04
  • 5
    @JSBangs: install a pre-commit style checker and have it refuse commits. That will get them to format correctly quickly. Or have the pre-commit hook run a formatter for you. Some stuff will look but it's better than weird is better than "horrible" I guess.
    – haylem
    Dec 3, 2010 at 16:18
  • 3
    One more thought - it may seem petty, but its petty to a purpose (assuming that a) there's a coding standard and that b) everyone else agrees and adheres to it)
    – Murph
    Dec 3, 2010 at 16:21
  • 3
    What's this programmer's background? Sounds like he's worked for too many different companies with too many different code formatting conventions, and his brain has internalized them all into a jumbled mess. :-) Dec 3, 2010 at 20:24

17 Answers 17

21

Agree on a coding convention

Even if this is a one pager. I suggest that the whole team sit down and all agree on basic working coding convention that the whole team can use.

3
  • 1
    Absolutely - then a) everyone is trying to achieve the same standard and knows what it is and b) your rejection at the code review can be reduced to "fails to adhere to coding standards" (at least if the file as a whole is a mess - if its just one or two things you'll need to be specific)
    – Murph
    Dec 3, 2010 at 16:20
  • 1
    Usually I've never seen a team managing to "agree" in one hour on a fully-blow coding convention for any language :) But if by agree you mean "discuss until disagreement, and then impose by rank and authority", then that works. You have to put your foot down on some points because you won't find consensus, or you're very lucky with your team.
    – haylem
    Dec 3, 2010 at 17:45
  • 1
    I'd go one step further: use an automated tool to verify that any code that is checked into source control matches the coding conventions. Nowadays, there are code style checkers for every language. Some languages even have automatic code style fixers. Jun 25, 2020 at 11:39
28

I think you just have to keep doing what you are doing. Have a clear set of coding guidelines, and enforce them during code reviews. If a developer gets 50 or 100 lines of "Add a space here" and "Spell 'iterator' correctly" every time he tries to check something in, and he is actually not allowed to check in before all of those get fixed, eventually he'll have to start writing cleaner code just to avoid the hassle.

I think if you fix these things yourself, like NimChimpsky suggested, you will be cleaning up after this person forever.

9

I call BS on everyone who said that variable name spelling errors and formatting don't matter. Obviously, they've only read their own code. And notice that word right there - read. Imagine reading a book with lots of spelling mistakes, mish-mashed formatting, inconsistent line spacing, and various other laziness prevalent in a lot of source code. It would be tedious.

For a profession where your syntax must be 100% correct to work, there is simply no excuse for any real developer to not have a clean, consistent code style. Anything else is sloppiness and laziness. I always question sloppily formatted code's correctness in implementation.

5

I think conventions such as naming of classes and variables are important and should be followed through, elegant and efficient code too, but at the risk of having my answer downvoted many times, I have to say that in general the "pretty code" paradigm that is pushed a lot around is in IMHO very overrated.

First off, the developer who wrote it will have to maintain it in the first place, and if he is ever hit by a bus and another programmer cannot figure out how it works because the code is not "pretty", I'd say that the other developer is not very good anyway. And there is a lot of automated formatters/beautifiers out there, so anyone can use those to beautify the code if necessary, w/o wasting time while being "in the flow"/"in the zone".

Please note that I'm not advocating spaghetti/cowboy style coding here, in fact I've seen some very nicely formatted spaghetti code (function bodies spanning 4-5 screens, global variables scattered around different source code files, generally bad picks of names, etc).

2
  • What do you say about code like this: stackoverflow.com/questions/6221098/save-mapview-as-a-bitmap/… Do you still think that the programmer who has do deal with this "style" is a bad programmer if he have serious trouble with it? Jun 3, 2011 at 9:21
  • @WarrenFaith, you might want to revisit my 3rd paragraph, particularly this piece here: "Please note that I'm not advocating spaghetti/cowboy style coding here...".
    – Jas
    Jun 3, 2011 at 11:53
4

We have a good code review system where I work, so we do get to look over and fix the worst stuff. However, it feels really petty to send a code review that consists of 50 lines of "Add a space here. Spell 'itarator' correctly. Change this capitalization. etc."

I'd change it myself and then add a polite comment in the code.

this assumes that there is already a style guide as the question stated :

We have a good code review system

So my suggestion is a last resort, I figure its just as quick to change it yourself and leave a comment, as it is to send an e-mail or whatever.

2
  • 11
    That gets old pretty fast. Dec 3, 2010 at 16:07
  • 3
    It's probably faster for you than to send an e-mail, and faster overall for the problem to be fixed, but it's slower than if the problem doesn't occur in the first place.
    – haylem
    Dec 3, 2010 at 17:46
3

One of my colleagues writes html in such a way that it makes my skin crawl. Imagine my html nice and structured with two space indents, hacked to pieces by tags added to the end of mine which end on the same line or on the next like some drunk who needs to throw his arm around you to stay standing. New lines are rarely indented but if they are, I'm sure there's some highly chaotic black hole in some part of the galaxy spitting out irrational temperature values in such a way that somehow its digits mirror the number of spaces or tabs used in such indentation by this woman. If I'm lucky, I'll see an input tag that's closed with "</input>". Total nightmare you can understand.

Nobody seems to understand this either, seeing how for most higher ups here, organized code or unorganized code to them is like the difference between us putting swiss cheese or american cheese on our sandwiches, which is to say, they could really care less. I started letting it slide because I was stressed with another project, and I think she began to realize how difficult it was to understand code like that before she wanted to improve. My advice would be to demonstrate why it's preferable to style your code more than simply telling them to do it.

3

The code I'm talking about is usually technically correct, reasonably structured, and may even be algorithmically elegant...

Be happy that you got all of that. Most programmers will maybe give you the first thing on that list. I think that variable naming and spacing is the least important thing to worry about.

5
  • 3
    Programmers spend more time reading code than writing code. If code is unreadable, the cost of extending or maintaining it becomes huge. And if variable names are inconsistent, misspelled, and non-descriptive, that makes code unreadable.
    – Dima
    Dec 3, 2010 at 17:49
  • @Dima, true, but that code that works and is elegant is already easier to read than code that is broken and inelegant.
    – jjnguy
    Dec 3, 2010 at 17:50
  • 1
    My point is that you should be able to look at a variable name, or a class name, or a function name, and immediately know how to use it without having to dig through the entire code base. You should also be able to type the name following the conventions and get it right without having to look it up. I recommend you read "Clean Code" by Robert C. Martin.
    – Dima
    Dec 3, 2010 at 17:56
  • @Dima, I agree that variables should have descriptive names. The OP doesn't mention that the names are bad, just that they are inconsistent.
    – jjnguy
    Dec 3, 2010 at 17:58
  • 1
    In my experience, when names are inconsistent, they also tend to be non-descriptive. But there is another problem. When names are inconsistent, it takes you longer to remember what they are, and you have to spend time looking them up. A good IDE can help somewhat, but it would not fix the problem completely. Programming already puts enough load on your brain, so you want to reduce the amount of mental mapping and double-checking as much as possible.
    – Dima
    Dec 3, 2010 at 19:52
2

Sounds like you need to set up and agree to a style convention. If you don't you'll have libraries that have 3 space indents, others that have 4, some that use Camel Case and other that use underscore_case.

2

Are the changes you want to make your personal preferences or do you have an actual standard to follow? If you don't have an actual standard, then don't do this. First set a standard. Then you can get software that can be set to refactor the code to the standadrd settings (well at least of some things).

If you have a standard, start enforcing it in code review. There is no point to having a standard if you don't enforce it in code review. This will mean a lot of extra work in maintenance as people will have to fix old code that didn't meet the standard orginally when they touch it.

Even without a standard, insist on fixing mispellings in variable names (I wouldn't particularly worry about comments) as they will drive everyone who touches the code crazy forever.

2

Coding standards need to be identified so everyone knows what they are and then they need to be enforced. There should be consequences to not following the rules.

Here are the things that should provide some incentive:

  1. Code reviews will be tedious and longer than necessary.
  2. Code will be rejected more often.
  3. Schedules will not be met.

If this person either doesn't have to worry about this because no one enforces your rules or they don't care if they are unproductive (and no one does anything about that), there's not much you can do about it.

2

I'd be tempted to suggest having a private chat and see if both of you could find a root cause:

  1. Is the co-worker rushed and because someone wanted the code yesterday the person is trying to get something working as fast as they can? This can be an opportunity to inform this person to focus more on quality than speed in their work. A mantra like, "Take your time," could be useful if this isn't counter-productive.

  2. How does the person view their work? If there is a sense of pride, then you may have an angle to use in getting one to improve. If it is just a job that pays the bills, it may be a lot harder to get changes. Do they know they aren't doing a great job but are so close to it?

  3. Does this person disagree with the conventions and is trying to code in protest? If so, then you may have a big problem, but this is worth finding out if this is the case or is the person merely lazy? What kinds of motivation may be useful here,e.g. could you appeal to greed, pride, or some other vice to get the person to improve. This is sneaky but possibly effective if trying the nice guy route gets one nowhere.

  4. How to Win Friends and Influence People has a few suggestions in terms of being persuasive that may work, such as praising improvements and giving the person a fine reputation to uphold.

As for why this has to be done privately, here are a few reasons:

  1. There is a good chance for humiliation, criticism or other unpleasantness that is better kept behind a door than left out in the open where someone may feel their character is being assassinated.

  2. You want to encourage this other person to open up a bit. A challenge here is that some people are so guarded it can take a long time to get them to take their walls down.

  3. If possible, I'd suggest trying to do this a little away from the office. Go out for lunch, take a walk, or do something so that the surroundings are altered enough that the person may feel a bit more comfortable. This can be a challenge and requires knowing the person, but the idea here is that in the office some people will wear a work mask that isn't likely to be helpful here.

  4. Be prepared for the conversation to get rather heated or ugly, but this could well be a good sign if you can keep the other person engaged and have a good dialogue. Some people like to keep things out in the open and others may prefer more subtle ways of getting things done. The key is to make sure you are listening to the other person enough to empathize and try to understand their side.

2

We have a JUnit test that looks for formatting issues. It runs as part of the build. I continually get bit by omitting a space between if, while or for and the opening parenthesis. Our code is consistently formatted, though.

http://code.google.com/p/kawala/wiki/BadCodeSnippetsRunner

1

Code beautification like unscrutify will be able to solve some of your problems. If you are ready to pay for this then there are high level softwares which embed the rules in the source code itself like Parasoft. Parasoft makes it compulsory to write the code in uniform style. You can embed your own rules too. When such tools are used the developers are forced to use uniform style. And after a while they will get used to it.

1

If you use Eclipse, then enable Save Actions for the Java Editors, and tell everybody to use it. This fixes formatting issues on every save, but does not fix bad capitalization. It might be quite helpful though!

alt text

1

How hard is it to follow style conventions? I understand the spelling errors but the rest is an indicator of sloppy thinking and coding. Tell the person that they need to be more consistent when it comes to production code because they are not the only ones that will be looking at it. It's just plain rude, selfish, and inconsiderate to write production code in an inconsistent style.

0

LOL. You would absolutely hate my code. I can't spell to save my life and I don't care.

But I know some people do actually care about those things.

I suggest you fire the person writing that ugly code if they don't change then find somebody that makes things really pretty and hope they can write code that

is usually technically correct, reasonably structured, and may even be algorithmically elegant

and if they can't then at least you can show the broken pretty code to the customer and sell them on that!

But seriously. Focus on the actually important things first. If you can't find a good, solid reason outside "it hurts my delicate sensibilities" then ignore it for now. If it is actually important then sit down with the person and convince them of that importance. Things like standards that make it easy to tell the difference between class level, method level, shared, constant variables do make a difference. If the coder in question cares at all about his/her profession they will understand and try to do the right thing.

4
  • 5
    If your variable names are misspelled, the next guy who uses your code will have to spend extra time fixing compiler errors when he does spell them correctly. This is not about "delicate sensibilities". These seemingly trivial things cause errors, which cause frustration, which causes more errors. All that adds up to huge code maintenance costs.
    – Dima
    Dec 3, 2010 at 17:45
  • 4
    It's a bit more than about "delicate sensibility", but productivity I don't mind someone with slightly different coding styles, occasionally forgetting a space, etc... We're not perfect. But when the whole file looks like it's been writtenby an undergrad, w/ inconsistent line spacing, positions, indenting and general code flow, then I'd just hit the "reject" (or revert) button very quickly.
    – haylem
    Dec 3, 2010 at 17:53
  • 2
    A lot of successful open source projects (linux included) do this: if you don't have the right style (and the unit tests), then it's rejected. Too bad if it was good and solved a real problem: you cannot always fix other people's code. Overall, you loose less time and money just passing on the occasional piece of genius that goes through but looks like hell or is unmaintainable.
    – haylem
    Dec 3, 2010 at 17:54
  • 1
    Funny stuff. But of course the main point seems to be missed. You first get the guy on board with the obvious things that you can make an actual strong case for. Then you work on the less important things. There are ways of working with people outside of just choking them with rules. And maybe, just maybe, the OCD crowd might compromise a bit or learn why there is so much variablitly in the others code. There might actually be a cause or reason. Dec 3, 2010 at 20:31
0

My solution when dealing with outsourced resources that didn't give a &%$# about formating (or easily preventable bugs for that matter) was to have the build server enforce this. I created a CI server job that ran every night that checked the code out, ran Jalopy and findbugs then checked the code back in. Once the other team learned that not using the standard code conventions would make their job harder, they started to use their IDE to maintain a standard format.

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