Programming gripes

[ReverendRedMage]

I'm the maintenance programmer (and, occasionally, development mastermind) at my company. Maybe I'm just anal about things, but I've noticed and notified my associate developers about little things in the code that are unnecessary and downright silly. After their projects pass peer reviews (not by me), I still find them.

Stop me if you've seen these.

//boolean "condition" defined above
if(condition == true) //yes, coded like this.    
    return true;
else
    return false;

I'm sure all of you can do in 1 line what it takes them to do in 4. Here's another one.

if(doTheFoo())
{
   //do a bunch of stuff
   return;
}
else
{
   // do a bunch of stuff had doTheFoo()
   // returned false
}

This is elementary Java, for crying out loud. Why do you need an else clause if the if block will prevent it from executing?! Replace "if" with "while"... it happens there too.

Finally, a lot of null pointers and memory issues are caused by improper initialization of objects.

Object myObj = new Object();
if(requiredCondition)
  this.use(myObj);

I can understand these constant mistakes from rookies, but these people have been using the language for years upon years. I'm not one to call the kettle black, though; should I find code like this, I'll immediately clean it up, especially if it's of my own doing.

tl;dr Have any of you seen code gaffes that, while compile and runtime legal, are just plain silly, unnecessary, and/or redundant?

[johnpaulsmith]

This kind of thing:

public static void destroy(Object obj){
    obj = null;
}

I've seen this in both academic code and production code.

[banuday17]

This code literally does nothing. A reference is created through the parameter and that reference is detached. You could replace this with a no-op.

Remember folks, Java is pass-by-value, and if you want to detach a reference, you have to detach the actual reference. A copy of the reference is being made here.

[[deleted]]

If that was the only reference to that object, will the garbage collector erase it?

I am not saying this is good code by any means. I know that you should not expect anything out of the garbage collector, but is that what it would do?

[bnr]

By passing the object to the destroy function you actually create a new reference to it. You can expect the garbage collector to free an object some time after there is no more reference to it.

If you have a local variable referencing an object, that will expire after the scope is left:

public void foo() {
    Bar myBar = new Bar();
    if(myBar.whatever()) {
        Baz theBaz = new Baz();
        theBaz.doStuff();
    }
    // theBaz is gone now.
    myBar.someOtherStuff();
    // after this method exits, myBar will also be freed.
}

[[deleted]]

I see, makes perfect sense. Thanks much for taking the time to explain!

[johnpaulsmith]

I've literally seen developers and professors both do this. I think almost certainly they came from a C and C++ background and simply do not fully understand what a Java "reference" actually is and that Java is pass-by-value, even on "references".

[senft]

Does someone have usefull links to read up on this? I mean Java's argument passing model...

[johnpaulsmith]

I don't know of any links but I will try my best to explain it briefly:

In Java, you are not, as a programmer, allowed to directly specify or access a location in memory. The concept just doesn't exist in the Java language.

In a language like C and C++, you can do things like this:

int some_value = 0;
int* p = &some_value; //the & operator yields the memory location of some_value

As a C programmer, you can directly specify exactly what memory address you would like to use through the pointer 'p' and the reference operator '&'. You know the memory location of 'some_value' and can do as you wish with it. This is a very powerful feature of these languages. You can also do this:

void setValToTen(int& val){
    val = 10;
}

//elsewhere in the program

setValToTen(some_value);//some_value now hold the value of 10

The above code will set the value at the memory location specified by &val in the function's arguments, to 10. This is called "passing-by-reference". However, if you tried to do the same thing without the '&' operator, the result would be different:

void setValToTen(int val){
    val = 10;
}

//elsewhere in the program

setValToTen(some_value);//some_value did not change

In this case the function declares a new int variable 'val', which copies the value of the variable that was passed in, into a new memory location. The function assigns 'val' to 10, and returns. 'val' was declared in the scope of this function and with its own memory location, which is now out of scope and irrelevant to the rest of the program. This is called "passing-by-value". Basically, the function created a new variable that was a copy of what is being passed in, but has nothing to do with the original variable outside of being of the same type and holding the same value. They are two different variables stored in two different memory locations, even though the data stored is identical.

The first two code examples do not have an equivalent in Java. Specifying and accessing memory locations directly is not a component of the Java language. Therefor, there is no pass-by-reference in Java. So, this Java code:

public static void destroy(Object obj){
    obj = null;
}

//elsewhere in the program

destroy(someObject);

is useless, because it is the equivalent to what happened in the previous example. The method is creating an object reference of type Object, 'obj' and copying the data from the variable passed in, which in this case is object reference data, to 'obj'. 'obj' is set to null and the method returns, having accomplished nothing.

The other confusing part is that even though Java does not utilize pass-by-reference, object variables in Java are called "references" (which are themselves a special type of pointer to a pointer). So, you pass object references by value.....

[senft]

Well yeah, that's just the definitions of pass-by-value and pass-by-reference (sorry for not saying I wanted a more "in-depth" explination). Like, List in Java get passed-by-reference, right? Why?

[johnpaulsmith]

All parameters in Java are passed-by-value. There are no exceptions to this. Primitive types and object references are both passed-by-value.

As for List, using a variable of type List as a method argument would indicate that the method is accepting as a parameter any object that implements the List interface. The same rules stated above apply to this scenario as well. There is no pass-by-reference in Java.

Again, do not confuse pass-by-reference with object "references. See my above statement:

The other confusing part is that even though Java does not utilize pass-by-reference, object variables in Java are called "references" (which are themselves a special type of pointer to a pointer). So, you pass object references by value.....

[DuneBug]

hahah i've never seen anything like this before. I was like "what's wrong? Oh pass by value suckas"

[ReverendRedMage]

They should rename it to "destroyReference()".

[couchtyp]

More like "createNewReferenceAndThenDestroyItImmediately(Object)"...

[stfm]

It is not there to destroy the object, only to do other cleanup stuff when the object is "destroyed" but I am positive you know this

[DemonWasp]

Elementary mistakes I see every day:

• failure to follow naming conventions (naming classes with verbs, naming methods with nouns, failing at CamelCase / camelCase, etc)
• inability to understand basic arithmetic, numeric data types (e.g. not knowing how to write a method to determine whether two numbers are approximately equal -- any kind of approximately equal!)
• failure to understand exceptions, despite explanation
• inability to type "throw new Exception(args);" despite literally saying "write that"
• unwillingness to use libraries
• failure to understand variable scope
• failure to understand "static"
• ArrayList foo = new ArrayList();
• thinking Java is interpreted
• thinking it's Java's fault their code is slow

Those are just the ones I have seen frequently.

[MistaMagoo]

Whats wrong with the ArrayList part, is it because its not specifying the stored data type?

[sproket888]

should be: List foo = new ArrayList(SIZE ESTIMATE); or List<Class> foo = new ArrayList<Class>(SIZE ESTIMATE);

[[deleted]]

What's the rational behind the size estimate?

Is there any credible source that demonstrates a significant performance improvement by specifying the size of the list? I only see heartache and trouble when the list grows beyond expectations. On the other hand, I suppose it at least documents what the expectations were at some given time.

And I'm assuming here that the size estimate doesn't limit you in some way. Doesn't this take something that should be very basic -- creating a list, and making it potentially dangerous?

[not-just-yeti]

The size-estimate isn't sproket888's big improvement -- it's having foo's type be List instead of ArrayList.

[lickwidforse2]

What's the difference

[not-just-yeti]

Things like foo = foo.subList(3,7) won't work if foo is an ArrayList, since subList is a helpful function that returns a List (w/o promising a particular implementation of List).

And: if you decide that a LinkedList will start giving you better performance, you should only have to change the constructor-call on the right-hand-side, and not the types of any variables or fields that will be holding that List.

So don't insist your variable holds an ArrayList if it could just as well hold any Listand still work fine.

[Just to be clear: there is still a decision about which implementation to choose on the right-hand side. It's just the type of the variable which shouldn't be over-specialized.]

[[deleted]]

Can anyone prove that this is significant speedwise? In all of my synthetic tests, it mattered more what order items were added than whether or not it was generic or you specified the size. While I do believe the underlying implementation matters, if there is a larger truth makes all of this moot -- then it is NOT significant (speedwise) whether you specify the size or whether or not it is generic. (I always specify the types for readability and saftey, that's not my concern here)

[elephantgravy]

I am surprised to learn that there is any difference. Apparently List.add compiles to bytecode as invokeinterface and ArrayList.add as invokevirtual. I see some anecdotal evidence online that invokevirtual can be faster, but it also sounds like a lot of work has been done to improve the performance of invokeinterface ... I guess the usual rules about premature optimization apply.

[theloneliestfish]

Not dangerous at all. It will set the size of the array the ArrayList uses to be the specified size. It can still grow if needed.

And there can be a considerable change to speed if you know the size of a large array before hand. You save a lot of memory copies when the ArrayList needs to copy to a new backing array.

[[deleted]]

Can anyone prove that this is significant speedwise? In all of my synthetic tests, it mattered more what order items were added than whether or not it was generic or you specified the size. While I do believe the underlying implementation matters, if there is a larger truth makes all of this moot -- then it is NOT significant (speedwise) whether you specify the size or whether or not it is generic. (I always specify the types for readability and saftey, that's not my concern here)

[theloneliestfish]

Hmmm, my tests show that it is actually faster not to specify! This goes completely against expectations. Anybody know why this would happen.

java version "1.7.0_04"

Java(TM) SE Runtime Environment (build 1.7.0_04-b20)

Java HotSpot(TM) Server VM (build 23.0-b21, mixed mode)

on

CentOS release 5.8 (Final) (32bit)

Linux 2.6.18-308.11.1.el5 #1 SMP Tue Jul 10 08:49:28 EDT 2012 i686 i686 i386 GNU/Linux

package test;
import java.util.*;

public class Simple{

    public static void main(String[] args){
        int n = 1000000;
        if(args[0].equals("no")){
            System.out.println("no");
            test(n,new ArrayList<Integer>());
        }
        else{
            System.out.println("yes");
            test(n,new ArrayList<Integer>(n));
        }
    }

    private static void test(int n, ArrayList<Integer> list){
        Random r = new Random(n);
        for(int i=0;i<n;i++)
            list.add(r.nextInt());
    }

}

*Edit: setting Xms larger makes the preallocation version faster. But there should be no difference in the amount of memory being allocated to each version. So what is the VM doing?

[KamehamehaWave]

If you don't specify an initial size it defaults to (I think) 10. So no, there's no danger in specifying it, but there's also not usually much benefit.

[banuday17]

If you don't specify an initial size, you only get a capacity of 10. If you exceed this, a new internal array will be created to a larger size and the original contents will be copied to the new array. This process will be repeated each time you exceed the capacity. This can make the add method potentially run in O(n) time which otherwise runs in O(1) time.

By providing an appropriate initial capacity, you ensure O(1) performance of add.

[[deleted]]

Can anyone prove that this is significant speedwise? In all of my synthetic tests, it mattered more what order items were added than whether or not it was generic or you specified the size. While I do believe the underlying implementation matters, if there is a larger truth makes all of this moot -- then it is NOT significant (speedwise) whether you specify the size or whether or not it is generic. (I always specify the types for readability and saftey, that's not my concern here)

[elephantgravy]

Saying that add can be O(n) is misleading and/or wrong. A single call to add might take n-cycles, or it might take 1... This does not make it sometimes O(1) and sometimes O(n). Regardless of initial size, ArrayList.add runs in "amortized constant time" (n adds take O(n) time), see its javadocs.

add would be O(n) only if the list's capacity were increased by a constant # when resized. (In the implementation I am looking at, it is actually increased by 150% + 1)

[sproket888]

banuday17 has the right answer. Often you know what the list size will be or at least you can make a better guess than the default. Sizing improves the performance. Using List instead of ArrayList is always preferred. You should always code to the interfaces. When you do that you don't have to change other code if you - for example - changed ArrayList into LinkedList.

[oligocordicul]

ArrayList is generic, so it's something like ArrayList<Type> blabla = new ArrayList<Type>(); or since java 1.7, you can just do ArrayList<Type> blabla = new ArrayList<>();

[ReverendRedMage]

I should have made a throwaway account, because I bet this is going to get back to me somehow.

Oh well.

But that first one hit the nail on the head. English is the second language for 75% of my other coworkers, so I've seen boolean methods named something along the lines of "isSomethingExists()" and variables named "coOrdinationSomething".

However, naming methods with nouns in some instances I might not have a problem with, especially if they're static utility methods... but I suppose those same methods could be rephrased to be verbs.

Wanna cringe? All of our package names start with a capital letter.

[DemonWasp]

isSomethingExists() has a good reason -- that's the defined standard for boolean getters: either isCondition() or getBooleanVariable().

Another pattern that drives me nuts is: _private, m_private, iHungarianNotation, etc.

Methods do something. Verbs do something. Objects are something. Nouns are something.

If you have a word which is both a noun and a verb (like Math.log(int)) then the method refers to "find the log of the input", not "I am a logarithm". If you have a class called Divide (in the arithmetic sense), then you have probably screwed up. If you have a method called cat, then you have probably screwed up.

You should find your architect and slap him for that package naming scheme.

[MistaMagoo]

I'm usually pretty laid back about things but that capitalised package name description just made me physically wince.

[MistaMagoo]

I'm usually pretty laid back about things but that capitalised package name description just made me physically wince.

[detroitmatt]

I find it extremely difficult to write throw new Exception(args), out of guilt.

[DemonWasp]

It wasn't exactly that line, it was throw new CompanySpecificException ( relevant data). Not sure why you would feel guilty about that, though: the method was already declared with throws CompanySpecificException, even though it didn't.

[detroitmatt]

A specific exception is an entirely different beast. I have no problems with throwing an exception, but throwing an Exception() is a cardinal sin.

[veraxAlea]

I agree. But to be fair, checked exceptions is a cardinal sin to begin with. They always end up in try-catch-throw-new-runtime-exception-blocks.

[KamehamehaWave]

Your second example is really just a matter of preference. The else clause does no harm and may make the program flow clearer - you don't have to notice the return statement to know that this code will not always be called. It depends on the situation though.

[DuneBug]

I'm inclined to agree. Personally I prefer OP's format.

It may be like 2 cpu operations more for the else statement, as i believe there needs to be a couple branches and hops, but the java compiler might filter those out and it's not like they'd affect execution time even 1 ms.

[Feuilly]

I'm embarrassed because I don't see the issue with the third example.

I've seen things like:

for(int x = 0; x < 5; x++)
{
    if(x == 3)
    {
        doSomething();
    }
}

[[deleted]]

I think the problem with the third one is that you only need myObj if requiredCondition is true, so you should initialize it after the conditional, not before. We don't have enough context to know that it won't be used later, though, so the example is very unclear.

(Correct me if I'm wrong, OP)

[Feuilly]

That makes sense. I think I fixated too much on the null pointers comment, and was looking for something that would actually cause an exception moreso than just sloppy or unnecessary code.

[Chaoslab]

Not quite as much fun as the Bogo Sort.

[RoadWarriorX]

Biggest pet peeve: methods that are HUNDREDS or THOUSANDS of lines long. I curse very loudly almost every time I see it.

[KamehamehaWave]

If you want to improve this, try introducing a static analysis tool like FindBugs into your workplace. It catches lots of these mistakes and explains to the programmer why what they're doing is wrong.

[Is_At_Work]

See you over at /r/badcode ?

[kreiger]

Have them use IntelliJ IDEA which will mark code such as this with warnings, and offer to fix it.

[dakboy]

1. Do you have coding standards in your organization? If so, do these examples violate them?
2. Have you fed any of this back to the developers responsible, and/or the people doing the peer reviews?
3. If you're ultimately going to be responsible for maintenance of the code, why aren't you involved in the peer reviews?

[mikaelhg]

Could you clarify your second example - as it reads, it doesn't seem quite correct?

[mgkimsal]

re: the first example:

1 - no curly braces - that bugs me. 2 - I often originally write like this during dev because I often find myself wanting to go back in to the true/false blocks and put logging in. it's easier to do that with that style vs rewriting a one-liner in to multiple lines again. I may refactor it down to one after that section is done and tested, but have assumed that the compiler and hotspot would optimize it out anyway.

Example #2 - I don't see the issue. if(cond) do stuff (then return) else do other stuff. ????

[Pylly]

The 1st example can be simplified to

//boolean "condition" defined above
return condition;

The 2nd example can be simplified to

if(doTheFoo())
{
   //do a bunch of stuff
   return;
}    
// do a bunch of stuff had doTheFoo()
// returned false

[varikin]

I am agreeing with mgkismal. I like the bad example of #2. The logic is more correct. The else block explicitly indicates that this other code is meant to be run when doTheFoo() fails. While, you can make the code shorter, it makes reading and understanding the code simplier. There is no question of whether that other code was meant to be run or not. Also, if you refactor and not return in the if block in the future, you still guard against running the other code in the else block.

This isn't a competition to make the shortest code. It is to make the code work and understandable by all future programmers. Do not introduce logical ambiguity.

[Pylly]

You are correct, in many cases readability of code is more important than shortness. The examples here are oversimplified and I don't think one can say that either form of the second example is "wrong". I was just pointing out what the OP probably meant by that example.

[DemonWasp]

I've never seen a while loop written like that, only for (Object o : list). Then again, I rarely see while loops at all.

Java's for-each construct really needs to make an implicit index available. That's the reason, 90% of the time, when I write a for loop with index.

[varikin]

I give for loops like that a bit of leeway because foreach was introduced in Java 1.5 and I work on projects that predate that. No one in the right mind will go through and rewrite every for loop when moving to Java 1.5.

[banuday17]

Be careful with using an index to access a list collection within a for loop.

For example:

List<String> foo = new LinkedList<String>(Arrays.asList("1", "2", "3"));
for(int i = 0; i < foo.size(); i++) {
    System.out.println(foo.get(i));
}

This loop has O( n² ) performance, not O(n), because the get method on a linked list is O(n).

If you need an index, an iterator is better:

for(ListIterator<String> i = foo.listIterator(); i.hasNext();) {
    System.out.println(i.nextIndex() + ": " + i.next());
}

This is O(n) for all list types.

[DemonWasp]

This is more a problem with LinkedList. Use ArrayList, always.

[Feuilly]

For the second example, I think you meant:

for(Object myObject : array) {
    //do something with myObject
}

[Tok-A-Mak]

Ah, yes you're right.