When reviewing somebody else's code, what is it that you usually find most disturbing?When reviewing somebody else's code, what is it that you usually find most disturbing?
Things that seem like evidence of cargo cult programming [1]. 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."
[1] http://en.wikipedia.org/wiki/Cargo%5Fcult%5FprogrammingExtremely 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.
Usually that they can write better code than me.
ACCEPTED]
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.
gg=G in vim). But can you read it without re-formatting? - soulmerge
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
query = "select * from table where field like '%" + someTextBox.Text + "%'";
Lack of comments in the code yet detailed explanations in the review email.
Using the same variable for different purposes (a common habit of electrical engineers that write SW)
string buffer = ReadFileLine();
// ...
buffer = "Program Success!!!!";
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.
The thing that disturbs me the most is finding code that was clearly never tested.
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:
Long methods that do a million different things.
If statements comprised entirely of:
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
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.
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...
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.
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
You might be interested in reading the "code smells" question [1] - it goes over a number of common "code smells" (indicators of bad code) and how to correct them.
[1] http://stackoverflow.com/questions/114342/what-are-code-smells-what-is-the-best-way-to-correct-themNo 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()
That I'm reviewing someone else's code.
Code that's obviously been copied from a blog somewhere and pasted into production code.
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.
Not using const when they obviously should be...
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.
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.
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.
2-spaces indentation.
Not to repeat everyone else but variables and functions named in a foreign language confuse [and disturb] me.
Variable names not declared according to the context .
Dispatcher.BeginInvoke( () => someTextBox.Background = Brushes.Red );
Bad/Poor commenting.
When I mark a section as needing a total rewrite, and then realize it's something that I wrote years before.
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.
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 */
}
pthread_create()
Bugs; especially any timing-related, intermittent bugs.
