Return to Answer

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.

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.