Bizarre Java error/bug: Stack Overflows, null pointers and double locking

Question

I really don't know how to explain this error properly...

It started when I added this method to my controller class:

public void loadPlayerComboBox() {
    try{
        final PreparedStatement collectPlayerNames = 
                ConnectionManager.getConnection().prepareStatement("SELECT "+PLAYER_NAME+"  FROM PLAYERS");
        final ResultSet playerNameResults = collectPlayerNames.executeQuery();
        while(playerNameResults.next()){
            IViewManager.Util.getInstance().getMyContainerPane().getMyPlayerManagerPane().getPlayerNameList()
            .add(playerNameResults.getString(PLAYER_NAME));
        }
    }catch(final SQLException e){
        System.out.println("SQLException. Reason: " + e.getMessage());
    }
}

And I've confirmed that without this method (which is called on by a class, on start up of the program) I don't have an error.

My error? When I run from eclipse, the programe starts to load the top bar of the frame appears, but the actual content doesn't.

As well as this there is a strange drowing empty box, that I believe is caused by multiple processes being started up.

This, I beleive was caused by a stack overflow, as my getInstance() methods recursively called each other. I can't get the stacktrace back for some reason, and at the moment the debug screen is just blank. I've checked my .log and it doesn't have an error corresponding to that time stamp. I don't know how they could call each other though. Here is the ViewManager method:

static class Util {
    static private IViewManager viewManager = null;
    static public synchronized IViewManager getInstance() {
            if (viewManager == null) {
                    viewManager = new ViewManager();
            }
            return viewManager;
    }
}

And the constructor:

public ViewManager(){
    super("Tony Larp DB Manager");
    this.setVisible(true);
    this.setDefaultCloseOperation(3);
    myContainerPane = new ContainerPane();
    myContentMenu = new ContentMenu();
    IController.Util.getInstance();
    IPlayerCharacterManager.Util.getInstance();
    this.setJMenuBar(myContentMenu);
    this.getContentPane().add(myContainerPane);
    this.pack();        
}

The IController.Util.getInstance() method is identical, except the names of the class and objects it calls are different. Double checked locking just causes the first instance of IViewManager.getInstance() in my launcher to return a null pointer, this is at the start of the synchronised block.

To clarify, the programme calls IViewManager.Util.getInstance() the first time in the launcher, then it calls IController.Util.getInstance() for the first time in the above constructor. After that all calls to it should just return instances.

What would cause this sort of error? How can I even start to fix it?

Answer 1

Hope this doesn't sound to hard: your code is horribly structrued :-/

I can't give you a solution to your question. Maybe I could if I would see your complete code. But I would like to give you more general help. Maybe then you are able to solve your problem on your own.

You should try to simplify and clean up your structure. Here some suggestions:

If you need some singletons in your UI that's OK in my opinion. Singletons in general may be a "bad smell" but you always have to look careful. In your case you could strip lazy initializations, sychronization, etc.

Simply write

static class Util {
    public static final IViewManager viewManager = new ViewManager();
}

if you really only want one ViewManager in your UI (with sounds reasonable).

Then you don't need to call ... .Util.getInstance() anymore. Simply write Util.viewManager.

You should try to achieve Separation of concerns. Let one class do only one thing. The ViewManager manages views. Nothing else. A view class might be responsible for the layout of a dialog. But a view class should not know how to query a database to fill a combobox. Your view class even has to know the SQL. And your view class also knows the weird way to a MyPlayerManagerPane That are all tasks of other classes.

When you need game or player data, maybe you would like to ask something like a GameRepository? You could intoduce such a class. Then you can give an instance of the GameRepository class to your view and inside your view you could say:

...
ComboBoxModel playersModel = createPlayersModel(gameRepository.getPlayers());
playersCombobox.setModel(playersModel);
...

In this way you get relatively small classes. Such classes are easy to test on their own. And if you get strange behaviour at some point during development you can quickly find the reason.

Hope you get the point ;-)