Archive

Archive for December, 2009

Anti-Patterns once again

December 9, 2009 Leave a comment

I just stumbled over another list of interesting Java Anti-Patterns. Luckily I agree with all of them, so unlike the last time, I can just link them here.

Advertisements
Categories: Programming Tags: ,

Java Anti-Patterns revisited

December 8, 2009 5 comments

Today I’ve stumbled over an interesting Homepage that contains a section about Java Anti-Patterns. It’s really well written and funny to read. Unfortunately it also contains some advises that I cannot agree with at all:

Lists are overrated: I would say quite the opposite: Prefer lists to arrays unless you have a very good reason not to. Arrays might perform considerably better in some cases, but they are far more easily used incorrectly (because of a major design flaw) and don’t mix well with generics. Also, programming against the List interface is more convenient and adds an additional layer of abstraction that allows you to change the actual List implementation easily. EffJava2 dedicates Item 25 to this topic alone.

Hashtable, HashMap and HashSet are overrated: I cannot agree here too: Yes, the entry wrappers are bigger than one might initially expect as they contain an additional forward reference and a cache for the hash code. And yes, HashSet is in fact implemented using a HashMap. I have to admit that this made me a bit curious, so I looked at the corresponding implementations in libstdc++ for unordered_map and unordered_set. What I found out is that they are using basically the same entry wrappers, although they let you decide with a C++ template parameter if the hash code is cached, which is false by default. Here is a comment found in one of the related headers:

// Nodes, used to wrap elements stored in the hash table. A policy
// template parameter of class template _Hashtable controls whether
// nodes also store a hash code. In some cases (e.g. strings) this
// may be a performance win.

To be fair however, it should be noted, that the C++ implementation does not waste a pointer if caching is disabled. Also, although they too use a shared implementation for unordered_map and unordered_set, C++ templates allows them to do this without wasting any space for sets, or doing lots of casting (which would be unavoidable if one tried the same approach in Java). To cut a long story short: The implementations that are shipped with the Java standard library aren’t that bad at all, they just reflect the fact that Java isn’t C++, which features a turing complete, code generating metalanguage via the template mechanism, that looks like Java generics only on the surface. Should your analysis reveal that the Java implementations of HashSet and HashMap generally (not only in a few hotspots) waste too much space, then most likely Java isn’t the right tool for your task at all. Otherwise just use HashMap and HashSet and don’t care about the memory overhead, which isn’t that bad as long as you don’t store small objects in these containers. Using exotic Map implementations like IdentiyHashMap is the right thing to do only in rare occasions, because IdentiyHashMap intentionally violates Map‘s general contract which is normally not what you want.

Not properly propagating the exception: Here the author claims that you may loose information if you do

catch(SomeException e)
{
    throw new RuntimeException(e);
}

instead of

catch(SomeException e)
{
    throw new RuntimeException(e.getMessage(), e);
}

which is true in theory for the detail message of the constructed RuntimeException, but should happen in practice only if SomeException overrides Throwable.toString() or Throwable.getLocalizedMessage() in a rather obscure way. I’m not aware of any exceptions that do this (and if I was I would rather fix them), but maybe I missed something, so please correct me if I’m wrong.

Too much static: In this item the author makes an interesting point about storing loggers in static fields prevents unused classes from being garbage collected. This sounds perfectly reasonable and caused me some headaches as I always store loggers in static fields. Some research however revealed, that it is not true. Let me cite the relevant part from the Java Language Specification:

A class or interface may be unloaded if and only if its defining class loader may be reclaimed by the garbage collector as discussed in §12.6. Classes and interfaces loaded by the bootstrap loader may not be unloaded.

So abandoning static loggers won’t change anything as far as class unloading is concerned. Storing loggers in normal instance fields on the other hand side can be annoying in entity beans and can cause screwups with serialization, as correctly pointed out in The transient trap. So my advice here is: Just store your loggers in private static final fields, and be done with it. Should you ever have the need to access the logger of a superclass, you can still obtain it explicitly by something like

Logger log = LoggerFactory.getLogger(getClass().getSuperclass());

although I don’t know any usecase for this other than someone deliberately obfuscating the source of the log messages.

Categories: Programming Tags: , ,

Series about Java Concurrency – Pt. 2

December 6, 2009 2 comments

This post is a follow up to Series about Java Concurrency – Pt. 1, so be sure to read it before this one.

But now let us talk about the puzzle, shall we? Just by reading the source code it seems very reasonable that the program should just print out “[0, 1, 2, 3]”, as EntityManager is obviously thread save, and is lazily initialized in a synchronized block. However, if you run this program multiple times, you will most likely see it printing something different, like “[0]”. In fact, all one can say for sure about this program, is that it prints out a map that contains at least one element, and at most four elements from the set {0, 1, 2, 3}. Everything else depends on the mood of the JVM, the operation system, and the underlying hardware. So what’s wrong? The problem is, that if multiple threads access a shared resource, this resource has to be safely published unless it is immutable. One way to do so, is through a synchronized block where both writes and reads are guarded by the same object. The program above fails to do so here

            synchronized(this)

because this is a different object in every thread. Indeed, replacing the code above with

            synchronized(AllTheSameButDifferent.class)

makes our shiny little program print “[0, 1, 2, 3]” reliably. Still, locking on AllTheSameButDifferent.class might not be the best idea, as another, completely unrelated thread may acquire it too. If this thread happens to perform some lengthy computations while holding the lock, our program would be slowed down needlessly. So, locking on entityManager seems to be a better approach, but it’s still not ideal, as future versions of EntityManager (imagine for the moment that we are dealing with a library class) might use the same lock internally, which brings us down to basically the same reasoning as before. Thus I would rather recommend using

            synchronized(Action.class)

or even

            synchronized(entityMangerLock)

where entityMangerLock is defined in Action like this

private static final Object entityMangerLock = new Object();

Still, while all this reasoning about choosing the appropriate object for locking might very well be the right thing to do in another example, our problem has a solution that is much more elegant, as it makes the JVM to do everything for us. It relies on the fact, that classes are not initialized before they are used, and that class initialization is performed in a thread save manner automagically. You can find more about this in EffJava2 Item 71 as well as in JaCoP1. Here is how it looks like:

package at.lnet.puzzle;

import java.util.Collections;
import java.util.Set;
import java.util.TreeSet;

public class AlwaysTheSameAndLazilyInitialized
{
    private static class EntityManager
    {
        private final Set<Integer> data = Collections.synchronizedSet(new TreeSet<Integer>());
        
        public void persist(final int value)
        {
            data.add(value);
        }
        
        public Set<Integer> getData()
        {
            return Collections.unmodifiableSet(data);
        }
    }
    
    private static class Holder
    {
        private static final EntityManager entityManager = new EntityManager();
    }
    
    private static EntityManager getEntityManager()
    {
        return Holder.entityManager;
    }
    
    private static class Action implements Runnable
    {
        private final int data;
        
        public Action(final int data)
        {
            this.data = data;
        }
        
        @Override
        public void run()
        {
            getEntityManager().persist(data);
        }
    }
    
    public static void main(final String[] args) throws InterruptedException
    {
        final int NTHREADS = 4;
        Thread[] threads = new Thread[NTHREADS];
        
        for(int i = 0; i != NTHREADS; ++i)
            threads[i] = new Thread(new Action(i));
        for(int i = 0; i != NTHREADS; ++i)
            threads[i].start();
        for(int i = 0; i != NTHREADS; ++i)
            threads[i].join();
        
        System.out.println(getEntityManager().getData());
    }
}

Note that this version is only 3 lines longer than the original puzzle, which is exactly the length of the convenience method

private static EntityManager getEntityManager()
{
    return Holder.entityManager;
}

that we could have omitted in theory. If you don’t believe that entityManager is in fact still lazily initialized, step through the program with a debugger.

Categories: Programming Tags: , ,