Android game inspired by Space Invaders and Moon Patrol

Question

I'm creating a mini game for Android inspired by space invaders and Moon Patrol. I have completed the important part except for collision detection. Before I do collision detection I want to clean the code and make it more idiomatic but I am not sure how. It has several global variables and difficult to read and with "magic numbers". Could you give me some suggestions how to improve? I am going to try and rewrite it with less "magic numbers" and less global variables. Any other suggestions?

import android.content.Context;
import android.graphics.Bitmap;
import android.graphics.BitmapFactory;
import android.graphics.Canvas;
import android.graphics.Color;
import android.graphics.Paint;
import android.graphics.Rect;
import android.support.v4.view.MotionEventCompat;
import android.util.Log;
import android.view.MotionEvent;
import android.view.SurfaceHolder;
import android.view.SurfaceView;
import android.widget.Toast;

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

public class ParallaxView extends SurfaceView implements Runnable {

    List<Background> backgrounds;

    private volatile boolean running;
    private Thread gameThread = null;

    // For drawing
    private Paint paint;
    private Canvas canvas;
    private SurfaceHolder ourHolder;

    // Holds a reference to the Activity
    Context context;

    // Control the fps
    long fps = 60;

    // Screen resolution
    int screenWidth;
    int screenHeight;

    private void update() {
        // Update all the background positions
        for (Background bg : backgrounds) {
            bg.update(fps);
        }

    }

    ParallaxView(Context context, int screenWidth, int screenHeight) {
        super(context);

        this.context = context;

        this.screenWidth = screenWidth;
        this.screenHeight = screenHeight;

        // Initialize our drawing objects
        ourHolder = getHolder();
        paint = new Paint();

        // Initialize our array list
        backgrounds = new ArrayList<>();

        //load the background data into the Background objects and
        // place them in our GameObject arraylist

        backgrounds.add(new Background(
                this.context,
                screenWidth,
                screenHeight,
                "bg", 0, 120, 50));

        backgrounds.add(new Background(
                this.context,
                screenWidth,
                screenHeight,
                "grass", 70, 110, 200));

        // Add more backgrounds here

    }

    @Override
    public void run() {

        while (running) {
            long startFrameTime = System.currentTimeMillis();

            update();
            if (j > 2000) {
                j = -50;
                k = 0;
            }
            draw();

            // Calculate the fps this frame
            long timeThisFrame = System.currentTimeMillis() - startFrameTime;
            if (timeThisFrame >= 1) {
                fps = 1000 / timeThisFrame;
            }
        }
    }

    int numberOfshots = 1;
    int[] i = new int[200];//350;
    int j = 0;
    int k = 0;
    int l = 0;
    int m = 0;
    boolean down = true;
    long lastTurn = System.currentTimeMillis();
    int xbuggy = 0;
    int xbuggy2 = 0;
    boolean down2 = true;
    long lastTurn2 = System.currentTimeMillis();
    long lastTurn3 = System.currentTimeMillis();
    boolean jump = false;
    boolean shoot = false;
    int ind = 0;

    private void draw() {

        if (ourHolder.getSurface().isValid()) {
            //First we lock the area of memory we will be drawing to
            canvas = ourHolder.lockCanvas();
            if (jump) {
                xbuggy = xbuggy + 4;
            }
            if (shoot) {
                xbuggy2 = xbuggy2 + 4;
            }

            if (System.currentTimeMillis() - lastTurn3 >= 1000) {
                // Change direction here
                jump = false;
                lastTurn3 = System.currentTimeMillis();
                xbuggy = 0;
            }
            //draw a background color
            canvas.drawColor(Color.argb(255, 0, 0, 0));

            // Draw the background parallax
            drawBackground(0);

            // Draw the rest of the game
            paint.setTextSize(60);
            paint.setColor(Color.argb(255, 255, 255, 255));

            //canvas.drawText("MOONPATROL3000", 350, screenHeight / 100 * 5, paint);

            int resID = context.getResources().getIdentifier("vehicle",
                    "drawable", context.getPackageName());

            int alienResID = context.getResources().getIdentifier("object3_hdpi",
                    "drawable", context.getPackageName());

            int alienResID2 = context.getResources().getIdentifier("object2_hdpi",
                    "drawable", context.getPackageName());

            int alienResID3 = context.getResources().getIdentifier("object1_hdpi",
                    "drawable", context.getPackageName());

            // Load the bitmap using the id
            Bitmap bitmap = BitmapFactory.decodeResource(context.getResources(), resID);
            Bitmap alienbitmap = BitmapFactory.decodeResource(context.getResources(), alienResID);
            Bitmap alienbitmap2 = BitmapFactory.decodeResource(context.getResources(), alienResID2);
            Bitmap alienbitmap3 = BitmapFactory.decodeResource(context.getResources(), alienResID3);

            //paint.setTextSize(220);
            for (int i1 = 0; i1 < numberOfshots; i1++) {
                canvas.drawText("o", i[i1], (float) (screenHeight * 0.61), paint);
                canvas.drawText("o", 185, screenHeight / 100 * 95 - i[i1] - xbuggy2, paint);
                if (i1 == numberOfshots - 1 && i[i1] > screenWidth) {
                    if (numberOfshots > 0) numberOfshots--;
                    if (ind > 0) ind--;
                }
            }
            if (System.currentTimeMillis() - lastTurn >= 9000) {
                // Change direction here
                down = !down;
                lastTurn = System.currentTimeMillis();
            }

            if (System.currentTimeMillis() - lastTurn2 >= 7000) {
                // Change direction here
                down2 = !down2;
                lastTurn2 = System.currentTimeMillis();
            }

            canvas.drawBitmap(alienbitmap, j, k + screenHeight / 100 * 45, paint);
            canvas.drawBitmap(alienbitmap2, j + 10, l + screenHeight / 100 * 25, paint);
            //canvas.drawBitmap(alienbitmap3, j+20, k+screenHeight / 100 * 5, paint);
            drawBackground(1);
            canvas.drawBitmap(bitmap, 50, (float) (screenHeight * 0.5) - xbuggy, paint);
            // Draw the foreground parallax

            for (int n = 0; n < numberOfshots; n++)
                i[n] = i[n] + 20;

            j = j + 2;
            if (!down)
                k++;
            else
                k--;

            if (!down2)
                l++;
            else
                l--;

            // Unlock and draw the scene
            ourHolder.unlockCanvasAndPost(canvas);
        }
    }

    // Clean up our thread if the game is stopped
    public void pause() {
        running = false;
        try {
            gameThread.join();
        } catch (InterruptedException e) {
            // Error
        }
    }

    // Make a new thread and start it
    // Execution moves to our run method
    public void resume() {
        running = true;
        gameThread = new Thread(this);
        gameThread.start();
    }

    private void drawBackground(int position) {

        // Make a copy of the relevant background
        Background bg = backgrounds.get(position);

        // define what portion of images to capture and
        // what coordinates of screen to draw them at

        // For the regular bitmap
        Rect fromRect1 = new Rect(0, 0, bg.width - bg.xClip, bg.height);
        Rect toRect1 = new Rect(bg.xClip, bg.startY, bg.width, bg.endY);

        // For the reversed background
        Rect fromRect2 = new Rect(bg.width - bg.xClip, 0, bg.width, bg.height);
        Rect toRect2 = new Rect(0, bg.startY, bg.xClip, bg.endY);

        //draw the two background bitmaps
        if (!bg.reversedFirst) {
            canvas.drawBitmap(bg.bitmap, fromRect1, toRect1, paint);
            canvas.drawBitmap(bg.bitmapReversed, fromRect2, toRect2, paint);
        } else {
            canvas.drawBitmap(bg.bitmap, fromRect2, toRect2, paint);
            canvas.drawBitmap(bg.bitmapReversed, fromRect1, toRect1, paint);
        }
    }

    // Because we call this from onTouchEvent, this code will be executed for both
    // normal touch events and for when the system calls this using Accessibility
    @Override
    public boolean performClick() {
        super.performClick();
        launchMissile();
        return true;
    }

    private void launchMissile() {
        i[ind] = 350;
        ind++;
        xbuggy2 = 0;
        shoot = true;
    }

    // event listener for when the user touches the screen
    @Override
    public boolean onTouchEvent(MotionEvent event) {
        boolean gameOver = false;
        //if (paused) {
        //   paused = false;
        //}
        int action = MotionEventCompat.getActionMasked(event);
        int coordX = (int) event.getX();
        int coordY = (int) event.getY();
        Log.d("coordY", "coordY " + coordY);
        if (coordX < 220 && xbuggy == 0 && action == MotionEvent.ACTION_MOVE) {
            jump = true;
            shoot = false;
            lastTurn3 = System.currentTimeMillis();
            return true; // do nothing
        }

        if (coordX > 219 && action == MotionEvent.ACTION_DOWN) {
            numberOfshots++;
            performClick();
            return true;
        }
        return true;
    }
}

Answer 1

Thanks for sharing your code.

organize your code

use automatic code reordering

In your code all the different types of code are mixed. This make it hard to read your code.

There is a common order code is organized in a Java class. Most IDEs offer to reorder (sort) your code. The result of that is not always the best outcome. Especially since this automatic reordering does not care for the dependencies of methods and/or variables between each other. But you may go with that fore a while just to get used to how it should look like in general.

apply OO principles

OOP doesn't mean to "split up" code into random classes.

The ultimate goal of OOP is to reduce code duplication, improve readability and support reuse as well as extending the code.

Doing OOP means that you follow certain principles which are (among others):

• information hiding / encapsulation
• SRP/SoC (single responsibility principle/ separation of concerns)
• KISS (Keep it simple (and) stupid.)
• DRY (Don't repeat yourself.)
• "Tell! Don't ask."
• Law of demeter ("Don't talk to strangers!")
• SLA (single layer of abstraction)

you should start with the second (SRP/SoC) by splitting your long methods into smaller ones as suggeste by @Stephan in his comment. A good way to start with that is to look at your inline comments. You use this commenst to "mark" distinck sections in your long method. Just extract the code between two of this comments an into a method with a name derived from the upper comment:

private void draw() {
    if (ourHolder.getSurface().isValid()) {
        //First we lock the area of memory we will be drawing to
        canvas = ourHolder.lockCanvas();
        if (jump) {
            xbuggy = xbuggy + 4;
        }
        if (shoot) {
            xbuggy2 = xbuggy2 + 4;
        }

        if (System.currentTimeMillis() - lastTurn3 >= 1000) {
            // Change direction here
            jump = false;
            lastTurn3 = System.currentTimeMillis();
            xbuggy = 0;
        }
        //draw a background color
        canvas.drawColor(Color.argb(255, 0, 0, 0));

        // Draw the background parallax
        drawBackground(0);

would become this:

private void lockTheAreaOfMemoryWeWillBeDrawingTo(){
        canvas = ourHolder.lockCanvas();
        if (jump) {
            xbuggy = xbuggy + 4;
        }
        if (shoot) {
            xbuggy2 = xbuggy2 + 4;
        }

        if (System.currentTimeMillis() - lastTurn3 >= 1000) {
            // Change direction here
            jump = false;
            lastTurn3 = System.currentTimeMillis();
            xbuggy = 0;
        }
}

private void drawBackgroundColor(Color color){
    canvas.drawColor(color);
}

private void draw() {
    if (ourHolder.getSurface().isValid()) {
        lockTheAreaOfMemoryWeWillBeDrawingTo();
        drawBackgroundColor(THE_BACKGROUND_COLOR);
        drawBackgroundParallax(FIRST_POSITION); // existing renamed

---

Along with that you should apply SLA which means that a method should either call other methods or to "primitive" operations, not both. (loops, branches and try/catch are not "primitive" in that sense).

E.g.: if you have changed your draw() method as suggested the condition within the if statement is braking SLA. (OK, it is breaking "don't talk to strangers" more obvious but solving this needs a more complex solution...)
However, you should extract that condition so a separate method too:

private boolean isSurfaceValid(){
    return ourHolder.getSurface().isValid()
}

private void draw() {
    if (isSurfaceValid()) {
        lockTheAreaOfMemoryWeWillBeDrawingTo();
        drawBackgroundColor(THE_BACKGROUND_COLOR);
        drawBackgroundParallax(FIRST_POSITION);

apply Model View Controller pattern (MVC)

You may be attempted to do your collision detection by analyzing the graphics you crate. this is a quite hard thing to do.

The better approach is to create a "world model" which is independent from the target devices resolution It is much easier to find collisions in this "world model".

Also you should separate methods that directly handle User IO (drawing or getting input -> View layer) from code that manipulates the "world model" (Controller layer).