Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

For a homework assignment we have to write a class that simulates water breaking a floodbank (very simplistic 2D simulation).

My implementation works fine but I found that there were many downcasts (IMO a flaw in Greenfoot’s API, which should be using generic functions rather than functions that take classes as arguments, but I’m not sure if this is possible in Java since this is the first time I’m writing Java code).

Sandstone and Sand both derive from Actor, which is from Greenfoot.

import greenfoot.*;

public class Water extends Actor {
    public Water() {
        getImage().scale(64, 64);
    }

    public void act() {
        if (((DoorbraakWorld)getWorld()).stopped) return;

        if (Math.random() < 0.9) return;

        int dx = 0, dy = 0;
        switch ((int)(3.0 * Math.random())) {
            case 0: dx = 0; dy = 1; break;
            case 1: dx = 1; dy = 0; break;
            case 2: dx = -1; dy = 0; break;
        }

        // The next line looks so incredibly ugly…
        Sandstone sandstone = (Sandstone)getOneObjectAtOffset(dx, dy, Sandstone.class);
        if (sandstone != null) {
            getWorld().removeObject(sandstone);
            getWorld().addObject(new Water(), getX() + dx, getY() + dy);
        }

        Sand sand = (Sand)getOneObjectAtOffset(0, 1, Sand.class);
        if (sand != null) {
            ((DoorbraakWorld)getWorld()).stopped = true;
            return;
        }
    }
}

Could I reduce the number of downcasts or is it normal for Java code to contain many downcasts?

What else could I improve?

share|improve this question
add comment

2 Answers 2

up vote 3 down vote accepted

Downcasting can usually be avoided by using common methods in abstract types.

    if (((DoorbraakWorld)getWorld()).stopped) return;

Is this the same as getWorld().stopped()?

    // The next line looks so incredibly ugly…
    Sandstone sandstone = (Sandstone)getOneObjectAtOffset(dx, dy, Sandstone.class);
    if (sandstone != null) {
        getWorld().removeObject(sandstone);
        getWorld().addObject(new Water(), getX() + dx, getY() + dy);
    }

You can use the Actor type: Actor sandstone = getOneObjectAtOffset(dx, dy, Sandstone.class)

    Sand sand = (Sand)getOneObjectAtOffset(0, 1, Sand.class);

Here too: Actor sand = getOneObjectAtOffset(0, 1, Sand.class);

    ((DoorbraakWorld)getWorld()).stopped = true;

How about greenfoot.stop()?

share|improve this answer
add comment

I suggest you just try removing the casts (one at a time) and seeing if the code still compiles.

  • If it does, you've answered your question.
  • If it does not, there is probably not much you can do about it ... especially if the root cause is poor API design in a 3rd-party library.

(If getOneObjectAtOffset was declared as a generic function, then its declared return type could be the type of the Class object, and a downcast wouldn't be necessary. However: 1) the chances are that you would still require the Class parameter, and 2) there would still be a type cast happening behind the scenes when the result is assigned.)

However, there is one place where the cast absolutely cannot be removed:

switch ((int)(3.0 * Math.random())) {

Without the (int) cast, the switch would be switching on a double value, and that is not legal in Java. (But I guess, you weren't referring to that cast ... because it is arguably not a "downcast" in the conventional sense.)

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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