2
\$\begingroup\$

I make a mini adventure game demo for Android where the player can move between places and NPC characters can enter and exit the places and perform random actions.

The "dirty" code is the UI for Android while the game engine is very good and clean.

I have this class which is quite unclean but it works and therefore I want a review.

package dev.game.adventure;

import android.annotation.SuppressLint;
import android.content.Context;
import android.graphics.*;
import android.support.annotation.Nullable;
import android.os.*;
import android.support.v7.app.ActionBar;
import android.support.v7.app.AppCompatActivity;
import android.text.method.ScrollingMovementMethod;
import android.util.AttributeSet;
import android.view.*;
import android.widget.*;
import java.util.Collection;
import java.util.Iterator;


/**
 * An adventure that shows and hides the system UI (i.e.
 * status bar and navigation/system bar) with user interaction.
 */
public class AdventureActivity extends AppCompatActivity {
    /**
     * Whether or not the system UI should be auto-hidden after
     * {@link #AUTO_HIDE_DELAY_MILLIS} milliseconds.
     */
    private static final boolean AUTO_HIDE = true;
    PlaceView ag;

    /**
     * If {@link #AUTO_HIDE} is set, the number of milliseconds to wait after
     * user interaction before hiding the system UI.
     */
    private static final int AUTO_HIDE_DELAY_MILLIS = 3000;

    /**
     * Some older devices needs a small delay between UI widget updates
     * and a change of the status and navigation bar.
     */
    private static final int UI_ANIMATION_DELAY = 300;
    private final Handler mHideHandler = new Handler();
    private View mContentView;
    private final Runnable mHidePart2Runnable = new Runnable() {
        @SuppressLint("InlinedApi")
        @Override
        public void run() {
            // Delayed removal of status and navigation bar

            // Note that some of these constants are new as of API 16 (Jelly Bean)
            // and API 19 (KitKat). It is safe to use them, as they are inlined
            // at compile-time and do nothing on earlier devices.
            mContentView.setSystemUiVisibility(View.SYSTEM_UI_FLAG_LOW_PROFILE
                    | View.SYSTEM_UI_FLAG_FULLSCREEN
                    | View.SYSTEM_UI_FLAG_LAYOUT_STABLE
                    | View.SYSTEM_UI_FLAG_IMMERSIVE_STICKY
                    | View.SYSTEM_UI_FLAG_LAYOUT_HIDE_NAVIGATION
                    | View.SYSTEM_UI_FLAG_HIDE_NAVIGATION);
        }
    };
    private View mControlsView;
    private final Runnable mShowPart2Runnable = new Runnable() {
        @Override
        public void run() {
            // Delayed display of UI elements
            ActionBar actionBar = getSupportActionBar();
            if (actionBar != null) {
                actionBar.show();
            }
            mControlsView.setVisibility(View.VISIBLE);
        }
    };
    private boolean mVisible;
    private final Runnable mHideRunnable = new Runnable() {
        @Override
        public void run() {
            hide();
        }
    };
    /**
     * Touch listener to use for in-layout UI controls to delay hiding the
     * system UI. This is to prevent the jarring behavior of controls going away
     * while interacting with activity UI.
     */
    private final View.OnTouchListener mDelayHideTouchListener = new View.OnTouchListener() {
        @Override
        public boolean onTouch(View view, MotionEvent motionEvent) {
            if (AUTO_HIDE) {
                delayedHide(AUTO_HIDE_DELAY_MILLIS);
            }
            return false;
        }
    };

    TextView scrollable;

    static final String[] FRUITS = new String[] { "Apple", "Avocado", "Banana",
            "Blueberry", "Coconut", "Durian", "Guava", "Kiwifruit",
            "Jackfruit", "Mango", "Olive", "Pear", "Sugar-apple" };
    @Override
    public void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_fullscreen);
        scrollable = (TextView) findViewById(R.id.textView1);
        scrollable.setMovementMethod(new ScrollingMovementMethod());
        ag = findViewById(R.id.view);
        ag.text = scrollable;
        ag = findViewById(R.id.view);
        new Adventure(scrollable, this, ag);
        Button buttonOne = (Button) findViewById(R.id.close);
        buttonOne.setOnClickListener(new Button.OnClickListener() {
            public void onClick(View v) {
                ag.mainCharacter.go("North", ag.target);
                ag.display("North", ag.target, ag.mainCharacter.place);
            }
        });

        Button buttonOne2 = (Button) findViewById(R.id.close2);
        buttonOne2.setOnClickListener(new Button.OnClickListener() {
            public void onClick(View v) {
                ag.mainCharacter.go("East", ag.target);
                ag.display("East", ag.target, ag.mainCharacter.place);
            }
        });

        Button buttonOne3 = (Button) findViewById(R.id.close3);
        buttonOne3.setOnClickListener(new Button.OnClickListener() {
            public void onClick(View v) {
                ag.mainCharacter.go("South", ag.target);
                ag.display("South", ag.target, ag.mainCharacter.place);
            }
        });

        Button buttonOne4 = (Button) findViewById(R.id.close4);
        buttonOne4.setOnClickListener(new Button.OnClickListener() {
            public void onClick(View v) {
                ag.mainCharacter.go("West", ag.target);
                ag.display("West", ag.target, ag.mainCharacter.place);
            }
        });

        Button buttonOne5 = (Button) findViewById(R.id.close5);
        buttonOne5.setOnClickListener(new Button.OnClickListener() {
            public void onClick(View v) {
                ag.mainCharacter.go("Up", ag.target);
                ag.display("Up", ag.target, ag.mainCharacter.place);
            }
        });

        Button buttonOne6 = (Button) findViewById(R.id.close6);
        buttonOne6.setOnClickListener(new Button.OnClickListener() {
            public void onClick(View v) {
                ag.mainCharacter.go("Down", ag.target);
                ag.display("Down", ag.target, ag.mainCharacter.place);
            }
        });

        Button buttonOne7 = (Button) findViewById(R.id.close7);
        buttonOne7.setOnClickListener(new Button.OnClickListener() {
            public void onClick(View v) {
                ag.mainCharacter.say("Where is the key?", ag.target);

                for (Iterator<Person> i = ag.persons.iterator(); i.hasNext();) {
                    Person item = i.next();
                    if (item instanceof WalkingPerson) {
                        WalkingPerson item2 = (WalkingPerson) item;
                        item2.query(ag.mainCharacter, ag.target);
                    }
                }
            }
        });


        Button buttonOne8 = (Button) findViewById(R.id.close8);
        buttonOne8.setOnClickListener(new Button.OnClickListener() {
            public void onClick(View v) {

                ag.mainCharacter.say("Now I attack you.", ag.target);

                for (Iterator<Person> i = ag.persons.iterator(); i.hasNext();) {
                    Person item = i.next();
                    if (item instanceof WalkingPerson) {
                        WalkingPerson item2 = (WalkingPerson) item;
                        item2.attack(ag.mainCharacter, ag.target);
                    }
                }


            }
        });

        Button buttonOne9 = (Button) findViewById(R.id.close9);
        buttonOne9.setOnClickListener(new Button.OnClickListener() {
            public void onClick(View v) {
                ag.mainCharacter.grab("Key", ag.target); // TODO: Select what to grab
            }
        });

        Button buttonOne10 = (Button) findViewById(R.id.close10);
        buttonOne10.setOnClickListener(new Button.OnClickListener() {
            public void onClick(View v) {

                ag.mainCharacter.drop("Key", ag.target); // TODO: Select what to drop


            }
        });

        ListView DynamicListView = new ListView(this);

        final String[] DynamicListElements = new String[] {
                "Hello",
                "Hi",
                "Android Studio",
                "Java"
        };

        ArrayAdapter adapter = new ArrayAdapter (AdventureActivity.this, android.R.layout.simple_list_item_1, DynamicListElements);
        DynamicListView.setAdapter(adapter);



        TableRow element1 = (TableRow) findViewById(R.id.tableRow3);
        element1.addView(DynamicListView);
        ag.mainCharacter.goTo("Town", this);
        ag.display("", this, ag.mainCharacter.place);
    }


    @Override
    protected void onPostCreate(Bundle savedInstanceState) {
        super.onPostCreate(savedInstanceState);

        // Trigger the initial hide() shortly after the activity has been
        // created, to briefly hint to the user that UI controls
        // are available.
        // delayedHide(100);
    }

    private void toggle() {
        if (mVisible) {
            hide();
        } else {
            show();
        }
    }

    private void hide() {
        // Hide UI first
        ActionBar actionBar = getSupportActionBar();
        if (actionBar != null) {
            actionBar.hide();
        }
        mControlsView.setVisibility(View.GONE);
        mVisible = false;

        // Schedule a runnable to remove the status and navigation bar after a delay
        mHideHandler.removeCallbacks(mShowPart2Runnable);
        mHideHandler.postDelayed(mHidePart2Runnable, UI_ANIMATION_DELAY);
    }

    @SuppressLint("InlinedApi")
    private void show() {
        // Show the system bar
        mContentView.setSystemUiVisibility(View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN
                | View.SYSTEM_UI_FLAG_LAYOUT_HIDE_NAVIGATION);
        mVisible = true;

        // Schedule a runnable to display UI elements after a delay
        mHideHandler.removeCallbacks(mHidePart2Runnable);
        mHideHandler.postDelayed(mShowPart2Runnable, UI_ANIMATION_DELAY);
    }

    /**
     * Schedules a call to hide() in delay milliseconds, canceling any
     * previously scheduled calls.
     */
    private void delayedHide(int delayMillis) {
        mHideHandler.removeCallbacks(mHideRunnable);
        mHideHandler.postDelayed(mHideRunnable, delayMillis);
    }
}

class PlaceView extends View {
    public PlaceView(Context context, @Nullable AttributeSet attrs) {
        super(context, attrs);
        target = (AdventureActivity) context;
        this.cont = context;
        int resourceIdFalling = 0;
        int resourceIdHero = 0;
        falling = BitmapFactory.decodeResource(getResources(), resourceIdFalling); //load a falling image
        hero = BitmapFactory.decodeResource(getResources(), resourceIdHero); //load a hero image
    }

    public Collection<Person> getPersons() {
        return persons;
    }

    Person mainCharacter;

    public void setPersons(Collection<Person> persons) {
        this.persons = persons;
    }

    public void display(String s1, AdventureActivity target, Place p) {
        setBackground(p.image);
        target.scrollable.append("This place is called " + p.getName() + ".");
        if(p.getName().equals("Heaven")) {
            final Toast toast = Toast.makeText(target.ag.getContext(), "GAME OVER!\n", Toast.LENGTH_LONG);// duration);
            toast.show();
        }

    }

    /**
     * Persons at this Place.
     */
    public Collection<Person> persons;
    Bitmap falling, hero;

    Context cont;
    TextView text;
    AdventureActivity target;



    // update the canvas in order to display the game action
    @Override
    public void onDraw(Canvas canvas) {

        super.onDraw(canvas);
        int xx = 200;
        int yy = 0;
        if (persons != null) {
            synchronized (persons) {
                Iterator<Person> iterate = persons.iterator();
                while (iterate.hasNext()) {
                    Person p = iterate.next();
                    if (p.getImage() != 0) {
                        hero = BitmapFactory.decodeResource(getResources(), p.getImage()); //load a character image
                        // Draw the visible person's appearance
                        if(xx > canvas.getWidth())
                            xx = 0;
                        canvas.drawBitmap(hero , xx , canvas.getHeight()-hero.getHeight() , null);
                        // Draw the name
                        Paint paint = new Paint();
                        paint.setStyle(Paint.Style.FILL);
                        canvas.save();
                       // canvas.translate(100, 200);
                        paint.setStrokeWidth(1);
                        paint.setColor(Color.WHITE);
                        paint.setTextSize(50);
                        canvas.drawText(p.name, xx-75, (float) (canvas.getHeight() - 0.5*(hero.getHeight())), paint);
                        xx += hero.getWidth()*0.75;
                    }
                }
            }
        }
        canvas.save(); //Save the position of the canvas.
        canvas.restore();
        //Call the next frame.
        invalidate();
    }
}

The entire code is available on github http://www.github.com/montao/adventure

\$\endgroup\$
2
\$\begingroup\$

there are some minor issues that can easily be fixed...

1) you're declaring your view (in onCreate()) twice, funny, isn't it?

ag = findViewById(R.id.view);
ag.text = scrollable;
ag = findViewById(R.id.view);

2) there is much duplicated code for the six direction buttons:

/* skip this duplicated code
buttonOne2.setOnClickListener(new Button.OnClickListener() {
    public void onClick(View v) {
        ag.mainCharacter.go("East", ag.target);
        ag.display("East", ag.target, ag.mainCharacter.place);
    }
});
*/
//use a method instead
buttonOne.setOnClickListener(createMovementButton("North", ag); 
buttonOne2.setOnClickListener(createMovementButton("East", ag); 
...

//use THIS method instead
private Button.OnClickListener createMovementButton(final String dir, final PlaceView ag){ 
    return new Button.OnClickListener() {
        public void onClick(View v) {
            ag.mainCharacter.go(dir, ag.target);
            ag.display(dir, ag.target, ag.mainCharacter.place);
        }
    });
}

NOTE: i really hope that dir "NORTH"is not use as controlling the movement. if it is used for controlling movement you should switch to an Enum over a String to control movement (enum Direction {NORTH, EAST, SOUTH, WEST, UP, DOWN};)

4) make a seperate class-file for your view - there is no need to place it inside your activity.

3) the usage of the synchronized block in your onDraw-method is not optimal. instead of synchronizing the iterator during drawing you should rather synchronize the access while setting:

 public void setPersons(Collection<Person> persons) {
    synchronized (this.persons){
        this.persons = persons;
    }
}

4) don't decode your image during onDraw - encode it once before you ever draw and store the image in an variable and later re-use the variable during drawing.

5) don't call invalidate() in your onDraw method - if you want to create an endless loop use a Handler instead. (see http://www.mopri.de/2010/timertask-bad-do-it-the-android-way-use-a-handler/ for a small tutorial)

\$\endgroup\$
  • 1
    \$\begingroup\$ Even better for the enum: Use properties in it, such as NORTH(0, -1), EAST(1, 0) etc. and an appropriate constructor. \$\endgroup\$ – Simon Forsberg Feb 15 '18 at 15:04
  • \$\begingroup\$ @SimonForsberg that is a very good idea, and honestly i was already thinking about it - i had hoped the OP would write approperiate enums \$\endgroup\$ – Martin Frank Feb 16 '18 at 5:35

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.