Collision detection on Android

Question

This is the same game code from my previous question. I split the code into smaller "logical units". The runnable code "Moon Buggy" is available in beta from the google playstore.

It was previously just a few classes, new I am aiming at producing a dozen classes because the code has become large and need to get split up into smaller units.

The base class MoonSprite.java for sprites i.e. moving objects on the screen. The code is self-explanatory, but there might be too long methods and code in the wrong place.

import android.content.Context;
import android.graphics.Bitmap;
import android.graphics.BitmapFactory;

// This does not extend Bitmap because we might want to extend our own class instead
public class MoonSprite {

    public int screenWidth;
    public int screenHeight;
    private int id;
    private int width;
    private int height;

    private Bitmap bitmap;

    public MoonSprite() {

    }

    public MoonSprite(Context context, String name) {
        setId(context.getResources().getIdentifier("object3_hdpi",
                "drawable", context.getPackageName()));
        setBitmap(BitmapFactory.decodeResource(context.getResources(), this.getId()));
    }

    public int getId() {
        return id;
    }

    public void setId(int id) {
        this.id = id;
    }

    public Bitmap getBitmap() {
        return bitmap;
    }

    public void setBitmap(Bitmap bitmap) {
        this.bitmap = bitmap;
    }

    public int getWidth() {
        if (this.width <= 0) {
            return bitmap.getWidth();
        }
        else {
            return this.width;
        }
    }

    public void setWidth(int width) {
        this.width = width;
    }

    public int getHeight() {
        return height;
    }

    public void setHeight(int height) {
        this.height = height;
    }

}

I know that "object3_hdpi" is not a very good name for an object for several reasons and I am going to change that. Otherwise I think that the class above contains logically data that should be together (cohesion) and it has interfaces to communicate with its surroundings (coupling). And it is easy to understand.

But maybe I should put collision detection already in this class? Because I have put collision detection in lower sub-classes maybe duplicating code.

The following is a sub-class of MoonSprite: EnemyTank.java

import android.content.Context;
import android.graphics.Bitmap;
import android.graphics.BitmapFactory;
import android.graphics.Canvas;
import android.graphics.Paint;
import android.os.Handler;
import android.os.Looper;

import java.util.ArrayList;
import java.util.List;

import static dev.android.jamie.MoonUtils.randomize;

public class EnemyTank extends MoonSprite {
    private ArrayList<Missile> missiles;

    private int tankY = 0;

    public int getTankY() {
        return tankY;
    }

    public void setTankY(int tankY) {
        this.tankY = tankY;
    }

    public int getTankX() {
        return tankX;
    }

    public void setTankX(int tankX) {
        this.tankX = tankX;
    }

    private int tankX = 0;

    public Bitmap getBitmapRover() {
        return bitmapRover;
    }

    private Bitmap bitmapRover, explode;

    public int getJumpHeight() {
        return jumpHeight;
    }

    public void setJumpHeight(int jumpHeight) {
        this.jumpHeight = jumpHeight;
    }

    private int jumpHeight;

    private double retardation = 0.5;

    private double buggyXdistance = 0;

    public double getBuggyXdistance() {
        return buggyXdistance;
    }

    public void setBuggyXdistance(double buggyXdistance) {
        this.buggyXdistance = buggyXdistance;
    }

    public void increaseBuggyXdistance(double d) {
        buggyXdistance = buggyXdistance + d;
    }

    public void decreaseBuggyXdistance(double d) {
        buggyXdistance = buggyXdistance - d;
    }

    public double getRetardation() {
        return retardation;
    }

    public void increaseRetardation(double d) {
        retardation = retardation + d;
    }

    public void setRetardation(double retardation) {
        this.retardation = retardation;
    }


    public double getDistanceDelta() {
        return distanceDelta;
    }

    public void setDistanceDelta(double distanceDelta) {
        this.distanceDelta = distanceDelta;
    }

    private double distanceDelta;
    private Bitmap tankMissile;

    protected EnemyTank(Context context, String name) {
        super(context, name);
        missiles = new ArrayList<>();
        this.screenHeight = context.getResources().getDisplayMetrics().heightPixels;
        this.screenWidth = context.getResources().getDisplayMetrics().widthPixels;

        int roverId = context.getResources().getIdentifier(name,
                "drawable", context.getPackageName());
        bitmapRover = BitmapFactory.decodeResource(context.getResources(), roverId);

        int explodeId = context.getResources().getIdentifier("explode",
                "drawable", context.getPackageName());
        explode = BitmapFactory.decodeResource(context.getResources(), explodeId);
        int ufoMissileId = context.getResources().getIdentifier("missile_right",
                "drawable", context.getPackageName());
        tankMissile = BitmapFactory.decodeResource(context.getResources(), ufoMissileId);

        Handler handler = new Handler(Looper.getMainLooper());
        handler.postDelayed(new Runnable() {
            @Override
            public void run() {
            }
        }, randomize(20000, 18000));
    }

    //if tank was hit by UFO missile then return true, false otherwise
    //not implemented yet
    public boolean isHit(UFO ufo) {
        boolean isHit = false;
        return isHit;
    }


    //if tank was hit by UFO missile, hits a moon rock or a hole, then explode for some time
    //and after a while reset to beginning of section
    public void explode(Canvas canvas, Paint paint, float left, float top) {
        canvas.drawBitmap(explode, left, top, paint);
    }

    //if rover fires a missile then draw the missile
    //not implemented yet
    public void fireMissile(Canvas canvas) {

    }

    public void draw(Canvas canvas, Paint paint, float left, float top) {
        canvas.drawBitmap(bitmapRover, left, top, paint);
    }

    //if tank jumps then draw the jumping rover
    public void jump(Canvas canvas, boolean up) {
        if (up && jumpHeight < 500) {
            jumpHeight = jumpHeight + 7;
            if (distanceDelta < 3) distanceDelta = distanceDelta + 0.55;
        } else if (jumpHeight > 0) {
            jumpHeight = jumpHeight - 4;
            if (distanceDelta < 3) distanceDelta = distanceDelta + 0.55;
        }

    }

    boolean roverDestroysEnemyMissile, wasHit, waitForTimer, waitForUfoTimer;
    int missileOffSetY;

    // if buggy was hit by a missile then return true
    private boolean checkBuggyHitByMissile(Canvas canvas, ParallaxView view, int buggyXDisplacement, double buggyXDistance, Paint paint, Bitmap buggy, int jumpHeight) {

        for (Missile missile : missiles) {
            if (!roverDestroysEnemyMissile && !UFO.recent && !view.waitForTimer && java.lang.Math.abs(missile.getX() -  buggyXDisplacement + buggyXDistance ) < 150 ) {
                UFO.recent = true;
                canvas.drawBitmap(view.explode, (float) (buggyXDisplacement + buggyXDistance), (float) (screenHeight * 0.5) - jumpHeight, paint);
                ParallaxView.bombed--;
                missileOffSetY = 0;
                wasHit = true;
                view.recent = true;
                Handler handler = new Handler(Looper.getMainLooper());
                handler.postDelayed(() -> {
                    UFO.recent = false;
                    waitForTimer = false;
                    wasHit = false;
                }, 4500);
                waitForTimer = true;
            } else if (!roverDestroysEnemyMissile && !waitForTimer && !waitForUfoTimer && MoonBackground.checkpoint >= 'A') {
                //fire list of missiles
                canvas.drawBitmap(tankMissile, (float) missile.getX() , (float) (screenHeight * 0.56), paint);
                missile.setX(missile.getX() - 4);
            } else {
                //  missile.setY();
                  missile.setX(this.tankX-this.getBitmapRover().getWidth()/2);
            }
            wasHit = false;
        }
        return wasHit;
    }

    boolean alreadyExecuted = false;
    private long fireTimeout = System.currentTimeMillis();
    private int missileX = 25;

    //return boolean if tank fires, boolean which is not yet used
    private boolean checkFire() {
        if (System.currentTimeMillis() - fireTimeout >= randomize(10000, 8500)) {
            missileX = tankX;
            if (!alreadyExecuted) {
                List<Missile> thingsToBeAdd = new ArrayList<Missile>();
                thingsToBeAdd.add(new Missile((int) (this.getBuggyXdistance()-this.getBitmapRover().getWidth()/2), tankY));
                missiles.addAll(thingsToBeAdd);
                alreadyExecuted = true;
                Handler handler = new Handler(Looper.getMainLooper());
                handler.postDelayed(() -> {
                    alreadyExecuted = false;
                }, 10000);
            }
        }
        return true;
    }


    public boolean drawMissile(ParallaxView view, Canvas canvas, Paint paint, int buggyXDisplacement, double buggyXDistance, Bitmap buggy, int jumpHeight) {
        checkFire();
        return checkBuggyHitByMissile(canvas, view, buggyXDisplacement, buggyXDistance, paint, buggy, jumpHeight);
    }

}

Answer 1

1. Please keep all the class variables together, and getter/setters together, right now they are scattered in the class.
2. Either use Lambdas everywhere, or Anonymous classes for Runnable. You have used both in the class.
3. In the constructor, you may extract context.getResources() into a variable, as you are repeating it many times. Same goes with any other repeating calls.
4. All the strings used in the files should be used as static final or as Enum, like the strings 'drawable', 'explode', etc.
5. Parameter canvas is not used in method jump.
6. In the jump method, there are many numbers, you could give these numbers a name to easily identify what they represent.
7. Please add brackets {} after every if or else, even if it has only one statement.
8. if (distanceDelta < 3) distanceDelta = distanceDelta + 0.55; is duplicated in the same method.
9. if (!roverDestroysEnemyMissile && !UFO.recent && !view.waitForTimer && java.lang.Math.abs(missile.getX() - buggyXDisplacement + buggyXDistance ) < 150 ) is too big to understand, should be extracted to a method with meaningful name.
10. Lambda in checkFire doesn't need brackets as it has only one statement.
11. Usually, a method that returns boolean starts with is or has, so checkBuggyHitByMissile could be renamed to isBuggyHitByMissile. Just a suggestion, may depend on what convention you guys follow in the team.
12. And prefer composition over inheritance.

P.S.- Congratulations on the beta launch. All the very best.