8
\$\begingroup\$

I am using the following template to program my 2D games in. Is there any way I can improve it?

Splash screen:

package com.dingle.template2d;

import android.app.Activity;
import android.content.Intent;
import android.os.Bundle;



public class template2d extends Activity {

@Override
public void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);

    //set screen
    setContentView(R.layout.splash);

    // thread for displaying the SplashScreen
    Thread splashTread = new Thread() {
        @Override
        public void run() {
            try {
                int waited = 0;
                boolean _active;
                int _splashTime;
                 _active = true;
                 _splashTime = 500;
                while(_active && (waited < _splashTime)) {
                    sleep(100);
                    if(_active) {
                        waited += 100;
                    }
                }
                startActivity(new Intent("com.dingle.template2d.MENU"));
            } catch(InterruptedException e) {
                // do nothing
            } finally {
                finish();


            }
        }
    };
    splashTread.start();


   }

 }

Menu screen:

package com.dingle.template2d;

import android.app.Activity;
import android.content.Intent;
import android.os.Bundle;
import android.view.View;
import android.view.Window;
import android.widget.Button;

public class menu extends Activity {
/** Called when the activity is first created. */
@Override
public void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    requestWindowFeature(Window.FEATURE_NO_TITLE);
    setContentView(R.layout.main);
    this.button();

}

private void button(){
       Button play_button = (Button)this.findViewById(R.id.play_button);
       play_button.setOnClickListener(
                  new Button.OnClickListener() {
                      public void onClick(View v) {
                    // parentButtonClicked(v);
                          startActivity(new      Intent("com.dingle.template2d.GAME"));
                     }
                  });
}
}

GameView class: (this is where I put all the game code)

package com.dingle.template2d;

import android.app.Activity;
import android.content.Context;
import android.graphics.Bitmap;
import android.graphics.BitmapFactory;
import android.graphics.Canvas;
import android.graphics.Color;
import android.os.Bundle;
import android.view.SurfaceHolder;
import android.view.SurfaceView;
import android.view.Window;

public class GameView extends Activity {

@Override
public void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    requestWindowFeature(Window.FEATURE_NO_TITLE);
    setContentView(new Panel(this));
}

public class Panel extends SurfaceView implements SurfaceHolder.Callback{

private MainThread _thread;

public Panel(Context context) {
    super(context);
    getHolder().addCallback(this);
    _thread = new MainThread(getHolder(), this);
    setFocusable(true);
}

final int windowHeight = getResources().getDisplayMetrics().heightPixels;
final int windowWidth = getResources().getDisplayMetrics().widthPixels;
final float tscale = getResources().getDisplayMetrics().density;
final int scale = (int) tscale;
public int _x = 0;
public int _y = 0;
Bitmap _scratch = BitmapFactory.decodeResource(getResources(), R.drawable.icon);
/**
*
*I Declare everything here
*
**/


@Override
public void onDraw(Canvas canvas) {
/**
*
*I run all my code here
*
**/
}

/*  @Override
public boolean onTouchEvent(MotionEvent event) {
    _x = (int) event.getX();
    _y = (int) event.getY();
    return true;
}*/

@Override
public void surfaceChanged(SurfaceHolder holder, int format, int width, int height) {
    // TODO Auto-generated method stub

}

@Override
public void surfaceCreated(SurfaceHolder holder) {
    _thread.setRunning(true);
    _thread.start();
}

@Override
public void surfaceDestroyed(SurfaceHolder holder) {
    // simply copied from sample application LunarLander:
    // we have to tell thread to shut down & wait for it to finish, or else
    // it might touch the Surface after we return and explode
    boolean retry = true;
    _thread.setRunning(false);
    while (retry) {
        try {
            _thread.join();
            retry = false;
        } catch (InterruptedException e) {
            // we will try it again and again...
        }
    }
}
}
}

Main Thread:

package com.dingle.template2d;

import com.dingle.template2d.GameView.Panel;

import android.graphics.Canvas;
import android.view.SurfaceHolder;

public class MainThread extends Thread {
private SurfaceHolder _surfaceHolder;
private Panel _panel;
private boolean _run = false;

public MainThread(SurfaceHolder surfaceHolder, Panel panel) {
    _surfaceHolder = surfaceHolder;
    _panel = panel;
}

public void setRunning(boolean run) {
    _run = run;
}

@Override
public void run() {
    Canvas c;
    while (_run) {
        c = null;
        try {
            c = _surfaceHolder.lockCanvas(null);
            synchronized (_surfaceHolder) {
                _panel.onDraw(c);
            }
        } finally {
            // do this in a finally so that if an exception is thrown
            // during the above, we don't leave the Surface in an
            // inconsistent state
            if (c != null) {
                _surfaceHolder.unlockCanvasAndPost(c);
            }
        }
    }
}
}
\$\endgroup\$
1
  • \$\begingroup\$ When you say you put all the game code in the GameView class, do you mean only rendering and UI code? Or are you putting all of your game model in this class also? \$\endgroup\$ – bazola Mar 18 '15 at 19:28
13
\$\begingroup\$

Notions in no particular order, not probably what you're expecting though:

On the unpredictability of sleep

In the loop

int waited = 0;
while(_active && (waited < _splashTime)) {
   sleep(100);
   if(_active) {
      waited += 100;
   }
}

Method sleep might not guarantee that the time slept is actually 100 ms. It might more or it might be less depending on the context and clock accuracy etc. It might be better if you relied on the system clock instead of a counter:

long started = System.currentTimeMillis();
while(_active && System.currentTimeMillis() - start < _splashTime) {
   sleep(100);
}

If you end up using the waited counter, the if(_active) conditions might not be necessary.

On initial values

If you have no particular reason for defining the variables initial values on separate lines, you could define them in the same line

int waited = 0;
boolean _active;
int _splashTime;
_active = true;
_splashTime = 500;

becomes

int waited = 0;
boolean _active = true;
int _splashTime = 500;

On naming

I've seen underscore to denote instance variables and I after a quick skimming I thought _activeand _splashTime were such. Instead they were local variables. Perhaps not using and underscore might be more conventional.

You use underscore on instance variables as well, which is OK though I'm not a big fan of it.

There's also some inconsistency with variable names with multiple words: compare surfaceHolder and play_button. In Java it's convetional that variable names are in camel-case without spaces; playButton would be better.

Names of classes usually begin with a capital letter e.g. Template2d, Menu.

On constants

It seems to me you're trying to declare a constant _splashTime. Why not just do so with

public class template2d extends Activity {
  private static final SPLASH_TIME_IN_MILLISECONDS = 500;
}

On visibity

I would prefer if all instance variables would be either private or final.

It's at least not a bad idea to keep the scope of your variables as small as possible. In

Canvas c;
while (_run) {
    c = null;
    try {
        c = _surfaceHolder.lockCanvas(null);
        synchronized (_surfaceHolder) {
            _panel.onDraw(c);
        }
    } finally {
        // do this in a finally so that if an exception is thrown
        // during the above, we don't leave the Surface in an
        // inconsistent state
        if (c != null) {
            _surfaceHolder.unlockCanvasAndPost(c);
        }
    }
}

The canvas variable c exists outside of the while-loop while it isn't used anywhere but in it. You could declare the variable inside the loop to limit it's scope

while (_run) {
    Canvas c = null;
    try {
        c = _surfaceHolder.lockCanvas(null);
        synchronized (_surfaceHolder) {
            _panel.onDraw(c);
        }
    } finally {
        // do this in a finally so that if an exception is thrown
        // during the above, we don't leave the Surface in an
        // inconsistent state
        if (c != null) {
            _surfaceHolder.unlockCanvasAndPost(c);
        }
    }
}

On order of things

The contents of a class is usually in an order not unline the following

  1. The class signature
  2. public static constants
  3. private static constants
  4. private static variables
  5. private constants
  6. private variables
  7. constructors
  8. public methods
  9. private methods

It makes the code harder to follow if there are instance variable declarations in more than one place.

Lately I've been experimenting on how it feels like when everything private is tucked down to the bottom of the class and I like it. Coding conventions trump personal preferences though.

On doing just one thing

Each class and object should preferably do just one thing i.e. they should have a single responsibility. At least the class Panel contain the responsibilities of drawing the surface and responding to events. These two things could be split into two different classes if it makes sense.

On different ways of writing the same code

In this code

retry = true;
while (retry) {
    try {
        _thread.join();
        retry = false;
    } catch (InterruptedException e) {
        // we will try it again and again...
    }
}

You could achieve the same thing with a break statement to get rid of an extra variable

while (true) { // or for(;;)
    try {
        _thread.join();
        break; 
    } catch (InterruptedException e) {
        // we will try it again and again...
    }
} 

or defining the logic in a method to describe your intent more clearly

while(!hasThreadStopped) {
     // Retry until thread stops
} 
...

private boolean hasThreadStopped() {
    try {
        _thread.join();
        return true;
    } catch (InterruptedException e) {
        return false;
    }
}
\$\endgroup\$
5
  • \$\begingroup\$ Thankyou, this is the exact critism i was looking for, could you please tell me more about running all my code in the draw event? \$\endgroup\$ – DingleNutZ Aug 3 '11 at 13:33
  • \$\begingroup\$ i used to use game maker, so there was always a "Step" event and a "Draw" event, the step event was where all code that wasnt needed for drawing was executed, is there something like this in java? as im just not seeing it \$\endgroup\$ – DingleNutZ Aug 3 '11 at 13:34
  • \$\begingroup\$ You have to implement the step and draw events yourself because java doesn't provide certain specialized abstractions out of the box. I may elaborate this later if nobody else is faster. \$\endgroup\$ – Aleksi Yrttiaho Aug 3 '11 at 16:13
  • \$\begingroup\$ +1 esp on the underscore before variable name. _varname is an old habit of C programmer, and while it's okay to compile, using this underscore should not be encouraged. \$\endgroup\$ – Rudy Aug 4 '11 at 8:14
  • \$\begingroup\$ @Aleksi i would very much like to hear about the step event, i have some big projects planned, and i need everything to run as smooth as possible \$\endgroup\$ – DingleNutZ Aug 4 '11 at 11:55
8
\$\begingroup\$

I cannot comment on 2D gaming but will suggest a better alternative of creating a thread to delay the splash screen. Instead of…

// thread for displaying the SplashScreen
Thread splashTread = new Thread() {
    @Override
    public void run() {
        try {
            int waited = 0;
            boolean _active;
            int _splashTime;
             _active = true;
             _splashTime = 500;
            while(_active && (waited < _splashTime)) {
                sleep(100);
                if(_active) {
                    waited += 100;
                }
            }
            startActivity(new Intent("com.dingle.template2d.MENU"));
        } catch(InterruptedException e) {
            // do nothing
        } finally {
            finish();


        }
    }
};
splashTread.start();

Handler().postDelayed will call the run method of runnable after set time and redirect to your menu.

new Handler().postDelayed(new Runnable() {
        @Override
        public void run() {
            startActivity(new Intent("com.dingle.template2d.MENU"));
            finish();
        }
    }, 5000);
\$\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.