3
\$\begingroup\$

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);
    }
}
\$\endgroup\$
5
\$\begingroup\$

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();
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ UFO doesn't have a pause method. \$\endgroup\$ – abuzittin gillifirca May 29 '13 at 8:45
  • \$\begingroup\$ Thanks for the pause reference but UFO does have access to the pause method since it's brought in with the import statement at the top. Also great pointers! I read all your links in the answer and wanted to ask if you could elaborate a bit on the while(true) part and how I can apply the concept from c++(as in your link) to JAVA. \$\endgroup\$ – Jonny Sooter May 29 '13 at 20:55
  • \$\begingroup\$ Replace while(true) with a method that returns true or false depending on whether the loop should continue; all loops should terminate at some point. In the code shown, if gameOver() returns true, the winning UFO will blink. If gameOver() returns false (because the user has elected to play again or quit the program), then the UFO will stop blinking. \$\endgroup\$ – Dave Jarvis May 29 '13 at 20:59
  • \$\begingroup\$ I get it. It's basically the same as I did with the main race loop where I put a flag in right? Are there any other tips/pointers you would like to add? Also when do I have to use the this keyword? Like in your first point you just invoke setVisible() without this. Is there any case that I would have to use this? \$\endgroup\$ – Jonny Sooter May 29 '13 at 21:06
  • \$\begingroup\$ Exactly. (Aside, flag could use a more meaningful name.) For this, see: stackoverflow.com/questions/2411270/…. Some advice: ask questions, be curious, code a lot, read the Pragmatic Programmer (pragprog.com/the-pragmatic-programmer), and devour anything by Martin Fowler (martinfowler.com). \$\endgroup\$ – Dave Jarvis May 29 '13 at 22:03
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.