(java) interfaces problem.

Syth

Hi, I'm having a problem with interfaces in java. I've never written my own before, and we didn't cover em much when we were learning java last year.

Basically I'm trying to write like a clock so that I can use it in simple game that i'm working on. I've written a clock class that runs in it's own thread, as well as a ClockListener (the interface in question). I wanted the clock to have an interval and every time that interval ran out, it'd call the objects that were listening to the clock, that way the objects could do things on a regular basis, such as move an enemy etc. I wrote an interface that the clock listeners would have to implement, that way i'd know that the method clockInterval() would exist. I wanted the clock to call clockInterval() to tell the object that the clock had 'chimed'.

In writing the clock definition, i just called all of it's listeners Objects (keep it general right), but when I try to call the clockInterval() method while in the Clock, it says that Object doesn't have that method. How can I force all objects that are set to listen to the clock to have to implement ClockListener? When I just write ClockListener[] listeners; it says that the class ClockListener doesn't exist. I'm in new territory here.

The clock interval interface

public interface ClockListener {
	public void clockInterval(); 
}

The clock class.

public class Clock extends Thread {
	//The number of millisencond between the calls to the Clock listeners.
	protected long interval = 1000;

	//Only calls the clock listeners if this is true, if false then no calls are made.
	protected boolean enabled = true;

	//The array that holds the Clock Listeners
	protected Object[] listeners = null;

	//The time of the last time there was an interval
	protected long lastInterval = 0;

	/** some simple getters, setters and constructors. */

	public void addListener(Object o) {
		int numListeners = listeners.length;
		Object[] arr = new Object[numListeners+1];
		
		//Copy the old array onto the new one.
		for (int i=0; i<numListeners; i++) {
			arr[i] = listeners[i];
		}

		//add the new object
		arr[numListeners+1] = o;

		//set listeners to be the new array
		listeners = arr;

		//clean up
		System.gc();
	}
	
	public void run() {
		long currTime = System.currentTimeMillis();
		while (enabled) {
			if (currTime > lastInterval+interval) {
				alertListeners();
			}
		}
	}

	public void alertListeners() {
		for (int i=0; i<listeners.length; i++) {
			if (listeners[i] instanceof ClockListener) {
				listeners[i].clockInterval();
			}
		}
		lastInterval = System.currentTimeMillis();
	}
}

davros

Originally posted by Syth
when I try to call the clockInterval() method while in the Clock, it says that Object doesn't have that method.

Cast it to a ClockListener:

((ClockListener) listeners[i]).clockInterval();

Also, I wonder why you don't use, say, an ArrayList to hold your listeners. Then you wouldn't have to create a new array every time you add a listener. It looks as though the code is trying to second-guess the compiler, even making a call to the garbage collector. Is that really necessary these days with the latest JVMs?

Enygma

When you say ClockListener[] listeners are you saying

ClockListener[] listeners = new ClockListener[numListeners+1];

If that's the case then it's complaining because you can't instanciate (sp?) an interface (i.e. say new Interface())

You could use a Vector to hold all the listeners, when you loop over the Vector just cast them to ClockListener objects and call the clockInterval() method. This will call the clockInterval method of whatever subclass the Listener object implements.
i.e. If it's a TimesUpClockListener then it will call that classes clockInterval, or if its a TenSecondsLeftClockListener then it will call that one.

One thing I hate to see though is code like this:

public void alertListeners() {
	for (int i=0; i<listeners.length; i++) {
		if (listeners[i] instanceof ClockListener) {
			listeners[i].clockInterval();
        	}
	}
	lastInterval = System.currentTimeMillis();
   }

Saying "If the object is of type this, then do this, if it's of type that, then do that" is always a bad idea.
In this case you don't need to use instanceof at all, but next time you do use it, pinch yourself and try to find a cleaner way to do things, there usually is.

Hope this helps.

Enygma

Didn't notice the call to gc().

I wouldn't bother with things like that during development, it mightn't even actually call the garbage collecter when you think it will. It's usually best to concentrate on performance if and when you have performance problems.

At least then you can pinpoint bottlenecks using profilers and concentrate on areas that really need improvement. You could be wasting your time thinking about performance from the start.

davros

Also, I wonder why you don't use, say, an ArrayList to hold your listeners. Then you wouldn't have to create a new array every time you add a listener.

I've been doing some reading and I'll answer my own question now ...

Copying the array is done for thread-safety. You don't want to be iterating over the array in one method and adding an element at the same time in another method. So you make changes on a copy. Forget I brought it up!

Regarding 'instanceof': it's true that 'instanceof' is like a flag telling you that you haven't found the proper object-oriented way of implementing something. But it is a very common part of the event listener pattern being used here. And if it isn't used, then there will be some other check on the class of the object. Otherwise you could be notifying an event to an object that can't receive those events, throwing a runtime error.

Enygma

Ah right, I'm not too familiar with event models to be honest. It's just that instanceof always sets off alarm bells with me.

It still looks, to me, that it's completely unnessecary here though.

GillyS

Hi Davros,

First of all you should change the following code:

public void addListener(Object o)

to:

public void addListener(ClockListener listener)

This will catch the fact you are adding an object which doesn't implement the ClockListener at compile time and not runtime.

Also synchronize both the alertListener and addListener - this will make you code thread safe (you won't need to copy the array anymore). I'd also agree with Enygma that you should use an ArrayList instead of an array and use the Iterator to iterate the objects in the alertListener code.

BTW Always check in addListener that the listener isn't already in the list or you could end up calling it multiple times every time the event is fired.

Hope this helps.

Gilly

davros

Originally posted by GillyS
public void addListener(ClockListener listener)

This will catch the fact you are adding an object which doesn't implement the ClockListener at compile time and not runtime.

You can do that. My point was that in the event notification code, convention always seems to be to add an instanceof (or similar) check.

I don't know why that is. The event model comes from the JavaBeans specification and conforming to that will let other programs hook into your events by reflection. So perhaps it is something to do with that.

GillyS

Davros,

I've never seen an addListener method that didn't specify the interface type so in theory the instanceof is redundant - you are guaranteed everything in the list will at least implement the interface. The only reason I can think where it might be necessary is if you have extended the interface and want to distinguish between the types in your event firing method - but I don't think that would be very good practice.

Gilly

Syth

Cast it to a ClockListener:

Ah yes that seems to please the compiler.

One thing I hate to see though is code like this:

I only did that after i got the first error message about there not being a clockInterval() method in java.lang.Object, so I thought that if i did that, then the compiler would realise that the only time the clockInterval() mathod is called, that the clockInterval() method would exist because the object implements ClockListener. But no such luck. After casting it to ClockListener, I don't need those lines.

Copying the array is done for thread-safety.

Really? I only did it because it was easier, and out of habit.

BTW Always check in addListener that the listener isn't already in the list or you could end up calling it multiple times every time the event is fired.

That's a good idea that I'd totally forgotten about.

Also synchronize both the alertListener and addListener

What do you mean by this? This is my first venture into interfaceland and threadland.

Thanks for all the help guys.

GillyS

What do you mean by this? This is my first venture into interfaceland and threadland

add the synchronize keyword to your method definition:

public synchronized void addListener(ClockListener listener)

It just means that only one thread can execute this method at a time , this prevents a situation where two (or more) threads try to add a listener at the same time - which could cause unpredicatable problems with your code.

Gilly

Syth

Ah that makes sense, this is after all my first adventure into threadland. thanks.