Similar to this question...
What are the worst practices you actually found in Java code?
Mine are:
Overkill abstraction of object oriented design.
Same answer on a similar thread (applies to all languages which permit object oriented design).
Not related strictly to Java, but calling an expensive function over and over instead of storing the result, when you know it won't change. Example:
if (expensiveFunction() > aVar)
aVar = expensiveFunction();
for (int i=0; i < expensiveFunction(); ++i)
System.out.println(expensiveFunction());
Similar to yours, but taken a step further:
Use of class (static) variables when a request scoped variable was the correct thing to do in a Struts action. :O
This was actually deployed in production for a few months, and no one ever noticed a thing until I was reviewing the code one day.
Subclassing when you're not supposed to, e.g. instead of using composition, aggregation, etc.
Edit: This is a special case of the hammer.
Abstracting functionality out into a library class which will never be re-used as it's so specific to the original problem being solved. Hence ending up with a gazillion library classes which no-one will ever use and which completely obscure the two useful utilities you actually do have (i.e. CollectionUtils and IOUtils).
...pauses for breath...
I had to maintain java code, where most of the Exception handling was like:
catch( Exception e ) {}
Our intern used static modifier to store currently logged user in Seam application.
class Identity{
...
public static User user;
...
}
class foo{
void bar(){
someEntity.setCreator(Identity.user);
}
}
Of course it worked when he tested it :)
The worst Java practice that encompasses almost all others: Global mutable state.
Ridiculous OO mania with class hierachies 10+ levels deep.
This is where names like DefaultConcreteMutableAbstractWhizzBangImpl come from. Just try debugging that kind of code - you'll be whizzing up and down the class tree for hours.
@madlep Exactly! Parts of the Java community really goes overboard with extreme abstractions and crazily deep class hierarchies. Steve Yegge had a good blog post about it a couple of years back: Execution in the Kingdom of Nouns.
I once had to investigate a web application where ALL state was kept in the web page sent to the client, and no state in the web server.
Scales well though :)
Six really bad examples;
My favorite sorting algorithm, courtesy of the gray beard brigade:
List needsToBeSorted = new List ();
...blah blah blah...
Set sorted = new TreeSet ();
for (int i = 0; i < needsToBeSorted; i++)
sorted.add (needsToBeSorted.get (i));
needsToBeSorted.clear ();
for (Iterator i = sorted.iterator (); i.hasNext ();)
needsToBeSorted.add (i.next ());
Admittedly it worked but eventually I prevailed upon him that perhaps Collections.sort would be a lot easier.
Once I encountered 'singleton' exception:
class Singletons {
public static final MyException myException = new MyException();
}
class Test {
public void doSomething() throws MyException {
throw Singletons.myException;
}
}
Same instance of exception was thrown each time ... with exact same stacktrace, which had nothing to do with real code flow :-(
An API that requires the caller to do:
Foobar f = new Foobar(foobar_id);
f = f.retrieve();
Any of the following would have been better:
Foobar f = Foobar.retrieve(foobar_id);
or
Foobar f = new Foobar(foobar_id); // implicit retrieve
or
Foobar f = new Foobar();
f.retrieve(foobar_id); // but not f = ...
Creating acessors and mutators for all private variables, without stopping to think, sometimes automatically.
Encapsulation was invented for a reason.
A mistake made by junior programmers: unnecessarily using member variables instead of local variables.
A Java EE example:
Starting threads in servlets or EJBs (for example to start asynchronous processing tasks).
This breaks the scalability of your Java EE app. You're not supposed to mess with threading in Java EE components, because the app server is supposed to manage that for you.
We refactored this by having the servlet put a message on a JMS queue and writing a message-driven bean to handle the asynchronous processing task.
Excesive focuse on re-using objects that leads to static things everywhere. (Said re-using can be very helpfull in some situation).
Java has GC build-in, if you need an object, create a new one.
Defining the logic using exceptions where a for-loop or any form of loop would suffice.
Example:
while(i < MAX_VALUE)
{
try
{
while(true)
{
array[j] = //some operation on the array;
j++;
}
}
catch(Exception e)
{
j = 0;
}
}
Serious, I know the guy who wrote this code. I reviewed it and corrected the code :)
I saw this line a couple of minutes ago:
Short result = new Short(new Integer(new Double(d).intValue()).shortValue());
I think this one for me must be a record. A class is used for building a complex data model for the front end involving filtering. so the method that returns the list of objects goes something like this:
public DataObjectList (GenerateList (massive signature involving 14 parameters, three of which are collections and one is a collection of collections)
try {
250 lines of code to retrieve the data which calls a stored proc that parses some of it and basically contains GUI logic
} catch (Exception e) {
return new DataObjectList(e, filterFields);
}
So I got here because I was wondering how come the following calling method was failing and I couldn't see where the Exception field was being set.
DataObjectList dataObjectList= EntireSystemObject.getDataObjectList Generator().generateDataObjectList (viewAsUserCode, processedDataRowHandler, folderQuery, pageNumber, listCount, sortColumns, sortDirections, groupField, filters, firstRun, false, false, resetView);
dataObjectList.setData(processedDataRowHandler.getData());
if (dataObjectList.getErrorException() == null) {
do stuff for GUI, I think, put lots of things into maps ... 250 lines or so
}
return dataObjectList;
} else {
put a blank version into the GUI and then
throw new DWRErrorException("List failed due to list generation error", "list failed due to list generation error for folder: " + folderID + ", column: " + columnID, List.getErrorException(), ListInfo);
}
All with me so far?
Well at least they did tell us in the front end that something did actually go wrong!
AbstractSpringBeanFactoryFactoryFacadeMutatorBeanFactory. I can't stand this over-engineered, incomprehensible BS. Joel put it a bit more elegantly.