Android GameWatch game for learning/review

Question

I am hoping this is the right place. I was hoping to find someone to try and help me not only learn but have a look at what I have already done on Android.

DISCLAIMER: I am a long time (10+ years) script developer (PHP, Perl, Ruby) and am happy with OO but have not really had any experience with Java so this is a bit of an experiment so expect totally n00b questions.

I set myself a goal (it's always good to have one I find). I wanted to make an android version of the Nintendo Game & Watch game Fire. I used the graphics from the inter webs and I'm well aware that should I release this on Google Play that Nintendo will send me a nice letter telling me to remove it so I am currently just looking to use this as a learning process.

I have been reading an Android game developers book and implementing what I learn from that as well as surfing SO and the web in general for help.

So if your willing have a look at what I have so far and if you can give me some feedback/pointers I will appreciate it. I intend to update the project as often as I can with new questions that come up that I cannot solve and even write how I solved them (if I do), maybe even go so far as to use an organisation tool like Workflowy to do this.

I have pasted here the GameView file. Look in the update function where I am checking the collision of the jumper.

package com.example.gamewatch;
/**
 * Only Currently tested on an emulator, emullating a 7" screen (Nexus 7)
 * 1280 x 628
 */
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ArrayBlockingQueue;
import android.os.AsyncTask;
import android.content.Context;
import android.graphics.*;
import android.util.Log;
import android.view.MotionEvent;
import android.view.SurfaceHolder;
import android.view.SurfaceView;
import android.util.Log;
public class GameView extends SurfaceView implements SurfaceHolder.Callback {
 private GameLogic mGameLogic;
 private ArrayBlockingQueue<InputObject> inputObjectPool;
 private Map<String, AnimatedSprite> map = new HashMap<String, AnimatedSprite>();
 private Integer score = 0;
 // Not being used at the moment but maybe in the future
 private class ScoringTask extends AsyncTask<Canvas, Void, Boolean>
 {
 protected Boolean doInBackground(Canvas... canvas) {
 Paint paint = new Paint();
 paint.setColor(Color.RED);
 paint.setTextSize(30);
 //canvas.drawText("000", getWidth() - 100, 0, paint);
 //canvas.drawText("001", getWidth() - 100, 0, paint);
 return true;
 }
 }
 public GameView(Context context) {
 super(context);
 getHolder().addCallback(this);
 mGameLogic = new GameLogic(getHolder(), this);
 createInputObjectPool();
 setFocusable(true);
 }
 private void createSprites() {
 map.put("firemen", new FiremenSprite(
 BitmapFactory.decodeResource(getResources(), R.drawable.firemen)
 ));
 map.put("jumper", new JumperSprite(
 BitmapFactory.decodeResource(getResources(), R.drawable.jumper)
 ));
 }
 private void createInputObjectPool() {
 inputObjectPool = new ArrayBlockingQueue<InputObject>(20);
 for (int i = 0; i < 20; i++) {
 inputObjectPool.add(new InputObject(inputObjectPool));
 }
 }
 @Override
 public boolean onTouchEvent(MotionEvent event) {
 try {
 int hist = event.getHistorySize();
 if (hist > 0) {
 for (int i = 0; i < hist; i++) {
 InputObject input = inputObjectPool.take();
 input.useEventHistory(event, i);
 mGameLogic.feedInput(input);
 }
 }
 InputObject input = inputObjectPool.take();
 input.useEvent(event);
 mGameLogic.feedInput(input);
 } catch (InterruptedException e) {
 }
 try {
 Thread.sleep(16);
 } catch (InterruptedException e) {
 }
 return true;
 }
 public void processMotionEvent(InputObject input){
 int half = (getWidth() / 2);
 int movement = 0;
 AnimatedSprite firemen = map.get("firemen");
 if(input.x < half) { // Left
 firemen.moveLeft();
 } else { // Right
 firemen.moveRight();
 }
 }
 public void processKeyEvent(InputObject input){
 }
 @Override
 public void surfaceChanged(SurfaceHolder holder, int format, int width, int height) {
 }
 @Override
 public void surfaceCreated(SurfaceHolder holder) {
 createSprites();
 mGameLogic.setGameState(mGameLogic.RUNNING);
 mGameLogic.start();
 }
 @Override
 public void surfaceDestroyed(SurfaceHolder holder) {
 }
 public void onDraw(Canvas canvas)
 {
 Bitmap b = BitmapFactory.decodeResource(getResources(), R.drawable.background);
 int screenWidth = getWidth();
 int screenHeight = getHeight();
 float screenAspect = screenWidth / screenHeight;
 int addAmount = screenWidth - b.getWidth();
 int half = screenWidth / 2;
 //Log.d("Screen", "Height: " + getHeight() + " Width: " + getWidth());
 //b = b.createScaledBitmap(b, getWidth(), b.getHeight() + addAmount, false);
 Typeface tf = Typeface.create("Helvetica",Typeface.BOLD);
 Paint paint = new Paint();
 paint.setTypeface(tf);
 paint.setColor(Color.RED);
 paint.setTextSize(30);
 canvas.drawBitmap(b, 0, 0, null);
 canvas.drawLine(half, 0, half, screenHeight, paint);
 canvas.drawText("LEFT", half - 100, screenHeight - 50, paint);
 canvas.drawText("RIGHT", half + 50, screenHeight - 50, paint);
 // Iterate through the sprite map that was created earlier and draw them to the screen
 for (Map.Entry<String, AnimatedSprite> entry : map.entrySet())
 {
 ((AnimatedSprite) entry.getValue()).draw(canvas);
 }
 }
 public void update() {
 AnimatedSprite f = map.get("firemen");
 AnimatedSprite j = map.get("jumper");
 /**
 * Does the firemen still have more lives?
 */
 if(f.getLives() <= 0) {
 Log.d("LIVES", "GameOver");
 mGameLogic.setGameState(mGameLogic.FINISHED);
 // What I would like to do here is finish the activity and hand back to the menu but I have no idea how
 }
 // Is the jumper at the bottom of his animations and collided with the Firemen
 if(j.isAtBottom() && !f.collide(j))
 {
 f.setLives(f.getLives()-1);
 Log.d("LIVES", "The jumper is at the bottom but has not collided with the fireman" +
 " this should loose a life");
 // We also need to re-set the jumper here
 j.setCurrentFrame(0);
 } else if (j.isAtBottom() && f.collide(j)) {
 // Here is where we should do something with the score perhaps
 }
 // Iterate through the sprite map that was created earlier and update them all according
 // to the timer and check for a collision
 for (Map.Entry<String, AnimatedSprite> entry : map.entrySet())
 {
 AnimatedSprite s = ((AnimatedSprite) entry.getValue());
 s.update(System.currentTimeMillis());
 }
 }
}

• Simon Forsberg: Code Review is the right place for reviewing code, we are not here to solve problems. Most of your question is about reviewing code though so it seems like you have come to the right place.

Answer 1

Overall your code is good. If I only would say that though, you probably wouldn't learn much, and luckily for you I do have some points that you can improve:

• Within the (currently unused) doInBackground method in ScoringTask, you are creating a Paint object that you set color and text size on, but then you're not doing anything else with it which means that it will be garbage collected directly and what you just did was useless. I know it's currently unused, but still :)

• I don't see the meaning of this code:

  try {
   Thread.sleep(16);
  } catch (InterruptedException e) {
  }
  

  First of all, you should not run Thread.sleep on the UI-thread. Doing so will make your application not respond for the duration of Thread.sleep. Now, 16 ms is not a long time but it is still not good practice to make the UI-thread sleep. Right before this code snippet, you are also catching a InterruptedException - why? I really don't think that code should be able to throw such an exception. And if it does, try to avoid that (if you call Thread.sleep for example).

• The variables mGameLogic.RUNNING and mGameLogic.FINISHED should be set to public static final where they are declared. It sounds like these are constants (RUNNING = 1 and FINISHED = 2 or similar for example), constants should be static final. Then you should instead write GameLogic.RUNNING and GameLogic.FINISHED.

• When you iterate over the Map, you don't need to typecast entry.getValue() to AnimatedSprite. Since you are using generics correctly (Map.Entry<String, AnimatedSprite>), entry.getValue() returns an AnimatedSprite already. No need for typecasting.

• (Minor issue, not necessarily needed either) Instead of iterating on map.entrySet() which will give you both key and value, you can iterate on map.values() which only gives you the value. (You're currently not using the key when you are iterating on the entry set)

• In your update method, f and j are very short variable names, and therefore making their purpose unclear. firemen and jumper would be better names for those variables.

To finish the activity from the view either call getContext(), typecast that to an Activity (assuming this won't throw a ClassCastException) and call finish() on that object, or use a callback. (I would recommend using a callback, it is bad practice to let a view get it's activity. The view should be independent)

Something to read up on when you have some more experience: Should the view really be responsible for creating the GameLogic object? There is a design pattern called MVC which can be good to keep in mind (not necessarily following it to 100%) when dealing with both game logic and a view. Don't fix what isn't broken though, MVC can be heavy stuff. Learn that at your own risk.

• Catharsis: I'll deal with your comments briefly. The Thread.sleep was a copy and paste from the 'Beginners Android Tablet Game programming' book I have been reading Apress which states "Finally, you cause the main thread to sleep for 16 milliseconds to make sure you don’t gather too much input at one time.". The book may be old and out of date. I have used MVC extensively in PHP, but again I took the current approach from that book. Will look into the callback and continue to learn :) Thank you again

Answer 2

According to Android coding style guidelines, protected and private class members names should start from "m" letter, like mGameLogic.

• Catharsis: Can you point me at the guidelines that state this, I would like to know their reasoning behind it? I've run a quick google and not found anything (yet)
• Alfredo Cavalcanti: @catharsis take a look here
• syb0rg: You should include a link to the guidelines in this answer.
• artem: @syb0rg, source.android.com/source/…
• syb0rg: @RankoR Yes, by tacking on a link to the end of it. URL's are ugly, use hyperlinks instead. It looks prettier :)