Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite
1

i am using the following template to program my 2D games in. is there anyway 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);
            }
        }
    }
}
}

Also does anyone know of any good 3d that i could try? Thankyou very much for any help in advance.

share|improve this question

1 Answer

up vote 5 down vote accepted

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

On the inpredictability 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 constansts
  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;
    }
}
share|improve this answer
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? – DingleNutZ Aug 3 '11 at 13:33
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 – DingleNutZ Aug 3 '11 at 13:34
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. – Aleksi Yrttiaho Aug 3 '11 at 16:13
+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. – Rudy Aug 4 '11 at 8:14
@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 – DingleNutZ Aug 4 '11 at 11:55

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.