Images and OO

[Andrew]

Hello,

I am writing a very simple programs which displays a different coloured
target depending on what colour has been selected (using a combo box).

I have an abstract class called Target and three classes called GreenTarget,
RedTarget and YellowTarget which all extend Target.
Each subclass has a method called getColourIndex which returns an integer
relating to the correct coloured target to load in the images array.

To select the colour and return the correct subclass, I have used the
following code.

class Test extends Panel implements ActionListener
{
String colour;
Choice ch = new Choice();
Target t;
int colourIndex;
Button display = new Button("Display");
Image[] images = new Image[3];

init()
{
// Load Images

ch.add("Red");
ch.add("Yellow");
ch.add("Green");

add(ch);
add(display);
}
public void actionPerformed(ActionEvent evt)
{
colour = ch.getSelectedItem();

if(colour.equals("Red"))
t = new RedTarget();
if(colour.equals("Yellow"))
t = new YellowTarget();
if(colour.equals("Green"))
t = new GreenTarget();

colourIndex = t.getColourIndex();
repaint();
}
public void paint(Graphics g)
{
g.drawImage(images[colourIndex],0,0,this);
}
}

How could this code be better? How can I rearrange the code so that I am not
asking for raw data (the image indexes) from the subclasses?
Should I pass the Graphics object in the paint() method to one of the
subclasses?
Also, should the images be loaded in the init() method or should each
subclass load their own image?

I hope someone can help because I am always having this problem.
I have read the MVC pattern but I am finding it hard to understand.

 

[Oliver Wong]

I am writing a very simple programs which displays a different coloured
target depending on what colour has been selected (using a combo box).

I have an abstract class called Target and three classes called
GreenTarget,
RedTarget and YellowTarget which all extend Target.
Each subclass has a method called getColourIndex which returns an integer
relating to the correct coloured target to load in the images array.

To select the colour and return the correct subclass, I have used the
following code.

class Test extends Panel implements ActionListener
{
String colour;
Choice ch = new Choice();
Target t;
int colourIndex;
Button display = new Button("Display");
Image[] images = new Image[3];

init()
{
// Load Images

ch.add("Red");
ch.add("Yellow");
ch.add("Green");

add(ch);
add(display);
}
public void actionPerformed(ActionEvent evt)
{
colour = ch.getSelectedItem();

if(colour.equals("Red"))
t = new RedTarget();
if(colour.equals("Yellow"))
t = new YellowTarget();
if(colour.equals("Green"))
t = new GreenTarget();

colourIndex = t.getColourIndex();
repaint();
}
public void paint(Graphics g)
{
g.drawImage(images[colourIndex],0,0,this);
}
}

How could this code be better? How can I rearrange the code so that I am
not
asking for raw data (the image indexes) from the subclasses?
Should I pass the Graphics object in the paint() method to one of the
subclasses?
Also, should the images be loaded in the init() method or should each
subclass load their own image?

I hope someone can help because I am always having this problem.
I have read the MVC pattern but I am finding it hard to understand.

It seems to me that the model can be in one of three states, and one of
the view for these states is to represent one of three images, depending on
which state the model is in. So if I wanted to go with an MVC approach, I'd
probably do something like (untested pseudo code):

public class Model {
public static enum State {
Good("Green"), Warning("Yellow"), Error("Red");
State(String label) {
this.label = label;
}
final private String label;
public String getDescriptiveLabel() {
return label;
}
}

public static interface ModelListener {
public void stateChanged(State newState);
}

private State state;

public Model() {
state = State.Good; /*Or whatever the initial state is.*/
}

final List<ModelListener> listeners = new LinkedList();

public void addListener(ModelListener ml) {
listeners.add(ml);
}

public void removeListener(ModelListener ml) {
listeners.remove(ml);
}

public State getState() {
return state;
}

public void setState(State s) {
this.state = s;
for (listener : listeners) {
listener.stateChanged(this.state);
}
}
}

class Test extends Panel implements ActionListener, ModelListener {
final Choice ch = new Choice();
final Button display = new Button("Display");
final Image goodImage, warningImage, errorImage;
final Model model;

public Test(Model m) {
this.model = m;

}

init() {
// Load Images
for (State s : Model.State.values()) {
ch.add(s.getDescriptiveLabel());
}
add(ch);
add(display);
}

public void actionPerformed(ActionEvent evt) {
String colour = ch.getSelectedItem();
if(colour.equals(Model.State.Error.getDescriptiveLabel())) {
m.setState(Model.State.Error);
}
if(colour.equals(Model.State.Warning.getDescriptiveLabel())) {
m.setState(Model.State.Warning);
}
if(colour.equals(Model.State.Good.getDescriptiveLabel())) {
m.setState(Model.State.Good);
}
}

public void paint(Graphics g) {
switch (model.getState()) {
case Model.State.Good:
g.drawImage(goodImage,0,0,this);
break;
case Model.State.Warning:
g.drawImage(warningImage,0,0,this);
break;
case Model.State.Error:
g.drawImage(errorImage,0,0,this);
break;
}
}
}

There's a bit of clumsiness in actionPerformed because Choice only
allows strings as items. If you could use Swing instead of AWT, I recommend
you use JComboBox instead, and so rather than adding Strings to the
JComboBox, you add the State objects themselves. You'd have to add a
toString() method to State which returns the descriptive label, or provide a
custom renderer to explain to the JComboBox how to render each possible
choice.

- Oliver

 

[Oliver Wong]

So if I wanted to go with an MVC approach, I'd probably do something like
(untested pseudo code):

[a lot of the code snipped]

public static interface ModelListener {
public void stateChanged(State newState);
} [...]
class Test extends Panel implements ActionListener, ModelListener {

[...]
/*I forgot to implement the stateChanged method.*/
public void stateChanged(State newState) {
/*Ignore newState, and request a repaint. The paint method will requery
the state anyway.*/
repaint();
}

}

You may or may not need to make sure the call to repaint() occurs in the
Swing Event Dispatch Thread. This is one area I'm weak in, and I'm too lazy
right now to go through the documentation and find out whether or not
repaint belongs in the EDT.

- Oliver