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

Question

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

Answer 1

Usually that they can write better code than me.

Answer 2

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...

Answer 3

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

Answer 4

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

Answer 5

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.

Answer 6

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.

Answer 7

Bugs; especially any timing-related, intermittent bugs.

Answer 8

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

Answer 9

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.

Answer 10

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

Answer 11

• 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

Answer 12

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'

Answer 13

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.

Answer 14

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."

Answer 15

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.

Answer 16

pthread_create()

Answer 17

• 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

Answer 18

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.

Answer 19

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.

Answer 20

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.

Answer 21

2-spaces indentation.

Answer 22

• 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()

Answer 23

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.

Answer 24

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

string buffer = ReadFileLine();
// ...
buffer = "Program Success!!!!";

Answer 25

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 */
}

Answer 26

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.

Answer 27

That I'm reviewing someone else's code.

Answer 28

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

Answer 29

query = "select * from table where field like '%" + someTextBox.Text + "%'";

Answer 30

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

Answer 31

Not using const when they obviously should be...

Answer 32

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.

Answer 33

Variable names not declared according to the context .

Answer 34

Dispatcher.BeginInvoke( () => someTextBox.Background = Brushes.Red );

Answer 35

Bad/Poor commenting.

Answer 36

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