HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Java Array Traversal Optimization

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
arrayoptimizationjavatraversal

Problem

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 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].s

Solution

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();
    }

Code Snippets

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);
  }
}
ufos[i].alien.setColor(rgen.nextColor());
ufos[i].setPilotColor(rgen.nextColor());
if(ufos[i].getX() > WINDOWX){
  winner = i;
  flag = false;
}
if(ufos[i].crossedFinishLine( WINDOWX ) ){
  winner = i;
  flag = false;
}

Context

StackExchange Code Review Q#26714, answer score: 5

Revisions (0)

No revisions yet.