This one is for the Java experts

Dennis_E

I bumped into this Java code in a project I'm supposed to 'make work correctly'. Every method in the class works like this. (userBase checks if the user with the given email is allowed to execute the action and then fetched stuff out of the database.) How many weird things can you spot? It's not hilarious or something, just weird.

public class Controller
{
//Lots of other methods left out...

public User getUserByID(String email, long id) throws Exception
{
    final ObjectHolder userHolder = new ObjectHolder();
    final AtomicBoolean done = new AtomicBoolean(false);

    userBase.handleAction(email, new GetUserByIDAction(), new GetUserByIDActionExecutionDetails(id), new IActionListener()
    {
        @Override
        public void actionFinished(User user) throws Exception
        {
            synchronized (done)
            {
                userHolder.set(user);
                done.set(true);
                done.notifyAll();
            }
        }

        @Override
        public void exception(Exception exception) throws Exception
        {
            synchronized (done)
            {
                done.set(true);
                done.notifyAll();
                throw exception;
            }
        }
    });
    
    synchronized (done)
    {
        while (!done.get()) {
            done.wait(100);
        }

        if (userHolder.get() == null) {
            throw new UserNotFoundException(id);
        } else {
            return userHolder.get();
        }
    }
}

private class ObjectHolder
{
    private T t;
    
    public ObjectHolder(T t)
    {
        set(t);
    }
    
    public void set(T t)
    {
        this.t = t;
    }
    
    public T get()
    {
        return t;
    }
}

}

Nagy Vilmos

WTE does ObjectHolder do? That I can see no value to. The userBase model is common. You use an execution container to encapsulate permissions etc. The use of an anonymous class is, however, poor work. Why aren't the methods synchronized, instead of filling the method with a synchronise block? Damned weird, but there may be a reason.

Reality is an illusion caused by a lack of alcohol "Nagy, you have won the internets." - Keith Barrow

Fredrik Bornander

I guess ObjectHolder is to make the variable assignable from inside the anonymous code as that can only use final variables.

My Android apps in Google Play; Oakmead Apps

Dennis_E

Yes, i thought that's what it was for. However, I was more puzzled by: - The double synchronization of the AtomicBoolean. It is synchronized in the method itself (waiting for it to become true) AND in the listener. Since the method returns, something is smelly. - The throwing of 2 exceptions (one in the listener and the other in the method)

Nagy Vilmos

I wanted to mention that, the point of an AtomicBoolean is that it doesn't need synchronisation because it's, well, atomic. :-D

Reality is an illusion caused by a lack of alcohol "Nagy, you have won the internets." - Keith Barrow

Brisingr Aerowing

:doh:

brisingr_aerowing@Gryphon-PC $ rake in_the_dough Raking in the dough brisingr_aerowing@Gryphon-PC $ make lots_of_money Making lots_of_money

AlphaDeltaTheta

:doh: WTE the coder has done? AtomicBoolean is already atomic and doesn't need a syncd block

Beauty cannot be defined by abscissas and ordinates; neither are circles and ellipses created by their geometrical formulas.

~ Carl von Clausewitz ~

Source

ExcellentOrg

Believe it or not, this reminds me of one UI programmed in Java that I saw about a year ago. It was demo of a software meant for lawyers. Somewhere in UI, there was was a search window and the user was expected to enter the Primary Key (a 10 digit number), the name of person and part of his phone number.

englebart

Maybe they could not figure out how to make a generic, synchronous handler that returned a User object? This might as well be synchronous since it will block forever anyway. Are there any other threads involved here? Maybe some sort of Action pool? Maybe a similar model is used for full asynch? If it is single threaded, it is equivalent to

User ret = userBase.invokeAction(email,
new GetUserByIDAction(),
new GetUserByIDActionExecutionDetails(id));
if (ret == null)
throw new UserNotFoundException(id);

They had to use synchronized due to wait/notify.

svella

Nagy Vilmos wrote:

I wanted to mention that, the point of an AtomicBoolean is that it doesn't need synchronisation because it's, well, atomic.

The synchronization isn't necessarily for the setting/getting, but for the wait()/notifyAll(). They are using the variable both as a flag and a synchronization object, though it is a little unusual to wait with a relatively short timeout and then loop if you are going to signal the monitor as well. I personally would have used CountDownLatch or CyclicBarrier.