Worst Java practice found in your experience?

Question

Similar to this question...

What are the worst practices you actually found in Java code?

Mine are:

using instance variables in servlets (it's not just bad practice but bug, actually)
using Collection implementations like HashMap, and not using the appropriate interfaces
using seemingly cryptic class names like SmsMaker (SmsFactory) or CommEnvironment (CommunicationContext)

Simply using instance variables in servlets is *not* a bug or bad practice. Altering the variables after initialization of a servlet may be a bug (it depends on what those variables represent), and is bad practice, but just using instance variables in servlets is not bad practice. — MetroidFan2002, Oct 26 '08 at 18:22
Using Collection implementations instead of interfaces is also not a bug. It's only a bug if the concrete implementation of the interface is irrelevant to the client. — Steve B., Jun 28 '13 at 13:57

score 96 · Accepted Answer · answered Oct 26 '08 at 17:43

96

I had to maintain java code, where most of the Exception handling was like:

catch( Exception e ) {}

answered Oct 26 '08 at 17:43

asalamon74

6,120
9
46
60

2

People who "throw away" exceptions should be punished! – TM. Oct 26 '08 at 18:27
20

People also do: catch(Exception e){ // will not happen } :) – jb. Oct 26 '08 at 20:08
29

Unfortunately, my current project has done worse: `catch(Throwable th) { logger.log("something went wrong"); }` – Alan Oct 27 '08 at 01:23
5

Hey that's pretty common in C# code as well. – Malik Daud Ahmad Khokhar Oct 28 '08 at 06:09
8

I admit, while using Oracle's JDBC stuff, that I've written catch (SQLException ex) { /* What could possible be thrown here? */ } when closing a connection. – Powerlord Oct 29 '08 at 18:44
Had one of those. Worked well until there was 1200 concurernt users, then things started to break. Urgh. – Thorbjørn Ravn Andersen Aug 02 '09 at 16:15
Sometimes I do just want to ignore an exception. But that is _really_ rare. – Isaac Waller Aug 02 '09 at 18:30
4

Java quite often requires you to catch an exception that can never happen. For example, an IOException reading from a ByteArrayInputStream - assuming you are in control of the creation of said ByteArrayInputStream. – slim Oct 01 '09 at 10:19
2

This is going to open up the whole "checked vs unchecked exceptions" debate! :) – dogbane Oct 01 '09 at 10:24
2

or even worse - `catch (Throwable t) {}` – Chii Oct 01 '09 at 10:47
1

Catching exception silently and not doing anything about it is the worst sin there is. At some point I found one from Java platform. After intensive debugging of course. I can tell you I was not happy about it. – Carlos Oct 25 '09 at 01:21
@Alan Note that all you have to do is to change it to `..went wrong", th);`. – Thorbjørn Ravn Andersen Feb 16 '12 at 12:29
It’s so inconvenient having to put these in by hand, in visual basic you can just put "on error resume next" at the top and then you don't have to worry about errors anymore. *is stuck by righteous lightening* – Richard Tingle Nov 01 '13 at 14:29
If the exception should never happen, do `catch (Throwable t) /* or whatever class you want */ { throw new AssertionError(t); }`. – Demi Aug 09 '15 at 01:21
My personal favourite was when I saw the code `catch (Exception e){ throw new DuplicateKeyException(); }` I spent 2 hours to find my duplicate key in my data, even I looked after our definition for duplication, because I couldn't find anything suspicious. It was a simple NPE which was caught. – CsBalazsHungary Feb 17 '17 at 16:50
Best is: `catch (Exception e) { throw new RuntimeException(); }` – PragmaticProgrammer Sep 09 '22 at 19:17

score 39 · Answer 2 · answered Aug 03 '09 at 06:48

39

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 :-(

answered Aug 03 '09 at 06:48

Peter Štibraný

32,463
16
90
116

2

This came up for consideration recently in a discussion I was part of. (And I grimaced.) :o – Sam Harwell Aug 03 '09 at 06:50
I wonder if that is the rationale for the way .NET exceptions work – finnw Jan 20 '11 at 00:15
@finnw: can you elaborate? I don't know anything about .net exceptions, but now I'm curious what you mean. – Peter Štibraný Jan 20 '11 at 10:27
I was referring to the fact that the stack trace is populated when the exception is thrown instead of when it is constructed. – finnw Jan 20 '11 at 11:01
@finnw: rereading this one year later, how would that work if exception is thrown by multiple different threads at the same time? – Peter Štibraný Jan 11 '12 at 13:54

score 28 · Answer 3 · edited Apr 24 '15 at 21:14

28

Six really bad examples;

Instead of error reporting, just System.exit without warning. e.g.
if(properties.size()>10000) System.exit(0); buried deep in a library.
Using string constants as locks. e.g. synchronized("one") { }.
Locking on a mutable field. e.g. synchronized(object) { object = ...; }.
Initializing static fields in the constructor.
Triggering an exception just to get a stack trace. e.g. try { Integer i = null; i.intValue(); } catch(NullPointerException e) { e.printStackTrace(); }.
Pointless object creation e.g. new Integer(text).intValue() or worse new Integer(0).getClass()

edited Apr 24 '15 at 21:14

Jonas Czech

12,018
6
44
65

answered Aug 02 '09 at 18:08

Peter Lawrey

525,659
79
751
1,130

3

I still don't know what is wrong with having 10000 properties. ;) – Peter Lawrey Aug 05 '09 at 18:35
1

Oh wow, string constants as locks is kind of neat... :) – thenickdude Oct 01 '09 at 13:31
1

I have to admit that when I am debugging sometimes I do throw exceptions to get stacktraces, but I'd never check that in! If I'm feeling silly I'll make an ExpectedException class. – Captain Man Apr 28 '15 at 02:41
1

@CaptainMan it is quick a dirty, like system out println statements. I would think it's fine if you don't release the code this way. – Peter Lawrey Apr 28 '15 at 05:32
http://stackoverflow.com/questions/9622315/locking-on-a-mutable-object-why-is-it-considered-a-bad-practice – user2418306 May 28 '16 at 11:53

score 25 · Answer 4 · answered Oct 29 '08 at 18:33

25

if{
 if{
  if{
   if{
    if{
     if{
      if{
       if{
         ....

answered Oct 29 '08 at 18:33

Steve B.

55,454
12
93
132

23

}}} else { ... – user85421 Aug 03 '09 at 15:03
4

On a related note, using ifs instead of if-elses: if (i==1) {} if (i==2) {} instead of if (i==1) {} else if (i==2) {} – dogbane Oct 01 '09 at 10:35

score 24 · Answer 5 · answered Oct 26 '08 at 16:59

24

I hate it when people create interfaces just for hanging a set of constants on:

public interface InterfaceAntiPattern {
  boolean BAD_IDEA = true;
  int THIS_SUCKS = 1;
}

—Interfaces are for specifying behavioural contracts, not a convenience mechanism for including constants.

answered Oct 26 '08 at 16:59

John Topley

113,588
46
195
237

I guess you like static imports in Java 6? – Thorbjørn Ravn Andersen Aug 02 '09 at 16:15
3

Static imports were introduced in Java 5 and I don't see what they have to do with this point. – John Topley Aug 02 '09 at 16:41
Allows you to import e.g. Math.cos and refer to it just as cos. See http://java.sun.com/j2se/1.5.0/docs/guide/language/static-import.html – Thorbjørn Ravn Andersen Aug 02 '09 at 18:18
Math.cos is not a constant. Math.PI would be a better example. – finnw Aug 03 '09 at 06:54
3

Math is a concrete class not an interface, so Math.PI is not an example of this anti-pattern. – John Topley Aug 03 '09 at 11:11
4

What static imports have to do with this point is that they allow you the convenience of just saying `if (BAD_IDEA)` rather than `if (ConstantsClassPattern.BAD_IDEA)`, without having to extend `InterfaceAntiPattern`. – David Moles Aug 04 '09 at 08:57
This is the constant interface antipattern, and is one of the reasons why enums were added to Java - http://en.wikipedia.org/wiki/Constant_interface – Jesper Oct 01 '09 at 10:53
What is the alternative you propose? 1) Using classes is worse because you need to write `public static final` for each constant (which is unnecessary inside an interface). 2) For constants that need to have specific arbitrary values (say, `int MY_INT_CONSTANT = 5423;`) Java 5 enums could be used, but also require more coding. So, IMO, using an interface is often the best choice. – Rogério Feb 03 '10 at 14:50
@Rogerio I would recommend that you read Item 19 in Effective Java. "If the constants are strongly tied to an existing class or interface, you should add them to that class or interface. If the constants are best viewed as members of an enumerated type, you should export them with an enum type. Otherwise, you should export the constants with a noninstantiable utility class. In summary, **interfaces should be used only to define types. They should not be used to export constants**." – John Topley Feb 03 '10 at 14:57
@John Yes, I am familiar with Item 19: "Use interfaces only to define types". It's one of very few items in that excellent book I disagree with. This comes mostly from past experience, in a project where we had hundreds of interfaces just for constants; almost all of those interfaces were nested inside domain entity classes (and some entities had several such interfaces); adding the constants directly would be terrible, because we would lose the interface name, which was a description for an specific grouping of constants. I agree it's not ideal, but the alternatives (enums/classes) are worse. – Rogério Feb 04 '10 at 15:50
The argumentation in item 19 of Effective Java (pages 98-99) is actually against classes that *implement* a constant interface in order to use its constants. This is not the way I use them. So, in my case there was never a "leak into the class's exported API", simply because I never had other classes implementing a constant interface. In this respect, I fully agree with item 19. – Rogério Feb 04 '10 at 16:01
@Rogerio I understand where you're coming from. Interesting edge case. – John Topley Feb 04 '10 at 16:15

score 20 · Answer 6 · answered Oct 26 '08 at 16:16

20

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

answered Oct 26 '08 at 16:16

Claudiu

224,032
165
485
680

It depends what you mean by "expensive". Martin Fowler, in his book on re-factoring, actually recommends writing code like this as it is easier to refactor. – oxbow_lakes Oct 26 '08 at 17:17
4

how is it easier then using variable with value that was returned by that function? i.e. one line vs X lines... – dusoft Aug 02 '09 at 16:19
3

You have to be totally sure that the result of the method is not changing. If the method changes and therefor the result changes more often then thought while writing the depending method your code breaks without you knowing why. If you want to cache do it in the called expensive method. There you know if the result is changing. – Janusz Aug 02 '09 at 18:29
And Haskell programmers are laughing.... – Mechanical snail Aug 19 '12 at 08:43

Apocalisp · Answer 7 · 2009-08-02T18:27:58.963

18

The worst Java practice that encompasses almost all others: Global mutable state.

edited Aug 02 '09 at 18:27

answered Oct 27 '08 at 01:15

Apocalisp

34,834
8
106
155

3

or any mutable state if you could've used an immutable state – RAY Jan 06 '11 at 06:21

score 18 · Answer 8 · answered Oct 27 '08 at 02:49

18

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.

answered Oct 27 '08 at 02:49

madlep

47,370
7
42
53

score 13 · Answer 9 · edited May 23 '17 at 11:46

13

Subclassing when you're not supposed to, e.g. instead of using composition, aggregation, etc.

Edit: This is a special case of the hammer.

edited May 23 '17 at 11:46

Community

1
1

answered Oct 26 '08 at 17:13

volley

6,651
1
27
28

score 12 · Answer 10 · answered Oct 26 '08 at 18:12

12

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 :)

answered Oct 26 '08 at 18:12

jb.

23,300
18
98
136

5

Interns are there to learn, so I guess it is forgivable. Hopefully he won't do it again! – TM. Oct 26 '08 at 18:26
5

I have done this! My only saving grace is that I was a student, and while the people grading my work didn't catch my mistake, I did figure it out on my next project. – Athena Oct 27 '08 at 01:05

CodeClimber · Answer 11 · 2011-02-14T11:55:41.923

11

saw something like this:

public static boolean isNull(int value) {
    Integer integer = new Integer(value);

    if(integer == null) {
        return true;
    } else {
        return false;
    }
}

They had a similar method for longs.

I presume they had originally done something like

if(value == null) {

and got a compile error and still didn't realise that primitive values couldn't be null.

edited Feb 14 '11 at 11:55

answered Feb 14 '11 at 11:06

CodeClimber

4,584
8
46
55

4

I am deeply ashamed now, but when I started programming, I made a method like this: `public boolean isNull(){ return this == null }` – CsBalazsHungary Jan 08 '16 at 14:47
I dont see case when this method returns true, new is always called. :/ – Spasoje Petronijević Nov 15 '17 at 14:24

score 9 · Answer 12 · answered Aug 02 '09 at 16:12

9

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 :)

answered Aug 02 '09 at 16:12

Thorbjørn Ravn Andersen

73,784
33
194
347

2

For bonus points, also send *user credentials* to the browser. – finnw Aug 03 '09 at 06:58

score 9 · Answer 13 · answered Oct 01 '09 at 10:32

9

Not closing database connections, file handles etc in a finally{}

answered Oct 01 '09 at 10:32

dogbane

266,786
75
396
414

And then try to delete a file while the program is still running... – Saphyra Jun 04 '19 at 14:36

score 8 · Answer 14 · answered Oct 01 '09 at 10:17

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

score 8 · Answer 15 · answered Oct 01 '09 at 10:46

8

Creating acessors and mutators for all private variables, without stopping to think, sometimes automatically.

Encapsulation was invented for a reason.

answered Oct 01 '09 at 10:46

Emilio M Bumachar

2,532
3
26
30

Do you like Lombok? – Saphyra Jun 04 '19 at 14:37
@Saphyra Looks like a tradeoff that can be very good on most cases. It exposes everything, but all that code is implicit, so it costs that much less. No one will pay the price of reading it over and over again. (Had never heard of it, watched the demo now, didn't try it, haven't programmed in Java for years). https://projectlombok.org/ – Emilio M Bumachar Jun 04 '19 at 16:21

score 8 · Answer 16 · answered Oct 26 '08 at 17:21

8

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

answered Oct 26 '08 at 17:21

oxbow_lakes

133,303
56
317
449

Why's this so bad? It may be unnecessary, but it doesn't seem like it *hurts* much. Also it enforces separation of concerns, even if the code will never be reused. – Mechanical snail Aug 19 '12 at 08:45

score 5 · Answer 17 · edited May 23 '17 at 12:09

5

Overkill abstraction of object oriented design (Deleted so 10k only).

Same answer on a similar thread (applies to all languages which permit object oriented design).

edited May 23 '17 at 12:09

Community

1
1

answered Oct 26 '08 at 16:14

Inisheer

20,376
9
50
82

score 5 · Answer 18 · answered Feb 16 '11 at 23:55

Not thinking like a programmer should.

After prolonged exposure, Java does that to some people.

Why? My opinion is that it's because there's too much Intellisense and no sense. It lets you do stupid things so quickly that people don't stop to think.

Example 1:

boolean negate( boolean shouldNegate, boolean value ) {
  return (shouldNegate?(!value):value;
}

which, of course is the same as value ^ shouldNegate, a simple XOR.

Example 2: (I swear I'm not making this up)

boolean isNotNull( Object o ) {
  return o != null;
}

Both with additional 4-6 lines of Javadoc, explaining what those methods did.

Example 3:

/**
*
*
*/

An empty Javadoc, to make those annoying Eclipse "missing Javadoc" warnings go away.

Example 3b:

/**
* A constructor. Takes no parameters and creates a new instance of MyClass.
*/
public MyClass() {
}

score 3 · Answer 19 · answered Aug 03 '09 at 06:38

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.

score 2 · Answer 20 · answered Oct 01 '09 at 13:22

2

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 :)

answered Oct 01 '09 at 13:22

Prabhu R

13,836
21
78
112

magnificent , well found from him – maazza Sep 02 '12 at 08:57

score 2 · Answer 21 · answered Oct 28 '08 at 06:06

2

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

answered Oct 28 '08 at 06:06

albertb

2,874
1
20
16

score 2 · Answer 22 · answered Jul 29 '10 at 08:32

2

I saw this line a couple of minutes ago:

Short result = new Short(new Integer(new Double(d).intValue()).shortValue());

answered Jul 29 '10 at 08:32

cringe

13,401
15
69
102

score 2 · Answer 23 · answered Jan 12 '11 at 20:07

In File I/O: incorrect use of try-catch block.

try {
   /* open file */
}
catch(Exception e) {
  e.printStackTrace();
}

try {
   /* read file content */
}
catch (Exception e) {
  e.printStackTrace();
}

try {
   /* close the file */
}
catch (Exception e) {
  e.printStackTrace();
}

score 1 · Answer 24 · answered Oct 01 '09 at 11:07

1

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.

answered Oct 01 '09 at 11:07

PeterMmm

24,152
13
73
111

score 1 · Answer 25 · answered Oct 26 '08 at 16:24

1

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.

answered Oct 26 '08 at 16:24

Jack Leow

21,945
4
50
55

2

We had a problem like that lead to a situation where only one user could press the 'Submit' button at a time without getting a stack trace. Luckily the users were internal and eventually somebody on the dev team asked the professional services guys why the kept yelling over the cube wall "Fire in the hole!" – David Moles Aug 04 '09 at 09:00

score 1 · Answer 26 · edited Sep 17 '11 at 11:18

1

AbstractSpringBeanFactoryFactoryFacadeMutatorBeanFactory. I can't stand this over-engineered, incomprehensible BS. Benji Smith puts it a bit more elegantly.

edited Sep 17 '11 at 11:18

Marat Salikhov

6,367
4
32
35

answered Oct 26 '10 at 17:31

Yevgeniy Brikman

8,711
6
46
60

score 1 · Answer 27 · answered Jan 12 '11 at 19:50

Here is a cropped sample from an actual applet i was to maintain it took me forever to realize what is was doing.

int sval, eval, stepv, i;
double d;
                if (/*someCondition*/)
                    {
                    sval = 360;//(all values multiplied by 20)
                    eval = -271;
                    stepv = -10;
                    }
                else if (/*someCondition*/)
                    {
                    sval = 320;
                    eval = -601;
                    stepv = -10;
                    }
                else if (/*someCondition*/)
                    {
                    sval = 0;
                    eval = -311;
                    stepv = -10;

                    }
                    else
                    {
                    sval = 360;
                    eval = -601;
                    stepv = -10;
                    }
                for (i = sval; i > eval; i = i + stepv)
                    {
                    d = i;
                    d = d / 20.0;
                    //do some more stuff in loop
                    }

turns out he wanted to iterate by .5 over the loop and thats not a pasting error, that is the indentation scheme

score 1 · Answer 28 · answered Feb 15 '12 at 11:06

1

Huge Class names. I remember: AbstractTableComponentListeningBehaviourPanel.java and other similar monsters.

The worst part is, even though the naming convention was crazy detailed, I still had to pick apart the files to work out their purpose.

answered Feb 15 '12 at 11:06

Jivings

22,834
6
60
101

Jesper · Answer 29 · 2009-10-01T10:56:34.567

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.

score 0 · Answer 30 · answered Oct 26 '10 at 17:02

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!

score 0 · Answer 31 · answered Feb 15 '12 at 10:58

Converting a naked XML message (without XSD/namespaces) into a DataObject:

DataObject operation_file = boFactory....

try{operation_file.setString("file_name", Constants.getTagValue("file_name", eElementOp));}catch (Exception e){operation_file.setString("file_name","");}
try{operation_file.setDate("proposed_execution_date", sdf.parse(Constants.getTagValue("proposed_execution_date", eElementOp)));}catch (Exception e){operation_file.setString("proposed_execution_date",null);}
try{operation_file.setString("instructions", Constants.getTagValue("instructions", eElementOp));}catch (Exception e){operation_file.setString("instructions","");}
try{operation_file.setString("description", Constants.getTagValue("description", eElementOp));}catch (Exception e){operation_file.setString("description","");}

I've called it the try-catch oriented programming (tcop)...

Worst Java practice found in your experience?

31 Answers31

Not thinking like a programmer should.

Linked