This is improved code from my previous question. This mini game which we call "Moon Buggy" is available in beta from the google playstore.
The action is that you control a vechicle on the moon and you defend yourself against evil UFO:s.
I have written a separate class for the UFO which is instanciated once for every UFO:
import android.content.Context;
import android.graphics.Bitmap;
import android.graphics.BitmapFactory;
import android.graphics.Canvas;
import android.graphics.Paint;
import android.os.Handler;
import android.os.Looper;
import java.util.Random;
public class AttackingAlien extends Alien {
private Bitmap alien;
static boolean recent = true; // switch this in beginning
private long changeDirections = System.currentTimeMillis();
private long fireTimeout = System.currentTimeMillis();
private int screenWidth;
private int screenHeight;
private int ufoY = 0;
private int ufoX = 0;
private int missileX = 25;
private int deltaUfoY = 7;
private int deltaUfoX = 7;
private int missileOffSetY = 0;
private int missileYstart = 0;
private boolean wasHit = false;
private boolean alienexplode;
private boolean waitForTimer, waitForUfoTimer;
private boolean toggleDeltaY = true;
private boolean runOnce = true;
private boolean startMissile = true;
public AttackingAlien(ParallaxView view, Context context, String name, final int screenWidth, int screenHeight, int p) {
super(context, name);
this.deltaUfoY = p;
int alienResID = context.getResources().getIdentifier(name,
"drawable", context.getPackageName());
alien = BitmapFactory.decodeResource(context.getResources(), alienResID);
int max = (int) (0.75 * screenWidth);
int min = 20;
int diff = max - min;
Random rn = new Random();
int i5 = rn.nextInt(diff + 1);
i5 += min;
missileX = i5;
ufoX = missileX;
max = 200;
diff = max - min;
this.screenHeight = screenHeight;
this.screenWidth = screenWidth;
ufoY = 0;
waitForUfoTimer = true;
int max2 = 20000;
int min2 = 18000;
int diff2 = max2 - min2;
Random rn2 = new Random();
int result = rn2.nextInt(diff2 + 1);
result += min2;
Handler handler = new Handler(Looper.getMainLooper());
handler.postDelayed(new Runnable() {
@Override
public void run() {
missileX = ufoX;
setRecent();
waitForUfoTimer = false;
}
}, result);
}
private void changeDirections() {
int max2 = 500;
int min2 = 100;
int diff2 = max2 - min2;
Random rn2 = new Random();
int result = rn2.nextInt(diff2 + 1);
result += min2;
if (System.currentTimeMillis() - changeDirections >= result) {
// Change direction here
toggleDeltaY = !toggleDeltaY;
changeDirections = System.currentTimeMillis();
}
}
public void update(Canvas canvas, Paint paint, boolean toggleDeltaY, int screenWidth, int screenHeight) {
if (ufoX > screenWidth - 250 || ufoX < 10) { // UFO change horizontal direction
deltaUfoX = -deltaUfoX;
}
//make sure UFO does not move too low
if (ufoY >= 20) {
deltaUfoY = -deltaUfoY;
}
if ((ufoY + screenHeight / 100 * 25) <= 0) // don't move outside the top
deltaUfoY = -deltaUfoY;
if (!waitForUfoTimer && Background.checkpoint >= 'A') { // && sectionComplete > 0) {
runOnce = true;
//alienY++;
canvas.drawBitmap(alien, ufoX + 10, ufoY + screenHeight / 100 * 25, paint);
}
//missileX = missileX + speedAlienX;
ufoX = ufoX + deltaUfoX;
if (waitForTimer) missileX = ufoX;
if (toggleDeltaY) {
deltaUfoY = -deltaUfoY;
}
ufoY = ufoY + deltaUfoY;
changeDirections();
}
public void checkBeingHit(int[] missiles, int buggyXDisplacement, double buggyXDistance, Canvas canvas, Bitmap explode2, Paint paint, int score, ParallaxView pview, int i1, int xbuggy2) {
// if UFO is being hit by buggy
if (!waitForTimer && java.lang.Math.abs(ufoX + 10 - 400 - buggyXDistance) * 2 < (alien.getWidth()) && java.lang.Math.abs(ufoY + screenHeight / 100 * 25 - (screenHeight / 100 * 95 - missiles[i1] - xbuggy2)) * 2 < (alien.getHeight())) {
missileOffSetY = -9999;
canvas.drawBitmap(explode2, ufoX + 10, ufoY + screenHeight / 100 * 25, paint);
if (runOnce) {
ParallaxView.score = ParallaxView.score + 100;
Handler handler = new Handler(Looper.getMainLooper());
handler.postDelayed(new Runnable() {
@Override
public void run() {
int max = (int) (0.75 * screenWidth);
int min = 20;
int diff = max - min;
Random rn = new Random();
int i5 = rn.nextInt(diff + 1);
i5 += min;
missileX = i5;//25;
ufoX = missileX;
ufoY = 0;
alienexplode = false;
waitForTimer = false;
waitForUfoTimer = false;
startMissile = true;
}
}, 3000);
}
runOnce = false;
waitForUfoTimer = true;
startMissile = false;
waitForTimer = true;
if (!alienexplode) {
pview.changeText();
}
alienexplode = true;
}
}
private void checkFire() {
int max = 15000;
int min = 12000;
int diff = max - min;
Random rn = new Random();
int i5 = rn.nextInt(diff + 1);
i5 += min;
if (System.currentTimeMillis() - fireTimeout >= i5) { // means how often the alien fires
fireTimeout = System.currentTimeMillis();
missileOffSetY = 0;
missileX = ufoX;
}
}
private void setRecent() {
AttackingAlien.recent = false;
}
public boolean drawMissile(ParallaxView view, Canvas canvas, Paint paint, int buggyXDisplacement, double buggyXDistance, Bitmap buggy, int jumpHeight, int screenHeight) {
wasHit = false;
checkFire();
// if buggy was hit by a missile
if (!AttackingAlien.recent && !view.waitForTimer && java.lang.Math.abs(((buggyXDisplacement + buggyXDistance) + buggy.getWidth() / 2) - (missileX + 10 + alien.getWidth() / 2)) < buggy.getWidth() / 2 && java.lang.Math.abs((ufoY + screenHeight / 100 * 25 + 75 + missileOffSetY) - ((screenHeight * 0.3) - jumpHeight + buggy.getHeight())) < 65) {
AttackingAlien.recent = true;
canvas.drawBitmap(view.explode, (float) (buggyXDisplacement + buggyXDistance), (float) (screenHeight * 0.5) - jumpHeight, paint);
ParallaxView.bombed--;
missileOffSetY = 0;
wasHit = true;
view.recent = true;
Handler handler = new Handler(Looper.getMainLooper());
handler.postDelayed(new Runnable() {
@Override
public void run() {
setRecent();
waitForTimer = false;
wasHit = false;
}
}, 7000);
waitForTimer = true;
} else {
// buggy was not hit so UFO fires more missiles
//TODO: check if the movements are realistic
if (!waitForTimer && !waitForUfoTimer && Background.checkpoint >= 'A') {
if (startMissile) {
startMissile = false;
missileYstart = ufoY;
}
canvas.drawText("●くろまる", missileX + alien.getWidth() / 2, missileYstart + screenHeight / 100 * 25 + alien.getHeight() + missileOffSetY, paint);
missileOffSetY = missileOffSetY + 4;
}
wasHit = false;
}
return wasHit;
}
}
The remainder of the code is available on request.
1 Answer 1
You should remove duplicated blocks.
int max2 = 20000; int min2 = 18000; int diff2 = max2 - min2; int result = rn2.nextInt(diff2 + 1); result += min2;
is repeated many times with slight variation in values of min and max. you can extract it as a function and pass the min-max values as parameters.
- You can replace your anonymous
Runnable
classes with lambda expressions if you are using Java 8 or above. Or extract them asinner class
with a meaningful name. - value of
diff
at line48
isn't used anywhere in the remaining code. - The
if
statement of methoddrawMissile
at line177
is too big to understand. Consider extracting it to a method with meaning full name, that explains the condition. - The constructor is too long and has too many parameters. And the variable
p
isn't a very useful variable name. You may group variables together in one class if they always come together, likescreeHeight
andscreenWidth
. Like we create a separateAddress
class to store address fields and methods, instead of keeping address fields and methods inUser
class orEmployee
class. Always group similar variables and methods together in a separate class. - The
update
method updates what? It isn't clear with the name if it will update Alien's position, UFO's position, Missile's positions, or anything else. - Method
checkBeingHit
too is very long and has a lot of parameters. Consider breaking down the method into smaller methods and reduce the parameter count by grouping similar variables together. - Same with
drawMissile
.
- Same with
- It can be renamed to
isHit
that returns aboolean
to show if the alien was hit or not, instead of having return type asvoid
. - Same with
checkFire
. setRecent
doesn't take any parameter to set the value of recent. Is it possible that you meantresetRecent
?
-
\$\begingroup\$ I cannot guess lambda syntax for Java 8 in this case. \$\endgroup\$Niklas Rosencrantz– Niklas Rosencrantz2018年07月08日 08:39:43 +00:00Commented Jul 8, 2018 at 8:39
-
\$\begingroup\$ @NiklasRosencrantz, it was just a suggestion if you were able to trim down the method of the anonymous class to one or two line. Else you can extract the whole anonymous block to inner class or a new class. \$\endgroup\$Ankit Soni– Ankit Soni2018年07月08日 23:36:33 +00:00Commented Jul 8, 2018 at 23:36