Java Array Traversal Optimization

Question

I'm currently learning JAVA in a class and we're using the ACM Graphics package. I wanted some pointers on my code. To provide some context here is the assignment:

Write a program that creates an array of 5 custom objects (cows, UFOs, cars, etc.) and then lines them up along the left edge of the window. Assign each object a random velocity, and then move the objects to the right edge of the window. When one of the objects (the "winner") reaches the right edge of the window, remove the other objects and cause the winner to flash on and off.

This works just fine and fulfills all the requirements from the assignment. What I want to know is how can I improve this code while still remaining within the requirements. Also any reading materials/videos that you think would be useful for my learning please feel free to share them.

Main Race file:

import acm.graphics.*;
import acm.program.*;
import acm.util.RandomGenerator;
import java.awt.Color;

public class Race extends GraphicsProgram {

    //Setting up variables
    private static final int WAIT = 20;
    private static final int WINDOWX = 400;
    private static final int WINDOWY = 600;
    private static final int UFO_COUNT = 5;
    private UFO[] ufos = new UFO[UFO_COUNT];
    private boolean flag = true;
    private RandomGenerator rgen = new RandomGenerator();

    public void init() {
        setSize(WINDOWX, WINDOWY);
    }

    public void run() {
        //Set up UFOs at start
        for (int i=0; i<UFO_COUNT; i++) {
            ufos[i] = new UFO(0, i*100);
            ufos[i].alien.setColor(rgen.nextColor());
            add(ufos[i]);
        }

        int winner = -1;
        waitForClick();

        while(flag) {
            pause(WAIT);

            //Move Ufos
            for (int i=0; i<ufos.length; i++ ) {
                ufos[i].move(rgen.nextInt(0, 2), 0);

                //Is there a winner?
                if(ufos[i].getX() > WINDOWX){
                    winner = i;
                    flag = false;
                }
            }
        }

        if(!flag) //Safeguard
            displayWinner(winner);
    }

    private void displayWinner(int winner) {
        //Hide others
        for (int i=0; i<ufos.length; i++ ) {
            ufos[i].setVisible(false);
        }

        //Make the winner flash
        while(true) {
            ufos[winner].setVisible(false);
            pause(200);
            ufos[winner].setVisible(true);
            pause(200);
        }
    }
}

UFO Constructor Class:

import acm.graphics.*;
import java.awt.*;

public class UFO extends GCompound {    
    public GOval body, bubble, alien;

    public UFO(int x, int y){
        //Draw ufo
        body = new GOval(100, 50);
        body.setFilled(true);
        body.setColor(Color.RED);

        bubble = new GOval(50, 50);
        alien = new GOval(20, 20);
        alien.setFilled(true);
        alien.setColor(Color.GREEN);

        add(bubble, x+26, y+0);
        add(alien, x+40, y+10);
        add(body, x+0, y+26);
    }
}

Answer 1

Encapsulation

An object's behaviour should be with the object. This means that the UFO should control how it flashes (or blinks):

public class UFO ...
  public void blink() {
    setVisible(false);
    pause(200);
    setVisible(true);
    pause(200);
  }

  /** You will have to implement and test this method. */
  private void pause( int milliseconds ) {
    Thread.sleep( milliseconds);
  }
}

Information Hiding

The following code exposes the internals of how a UFO is piloted:

ufos[i].alien.setColor(rgen.nextColor());

Rather, this should be exposed as an accessor:

ufos[i].setPilotColor(rgen.nextColor());

Similarly, exposing the UFO's location is not necessary to determine the winner:

if(ufos[i].getX() > WINDOWX){
  winner = i;
  flag = false;
}

Rather, the above code is more clearly stated as:

if(ufos[i].crossedFinishLine( WINDOWX ) ){
  winner = i;
  flag = false;
}

Only the UFO need know if it has crossed the finish line. The code is now self-documenting.

Make the following attributes private:

public GOval body, bubble, alien;

Otherwise your code will be susceptible to race conditions (no pun intended) and be difficult to debug. This is because the UFO class has no control over when its data changes.

While True

In my opinion, while( true ) is poor form. Instead, add a condition that allows the loop to exit:

    while( gameOver() ) {
        ufos[winner].blink();
    }

Answer 2

Overall, it looks pretty straightforward.

One thing I would recommend is breaking some of the functions down to smaller functions. Several of the comments in the code scream "move me into a separate function"! Take displayWinner() for example. This function does two things, clearly indicated by the comments. Extract those two blocks out into separate functions and call them from displayWinner() instead. If the functions are named appropriately, this will also eliminate the need for the comments.

Same thing for the run() method.