tags:

views:

1269

answers:

36

When reviewing somebody else's code, what is it that you usually find most disturbing?

+14  A: 

Usually that they can write better code than me.

Cyril Gupta
How lucky you are...
Vinko Vrsalovic
No, really. I've never been pissed off at crappy code. Maybe amused sometimes. It's a big shot in the arm for the ego to see faulty code everywhere, even in big programs. But now and then you see a gem of a routine, or a small application that run circles around your skills and then you're put back in place.
Cyril Gupta
In my experience, big projects seem to have more room for crappy code to hide, especially in places where it isn't changed often.
Paul Morie
+3  A: 

Working in a group environment I find it disturbing when different developers use different coding styles in the same document; sometimes even in the same function. You can come up with detailed coding standards, but a primary rule should be 'make your changes to a file look like the existing code in the file."

...and what Cyril Gupta said...

Gary.Ray
I worked with a guy once who insisted that Python's 4-space indentation convention was inherently superior to Ruby's 2-space convention. He worked on a Ruby on Rails project with me and any code he added used 4-space tabs, so not only did he not maintain the existing tab convention, but he also did not go back and update any of the 2-space tabs to be his 4-space tab. Talk about arrogant...
Sarah Vessels
+2  A: 

Basic best practices are the things that I find most disturbing, for instance in Java when I see this

for(int i =0; i < List.Length ; i++){
    Object obj = List[i];
}

When something like this would have been better

for(Item item: List){
   Object obj = item;
}

Especially when IDEs like Eclipse can automatically clean up your code. Or you could use something like checkstyle to assure your code follows some template

PSU_Kardi
Why is the second better?
Liran Orevi
Effective Java: A Programming Language Guide , would argue that you should go with a foreach when you can.Anyway, more often than not someone is going to ask me to help them fix a snippet of code. They can either write it their way , or they can write it my way....In the end it's going to be my way , so it's easier to save the headaches
PSU_Kardi
@Liran: Usually it’s type safer without having to typecast and above all it’s shorter. You don’t have to check off-by-one errors etc.
zoul
@zoul - You sometimes need it. An obvious example is modifying the array while iterating over it. I'm sure you are aware of that though :P.
Jonathan C Dickinson
It bugs me even more that you don't put a space before the braces. That is **really** ugly.
Zifre
What about if the coder needed the iterator in the loop?
PoweRoy
+10  A: 

Lack of comments in the code yet detailed explanations in the review email.

JaredPar
and lack of comments in the code, and the email explanation of "my code is self-documenting" :)
gbjbaanb
+15  A: 

Extremely complex and clever ways to do things that can (and should) be done in much simpler ways. The other side of this coin is extremely unclever and complex ways to do things that can (and should) be done in much simpler ways.

Common manifestation: doing things that should be done in the database in application code, for example, simple aggregate functions like min, sum, etc, being calculated in application code instead of in a query.

Paul Morie
OMG it kills me when folks don't make use of the power of SQL to do addition, etc. From my understanding, DBMS's are optimized to handle that sort of thing, not to mention then you would have all of that logic in one language rather than spread between SQL and, say, VB.NET.
Sarah Vessels
+3  A: 

Where to start?

The fact that most Java programmers don't remember to close files?

In C, it's usually rampant cut and paste or explict if...then...else logic for every possible case, when it could have been done in a more general way.

From experienced programmers, I often see premature optimization; the code is incredibly complex because it works a few cycles faster on the archetecture that we happen to be on today. Maybe.

Chris Arguin
I would call that over-engineering. People that are out to prove how much smarter they are than everyone else..
PSU_Kardi
A: 

Bugs; especially any timing-related, intermittent bugs.

ChrisW
+5  A: 

The thing that disturbs me the most is finding code that was clearly never tested.

Coov
Many people argue you should do a code review *before* testing. It means you don't waste a lot of time testing something you will be told to rewrite.I understand the logic, but good luck getting programmers to let someone else look at their code before they've vetted out the stupid errors! I'm also not sure how that is supposed to work in a TDD environment...
Chris Arguin
@Chris I prefer the opposite: to not spend other people's time in finding bugs which the original developer could have found by self-testing. In fact, once the developer learns to test their own code sufficiently well, then code reviews no longer find any bugs in their code, and then you ow longer need to review their code at all!
ChrisW
Yeah, code review before testing makes little sense. Like ChrisW said, you're wasting the reviewer's time with untested code. Also, tests may help the reviewer understand how the code operates. Finally, the tests need to be reviewed too, right? Shouldn't they be part of the same change?
Laurence Gonsalves
Don't shoot the messenger! I agree, as it happens; at the least, the code should be tested for the more basic errors. It's not so clear to me when you stop testing and do the review. If you completely test it, you will have a lot of intertia to not fix basic maintainability problems. If you don't test at all, you waste peoples time on simple, avoidable issues.
Chris Arguin
+1  A: 

Usually a code written by people who think writing code doesnt need to be looked again once it starts working. Those people usually dont folloow standards, have very little programming expirience, don't comment and finally use some weird constructs like one

String a = null;
try {
  len = a.getLength();
} catch (NullPointerException e){
  len = 0;
}

to check simple length. Btw, I really came accross this code, multiple times, since it was copy/pasted in different places. Oh, and yeah, copy/pasted code is also really disturbing.

Azder
Don't worry eventually these people are elevated to software lead or better yet management. Then they say things like "it's just code" or "does it compile" and let the horrible trend of crappy developers continue
PSU_Kardi
+11  A: 

Another favorite of mine is

//TODO:  If this doesn't work, remove code below

Or another favorite is lines of code that have just been commented out. We have things like subversion for a reason. No reason to leave code commented out from 3 iterations ago

//Code 
//Code 
//Code
PSU_Kardi
+1 keeping commented-out code around works wonders in Evolution, but it's no good in the programming world.
MusiGenesis
+5  A: 
  • Inconsistent naming conventions
  • "Clever" code without any comments that could justify its existence
  • Tight coupling between modules
  • Spaghetti, or "copy-pasta" code
  • Code that follows only the "common" scenario, without any provisions for handling the edge cases
ASk
'Code that follows only the "common" scenario, without any provisions for handling the edge cases' - this one makes me want to scream. :/
Arnis L.
+1 for copy-pasta!
rq
+5  A: 
  1. Useless code
  2. Incomplete code
  3. Half-ass completed code
  4. Not knowing the API and re-inventing the wheel with the wrong 'tools'
leppie
+2  A: 

You might be interested in reading the "code smells" question - it goes over a number of common "code smells" (indicators of bad code) and how to correct them.

Eric Burnett
+18  A: 

Things that seem like evidence of cargo cult programming. Stuff that doesn't make sense in the context. Typically, when asked why the solution is the way it is, a reply of "I don't know."

Rytmis
+1 for citing anti-pattern
Paul Morie
+1 Great term - never heard it applied to programming before, but so true.
MusiGenesis
+1 for citing anti-pattern²
Ciwee
+1  A: 

Use of #if 0 to block out code. This beats the very purpose of having a source control & makes the code very hard to read. Another very common issue is not initializing variables, a huge no - no but occurs very very often.

Raam
Agreed, although I hate even more seeing variables initialized for no reason. When I ask why, the reponse is, "It's good practice to initialize a variable in case you accidentally use it"... in which case you've successfully hidden that you have a problem!
Chris Arguin
@Chris In C++ for example it's considered good practice to avoid/defer/delay defining a variable until you have a value with which to initialize it.
ChrisW
I've enjoyed that aspect of C#, that it will gripe at you for not initializing declared variables before use.
Sarah Vessels
A: 

pthread_create()

C Pirate
this isn't very constructive
wroks
+8  A: 
  • Spelling errors in class names, method names
  • Untested edge cases
  • Rolling your own sort or other standard utilities, etc.
  • //Programmer's name (this is what blame is for)
  • All methods public
  • Methods with more than about 50 lines
  • Clear lack of understanding of meaning of protected/internal/private/static,etc.
  • Bizarre UI conventions (a drop down list for the City field!?!)
  • Copy-pasting blocks of code
jspru
The problem with blame is if the line was modified in later revisions, it will not show in the blame, unless you start doing tricks with revisions.
Artem Russakovskii
+5  A: 

Incompetence
When I realize the code is so bad that it would take a huge effort to even make the person understand why it's bad, let alone fix it.

shoosh
A: 

The naming conventions are most disturbing. Somebody use the names for variable just as int a,b like is not a good practise. Atleast he should comment on it. Another one I notify is lack of comments in codes. A good programmer should write comments as he can possible. This help him to review code.

Anothe wrong stratergy coming is lack of functions. Some one writes a large code for a button clickis not good. So he should use functions for various purposes.

Sauron
+12  A: 

Code formatting. I absolutely can not read code, if it is not formatted consistently. Just thinking of the mix of tabs and space for indenting makes me shudder.

soulmerge
In NetBeans you can do Alt-shift-f and it auto formats your code =)
ChrisAD
Can do this in Eclipse too. They call it code cleanup
PSU_Kardi
In Visual Studio press CTRL +K +D to format your code/HTML markup.
Cyril Gupta
Yes, you can automatically format badly formatted code (`gg=G` in vim). But can you read it without re-formatting?
soulmerge
code formatting in eclipse => **ctrl + shift + f**
Narayan
This is why I check in formatters and preferences set to 'auto-format on save'. No more formatting problems.
Robert Munteanu
Or run StyleCop.
Joe
If you consider yourself to be at least a half decent programmer, then you should be able to read code no matter how bad the formatting is. You certainly can't give up just because the curly brace is in the wrong location. And if you do re-format automatically in your editor, **don't save it** or you will clutter up the version control commits with whitespace changes.
too much php
A: 

2-spaces indentation.

Igor Oks
I think 2-space indentation is quite perfect. Why waste all the spaces just to make code look wider?
Artem Russakovskii
I agree, 4 spaces is a minimum, (and 8 is *almost* too much).
Zifre
Follow the conventions of the community, and then get used to it.
guns
In the Ruby community, 2-space tab is convention.
Sarah Vessels
Really? Of all the things you could find in a code review, two space indentation is the worse? Dang, I want those coders to work for me!
Hogan
+2  A: 
  • No modular code

    10,000 in single file with 2000 lines for one function. Too difficult!

  • A Bad code

    getA()->getB()->getC()->getD()->getE()...getZ()->CallSomeNonsenseFunction()

aJ
2000 line functions! 200 is way too much...
Zifre
A: 

When reading lines from a file, making the exact same database call with the exact same data for every line, when the entire table being searched only contains about 30 rows.

ck
+9  A: 

Using the same variable for different purposes (a common habit of electrical engineers that write SW)

string buffer = ReadFileLine();
// ...
buffer = "Program Success!!!!";
Igor Oks
Also a common habit of assembly programmers, who aren't used to having a compiler with a decent register allocator.
Steve Jessop
A: 

When someone puts the opening bracket on the same line:

class A {
    /* Body */
}

instead of the one true way of doing this:

class A
{
    /* Body */
}
User
oh, come on... -_-
Arnis L.
If that's the most disturbing, you are in programmer heaven.
Gamecat
as nothing compared to the single-line implied brackets form
annakata
The other way around bugs me. I can't stand it when it's not on the same line. Of course, braces bug me in general. I wish I could just use Python-style indentation.
Zifre
of course the GNU coding convention is the worst - it has both indentation of braces, and lining them up. Like example 2, but where the {} and contents are also indented.
gbjbaanb
+1  A: 

Inefficiency - like copypasta repeated code and absence of modularisation is the worst. Also, where one lengthy block of code could be replaced by a 12 character line of code.

Antony Carthy
+2  A: 

That I'm reviewing someone else's code.

Justicle
+2  A: 

Code that's obviously been copied from a blog somewhere and pasted into production code.

Will
Biggest hint is code that's double spaced. These are the same people who aren't smart enough to rewrite the term papers they buy.
Will
+11  A: 
query = "select * from table where field like '%" + someTextBox.Text + "%'";
Chris
i can haz sql injekshun??
Sarah Vessels
A: 

Not to repeat everyone else but variables and functions named in a foreign language confuse [and disturb] the shit out of me.

Artem Russakovskii
+1  A: 

Not using const when they obviously should be...

Zifre
+4  A: 

A Modest List

  • Inconsistent formatting even within the lines being changed.

  • Plainly visible (through IDE highlighting) compiler warnings.

  • Highly chained method calls (guess which one was null, it's a game!)

  • C-style variable names. For example, buf, ptr, cbuf. You and others are going to spend 10 times as much time - at the very least - reading the code. So you should optimize for reading, not writing.

  • Strange euphemisms for execution:

    • "walk the collection"
    • "exercise this code"
    • "spin through these things"
  • Long methods that do a million different things.

If statements comprised entirely of:

Unreadable Boolean-Chain Statements

if (!relevantBoolean 
    && CustomStringUtil.equalsIgnoreCase(thing, Constants.C_THING_TRUE) 
    && (CustomStringUtil.equalsIgnoreCase(otherThing, Constants.T_ERROR_STATE 
        || CustomStringUtil.equalsIgnoreCase(otherThing, Constants.T_WARNING_STATE))) {
    // do some stuff
    ...
}

Basically, complex to very complex booleans that have no unifying name. Sure, I the machine can figure that out and get it right. Sure, I can puzzle through it. But it wastes so much time as I try to decipher it and it makes people afraid to change it because it's one big honking statement that requires a strong mastery of boolean algebra to pick apart.

Conversely, you can encapsulate that stuff and make it readable. Below is a

Contra-Example

boolean hasStuffWeCareAbout = hasStuffWeCareAbout(record);
boolean containsSomethingWeAreOmitting = containsSomethingWeAreOmitting(record);
boolean shouldProduceOutput = hasStuffWeCareAbout && !containsSomethingWeAreOmitting;

if (shouldProduceOutput) {
    //  do stuff
    ....
}

Sure, it's wordier, but it's comprehensible, each unit of logic is discrete, modifiable, self documenting, it's easily debuggable since you see the result of each step, and it's unit testable.

Trampas Kirk
"Walk the collection"? Is there a dance for that? Walk like a cuh-leck-shun...
Sarah Vessels
A: 

Variable names not declared according to the context .

mithunmo
A: 
Dispatcher.BeginInvoke( () => someTextBox.Background = Brushes.Red );
vanja.
A: 

Bad/Poor commenting.

thunderror
A: 

When I mark a section as needing a total rewrite, and then realize it's something that I wrote years before.

MusiGenesis
lol
RCIX