4
\$\begingroup\$

I have created a small game to learn more about Android.

So the game is you start with a single gray square, and you try to take over neighboring squares. You can only attack 1 color per move, but each move takes any neighboring squares of that color and also turns them gray. Turn the whole field gray to win.

My main concern is inefficient use of the Android APK. There is probably a much better way to change colors of blocks than referencing a different object. I just couldn't seem to find a good way to change colors.

Field:

package com.timcorp.timotheus.colorgame;
import android.app.Activity;
import android.content.Intent;
import android.util.Log;
import android.view.Gravity;
import android.view.View;
import android.widget.GridLayout;
import android.widget.ImageView;
import android.widget.TextView;
import android.widget.Toast;

import java.util.ArrayList;
import java.util.List;
import java.util.Random;

public class Field {
    public static List<Tile> tiles = new ArrayList<Tile>();

    public static void init() {
        List<Tile> tiles = Field.tiles;
        int total = GlobalValues.total;
        int column = GlobalValues.column;
        int row = total / column;
        for (int i = 0, c = 0, r = 0; i < total; i++, c++) {
            if (c == column) {
                c = 0;
                r++;
            }
            Tile t = new Tile(c, r);
            Random rand = new Random();
            int rand3 = rand.nextInt(GlobalValues.numberOfColors) + 1;
            //GlobalValues.Colors randColor = GlobalValues.Colors.values()[rand3];
            switch (rand3) {
                case 1:
                    t.color = GlobalValues.Colors.red;
                    break;
                case 2:
                    t.color = GlobalValues.Colors.green;
                    break;
                case 3:
                    t.color = GlobalValues.Colors.blue;
                    break;
                case 4:
                    t.color = GlobalValues.Colors.yellow;
                    break;
                case 5:
                    t.color = GlobalValues.Colors.purple;
                    break;
                case 6:
                    t.color = GlobalValues.Colors.cyan;
                    break;
                default:
                    break;
            }
            tiles.add(t);
        }
        //set first tile and all same color tiles to selected
        for (Tile t : Field.tiles) {
            if (t.selected) {
                Field.evalTile(t, t.color);
            }
        }
    }

    public static void init(String layout) {
        List<Tile> tiles = Field.tiles;
        int total = GlobalValues.total;
        int column = GlobalValues.column;
        int row = total / column;
        layout = layout.replaceAll("\\s", "");
        int c = 0;
        int r = 0;
        for (char ch : layout.toCharArray()) {
            if (c == column) {
                c = 0;
                r++;
            }
            Tile t = new Tile(c, r);
            switch (ch) {
                case 'r':
                    t.color = GlobalValues.Colors.red;
                    break;
                case 'g':
                    t.color = GlobalValues.Colors.green;
                    break;
                case 'b':
                    t.color = GlobalValues.Colors.blue;
                    break;
                case 'y':
                    t.color = GlobalValues.Colors.yellow;
                    break;
                case 'p':
                    t.color = GlobalValues.Colors.purple;
                    break;
                case 'c':
                    t.color = GlobalValues.Colors.cyan;
                    break;
                default:
                    break;
            }
            tiles.add(t);
            c++;
        }
        //set first tile and all same color tiles to selected
        for (Tile t : Field.tiles) {
            if (t.selected) {
                Field.evalTile(t, t.color);
            }
        }
    }

    public static void evalNeighbors(Tile tile, GlobalValues.Colors color) {
        //North
        Tile n = getTile(tile.x, tile.y - 1);
        if(n != null)
            if(!n.selected)
                evalTile(n, color);
        //South
        Tile s = getTile(tile.x, tile.y + 1);
        if(s != null)
            if(!s.selected)
                evalTile(s, color);
        //West
        Tile w = getTile(tile.x - 1, tile.y);
        if(w != null)
            if(!w.selected)
                evalTile(w, color);
        //East
        Tile e = getTile(tile.x + 1, tile.y);
        if(e != null)
            if(!e.selected)
                evalTile(e, color);
    }

    public static Tile getTile(int X, int Y)
    {
        for(Tile t : tiles) {
            if (t.x == X && t.y == Y)
                return t;
        }
        return null;
    }

    public static void evalTile(Tile tile, GlobalValues.Colors color) {
        if(tile.color == color) {
            tile.selected = true;
            tile.changeColor(GlobalValues.Colors.selected);
            evalNeighbors(tile, color);
        }
    }

    public String ToString() //change to real string later
    {
        String s = new String();
        for (Tile t : Field.tiles) {
            s += t.color.toString().substring(0, 1) + " ";
            if (t.x == GlobalValues.column - 1)
                s += "\n";
        }
        return s;
    }

    public List<Tile> getSelectedList() {
        ArrayList<Tile> l = new ArrayList<Tile>();
        for (Tile t : Field.tiles) {
            if (t.selected)
                l.add(t);
        }
        return l;
    }


}

This is the UI:

package com.timcorp.timotheus.colorgame;

import android.app.ActionBar;
import android.app.Activity;
import android.content.Intent;
import android.graphics.Color;
import android.graphics.PorterDuff;
import android.util.Log;
import android.view.Gravity;
import android.view.View;
import android.widget.Button;
import android.widget.GridLayout;
import android.widget.ImageButton;
import android.widget.ImageView;
import android.widget.LinearLayout;
import android.widget.TableLayout;
import android.widget.TableRow;
import android.widget.TextView;
import android.widget.Toast;

import java.util.List;
import java.util.Random;
public class UI {
    public static MainActivity mainActivity;
    public static GridLayout gridLayout;
    public static GridLayout buttonsLayout;
    public static Field field = new Field();
    public static GlobalValues.Colors LastPressed;
    public static void UpdateColor(View view) {
        GlobalValues.moves++;
        TextView tv = (TextView)mainActivity.findViewById(R.id.moves);
        tv.setText("Moves: " + GlobalValues.moves);
        for (Tile t : field.getSelectedList()) {
            switch (view.getId()) {
                case 0:
                    Field.evalNeighbors(t, GlobalValues.Colors.red);
                    lockButton(GlobalValues.Colors.red);
                    break;
                case 1:
                    Field.evalNeighbors(t, GlobalValues.Colors.green);
                    lockButton(GlobalValues.Colors.green);
                    break;
                case 2:
                    Field.evalNeighbors(t, GlobalValues.Colors.blue);
                    lockButton(GlobalValues.Colors.blue);
                    break;
                case 3:
                    Field.evalNeighbors(t, GlobalValues.Colors.yellow);
                    lockButton(GlobalValues.Colors.yellow);
                    break;
                case 4:
                    Field.evalNeighbors(t, GlobalValues.Colors.purple);
                    lockButton(GlobalValues.Colors.purple);
                    break;
                case 5:
                    Field.evalNeighbors(t, GlobalValues.Colors.cyan);
                    lockButton(GlobalValues.Colors.cyan);
                    break;
            }
        }
        //update all new selected
        for (Tile t : field.getSelectedList()) {
            t.changeColor(GlobalValues.Colors.selected);
            t.image.setImageResource(Utilities.getImageColor(GlobalValues.Colors.selected));
        }
        if(Utilities.won(Field.tiles)) {
            Toast.makeText(mainActivity, "You won in " + GlobalValues.moves + " moves!",
                    Toast.LENGTH_SHORT).show();
            Utilities.resetStage();
            Intent intent = mainActivity.getIntent();
            mainActivity.finish();
            mainActivity.startActivity(intent);
        }
    }

    public static void init(Activity th) {
        gridLayout.removeAllViews();
        field.init();
        setButtons();
        int total = GlobalValues.total;
        int column = GlobalValues.column;
        int row = total / column;
        gridLayout.setColumnCount(column);
        gridLayout.setRowCount(row + 1);
        for (Tile t : field.tiles)
        {
            ImageView oImageView = new ImageView(th);
            //GlobalValues.Colors randColor = GlobalValues.Colors.values()[rand3];
            switch (t.color)
            {
                case red:
                    oImageView.setImageResource(R.drawable.red);
                    break;
                case green:
                    oImageView.setImageResource(R.drawable.green);
                    break;
                case blue:
                    oImageView.setImageResource(R.drawable.blue);
                    break;
                case yellow:
                    oImageView.setImageResource(R.drawable.yellow);
                    break;
                case purple:
                    oImageView.setImageResource(R.drawable.purple);
                    break;
                case cyan:
                    oImageView.setImageResource(R.drawable.cyan);
                    break;
                case selected:
                    oImageView.setImageResource(R.drawable.selected);
                    break;
                default:
                    break;
            }

            GridLayout.LayoutParams param =new GridLayout.LayoutParams();
            param.height = GridLayout.LayoutParams.WRAP_CONTENT;
            param.width = GridLayout.LayoutParams.WRAP_CONTENT;
            //param.rightMargin = 5;
            //param.topMargin = 5;
            param.setGravity(Gravity.CENTER);
            param.columnSpec = GridLayout.spec(t.x);
            param.rowSpec = GridLayout.spec(t.y);
            oImageView.setLayoutParams(param);
            gridLayout.addView(oImageView);
            t.image = oImageView;
        }

        //set first tile and all same color tiles to selected
        for (Tile t : Field.tiles) {
            if (t.selected) {
                Field.evalTile(t, t.color);
            }
        }
    }

    private static void lockButton(GlobalValues.Colors color) {
        ImageButton b;

        switch (color) {
            case red:
                b = (ImageButton) mainActivity.findViewById(0);
                enableAllButtons();
                b.setEnabled(false);
                LastPressed = GlobalValues.Colors.red;
                break;
            case green:
                b = (ImageButton) mainActivity.findViewById(1);
                enableAllButtons();
                b.setEnabled(false);
                //b.setBackground(R.drawable.selected);
                LastPressed = GlobalValues.Colors.green;
                break;
            case blue:
                b = (ImageButton) mainActivity.findViewById(2);
                enableAllButtons();
                b.setEnabled(false);
                LastPressed = GlobalValues.Colors.blue;
                break;
            case yellow:
                b = (ImageButton) mainActivity.findViewById(3);
                enableAllButtons();
                b.setEnabled(false);
                LastPressed = GlobalValues.Colors.yellow;
                break;
            case purple:
                b = (ImageButton) mainActivity.findViewById(4);
                enableAllButtons();
                b.setEnabled(false);
                LastPressed = GlobalValues.Colors.purple;
                break;
            case cyan:
                b = (ImageButton) mainActivity.findViewById(5);
                enableAllButtons();
                b.setEnabled(false);
                LastPressed = GlobalValues.Colors.cyan;
                break;
            default:
                break;
        }
    }

    private static void enableAllButtons() {
        for (int i = 0; i < GlobalValues.numberOfColors; i++) {
            ImageButton b = (ImageButton) mainActivity.findViewById(i);
            b.setEnabled(true);
        }
    }

    private static void setButtons() {
        for (int i = 0; i < GlobalValues.numberOfColors; i++) {
            ImageButton b = new ImageButton(mainActivity);
            //ImageView IV = new ImageView(mainActivity);
            switch (i) {
                case 0:
                    b.setImageResource(R.drawable.red);
                    b.setId(i);
                    break;
                case 1:
                    b.setImageResource(R.drawable.green);
                    b.setId(i);
                    break;
                case 2:
                    b.setImageResource(R.drawable.blue);
                    b.setId(i);
                    break;
                case 3:
                    b.setImageResource(R.drawable.yellow);
                    b.setId(i);
                    break;
                case 4:
                    b.setImageResource(R.drawable.purple);
                    b.setId(i);
                    break;
                case 5:
                    b.setImageResource(R.drawable.cyan);
                    b.setId(i);
                    break;
                default:
                    break;
            }
            //b.setBackgroundDrawable(IV.getDrawable());
            b.setOnClickListener(new Button.OnClickListener() {
                public void onClick(View view) {
                    UI.UpdateColor(view);
                }
            });
            buttonsLayout.addView(b);
        }
    }
}

Utils:

package com.timcorp.timotheus.colorgame;

import java.util.ArrayList;
import java.util.List;

public class Utilities {

    public static int getImageColor(GlobalValues.Colors color) {
        if(color == GlobalValues.Colors.red)
            return R.drawable.red;
        if(color == GlobalValues.Colors.green)
            return R.drawable.green;
        if(color == GlobalValues.Colors.blue)
            return R.drawable.blue;
        if(color == GlobalValues.Colors.selected)
            return R.drawable.selected;
        return R.drawable.green;//Replace later
    }

    public static boolean won (List<Tile> tiles) {
        for (Tile t : tiles) {
            if (!t.selected)
                return false;
        }
        return true;
    }
    public static void resetStage()
    {
        if(GlobalValues.best > GlobalValues.moves)
            GlobalValues.best = GlobalValues.moves;
        GlobalValues.moves = 0;
        Field.tiles =  new ArrayList<Tile>();
    }

    public static void clearStage() {
        GlobalValues.moves = 0;
        Field.tiles = new ArrayList<Tile>();
    }
}

Tile Class:

package com.timcorp.timotheus.colorgame;

import android.graphics.drawable.Drawable;
import android.widget.ImageView;


public class Tile {
    public boolean selected;
    public int x;
    public int y;
    public GlobalValues.Colors color;
    public ImageView image;
    public Tile(int X, int Y)
    {
        this.x = X;
        this.y = Y;
        if(this.x==0 && this.y == 0)
            this.selected = true;
        else
            this.selected = false;
    }
    public void changeColor(GlobalValues.Colors color)
    {
        this.color = color;
    }
    @Override
    public String toString()
    {
        return "(" + this.x + "," + this.y + ") " + this.color + " " + this.selected;
    }
}

Menu Class:

package com.timcorp.timotheus.colorgame;

import android.content.Intent;
import android.support.v7.app.AppCompatActivity;
import android.os.Bundle;
import android.view.View;
import android.widget.ArrayAdapter;
import android.widget.Spinner;
import android.widget.Toast;

public class MenuActivity extends AppCompatActivity {

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_menu);

        Spinner spinner = (Spinner) findViewById(R.id.spinner);
        ArrayAdapter<CharSequence> adapter = ArrayAdapter.createFromResource(this,
                R.array.numberOfColors, android.R.layout.simple_spinner_item);
        adapter.setDropDownViewResource(android.R.layout.simple_spinner_dropdown_item);
        spinner.setAdapter(adapter);
        spinner.setSelection(1);
    }

    public void difClick(View view) {
        switch (view.getId()) {
            case R.id.Easy:
                Spinner spinner = (Spinner) findViewById(R.id.spinner);
                Intent i = new Intent(MenuActivity.this, MainActivity.class);
                i.putExtra("NUM_COLORS", spinner.getSelectedItem().toString());
                startActivity(i);
                break;
        }
    }
}

Main Class:

package com.timcorp.timotheus.colorgame;

import android.content.Intent;
import android.os.Bundle;
import android.support.v7.app.AppCompatActivity;
import android.util.Log;
import android.view.Gravity;
import android.view.View;
import android.widget.GridLayout;
import android.widget.ImageView;
import android.widget.TableLayout;
import android.widget.TextView;
import android.widget.Toast;

import java.util.List;
import java.util.Random;

public class MainActivity extends AppCompatActivity {

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        Utilities.clearStage();
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);
        GlobalValues.numberOfColors = Integer.parseInt(getIntent().getStringExtra("NUM_COLORS"));
        GridLayout gridLayout = (GridLayout) findViewById(R.id.gridTable);
        GridLayout buttonsLayout = (GridLayout) findViewById(R.id.buttonsLayout);
        UI.gridLayout = gridLayout;
        UI.mainActivity = this;
        UI.buttonsLayout = buttonsLayout;
        UI.init(this);

        TextView tv = (TextView)findViewById(R.id.best);
        tv.setText("Best: " + GlobalValues.best);
    }
}

Global Class:

package com.timcorp.timotheus.colorgame;

import java.util.ArrayList;
import java.util.List;
public class GlobalValues {
    public enum Colors {red, green, blue, yellow, purple, cyan, selected}

    public static int numberOfColors = 3;
    public final static int total = 100;
    public final static int column = 10;

    public static int moves = 0;
    public static int best = 50;
}
\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$
public static void init() {
    List<Tile> tiles = Field.tiles;
    int total = GlobalValues.total;
    int column = GlobalValues.column;
    int row = total / column;
    for (int i = 0, c = 0, r = 0; i < total; i++, c++) {
        if (c == column) {
            c = 0;
            r++;
        }
        Tile t = new Tile(c, r);
        Random rand = new Random();
        int rand3 = rand.nextInt(GlobalValues.numberOfColors) + 1;
        //GlobalValues.Colors randColor = GlobalValues.Colors.values()[rand3];
        switch (rand3) {
            case 1:
                t.color = GlobalValues.Colors.red;
                break;
            case 2:
                t.color = GlobalValues.Colors.green;
                break;
            case 3:
                t.color = GlobalValues.Colors.blue;
                break;
            case 4:
                t.color = GlobalValues.Colors.yellow;
                break;
            case 5:
                t.color = GlobalValues.Colors.purple;
                break;
            case 6:
                t.color = GlobalValues.Colors.cyan;
                break;
            default:
                break;
        }
        tiles.add(t);
    }
    //set first tile and all same color tiles to selected
    for (Tile t : Field.tiles) {
        if (t.selected) {
            Field.evalTile(t, t.color);
        }
    }
}

This initialization code could do with some improvements.

My main point here is this line:

    for (int i = 0, c = 0, r = 0; i < total; i++, c++) {

I see this line and think "okaaaaay.... what do all these variables mean?". It's hard to understand straight away. You want to write the code in such a way that it is obvious how the code works.

    int total = GlobalValues.total;
    int column = GlobalValues.column;
    int row = total / column;
    for (int i = 0, c = 0, r = 0; i < total; i++, c++) {
        if (c == column) {
            c = 0;
            r++;
        }
        Tile t = new Tile(c, r);

Looking at this segment, you seem to be iterating over a set of columns and then rows in order to make tiles. Rules-as-written, the game seems to work on a set of tiles that doesn't have to form a square, as you could have a total of "14" and a column count of "4".

But you do work with columns and rows to make some sort of 2D playing field, so why not use two for loops?

    final int total = GlobalValues.total;
    int column = GlobalValues.column;
    int row = total / column;
    for (int i = 0, r = 0; i < total; r++) {
        for (int c = 0; c < column && i < total; c++, i++) {
            Tile t = new Tile(c, r);
            //...
        }
    }

This doesn't make things a lot clearer, though.

You have a row variable that doesn't even get used, and a column variable which indicates the maximum amount of columns. If we use better variable names...

    final int totalTiles = GlobalValues.total;
    int maxColumns = GlobalValues.column;
    for (int i = 0, row = 0; i < totalTiles; row++) {
        for (int column = 0; c < maxColumns && i < totalTiles; column++, i++) {
            Tile tile = new Tile(column, row);
            //...
        }
    }

It's a bit easier to read, now.

Alternatively, use maths:

  final int totalTiles = GlobalValues.total;
  final int maxColumns = GlobalValues.column;
  for (int i = 0; i < totalTiles; i++) {
      Tile tile = new Tile(i % maxColumns, i / maxColumns);
      //...
  }

This makes it harder to see which argument is the column and which is the row, though.


        Random rand = new Random();
        int rand3 = rand.nextInt(GlobalValues.numberOfColors) + 1;
        //GlobalValues.Colors randColor = GlobalValues.Colors.values()[rand3];
        switch (rand3) {
            case 1:
                t.color = GlobalValues.Colors.red;
                break;
            case 2:
                t.color = GlobalValues.Colors.green;
                break;
            case 3:
                t.color = GlobalValues.Colors.blue;
                break;
            case 4:
                t.color = GlobalValues.Colors.yellow;
                break;
            case 5:
                t.color = GlobalValues.Colors.purple;
                break;
            case 6:
                t.color = GlobalValues.Colors.cyan;
                break;
            default:
                break;
        }

You should put most of this (the switch statement specifically) in the Colors enumeration itself. Something like "fromNumber", which takes the random number and returns a color.

Same goes for this section:

        switch (ch) {
            case 'r':
                t.color = GlobalValues.Colors.red;
                break;
            case 'g':
                t.color = GlobalValues.Colors.green;
                break;
            case 'b':
                t.color = GlobalValues.Colors.blue;
                break;
            case 'y':
                t.color = GlobalValues.Colors.yellow;
                break;
            case 'p':
                t.color = GlobalValues.Colors.purple;
                break;
            case 'c':
                t.color = GlobalValues.Colors.cyan;
                break;
            default:
                break;
        }

fromChar seems a good name.


public static void evalNeighbors(Tile tile, GlobalValues.Colors color) {
    //North
    Tile n = getTile(tile.x, tile.y - 1);
    if(n != null)
        if(!n.selected)
            evalTile(n, color);
    //South
    Tile s = getTile(tile.x, tile.y + 1);
    if(s != null)
        if(!s.selected)
            evalTile(s, color);
    //West
    Tile w = getTile(tile.x - 1, tile.y);
    if(w != null)
        if(!w.selected)
            evalTile(w, color);
    //East
    Tile e = getTile(tile.x + 1, tile.y);
    if(e != null)
        if(!e.selected)
            evalTile(e, color);
}

Smoosh the if's together with &&, as the nesting doesn't help with understanding.

public static void evalNeighbors(Tile tile, GlobalValues.Colors color) {
    //North
    Tile n = getTile(tile.x, tile.y - 1);
    if(n != null && !n.selected)
        evalTile(n, color);
    //South
    Tile s = getTile(tile.x, tile.y + 1);
    if(s != null && !s.selected)
        evalTile(s, color);
    //West
    Tile w = getTile(tile.x - 1, tile.y);
    if(w != null && !w.selected)
        evalTile(w, color);
    //East
    Tile e = getTile(tile.x + 1, tile.y);
    if(e != null && !e.selected)
        evalTile(e, color);
}

Prevent duplication by migrating the check to a helper function.

public static void evalNeighbors(Tile tile, GlobalValues.Colors color) {
    //North
    Tile n = getTile(tile.x, tile.y - 1);
    evalTileIfNeeded(n, color);
    //South
    Tile s = getTile(tile.x, tile.y + 1);
    evalTileIfNeeded(s, color);
    //West
    Tile w = getTile(tile.x - 1, tile.y);
    evalTileIfNeeded(w, color);
    //East
    Tile e = getTile(tile.x + 1, tile.y);
    evalTileIfNeeded(e, color);
}

private static void evalTileIfNeeded(Tile tile, GlobalValues.Colors color) {
    if (tile != null && !tile.selected) {
        evalTile(tile, color);
    }
}

Instead of putting the description of the tile in the comment, put it in the variable name. This binds the meaning of the variable to the variable.

public static void evalNeighbors(Tile tile, GlobalValues.Colors color) {
    Tile north = getTile(tile.x, tile.y - 1);
    evalTileIfNeeded(north, color);

    Tile south = getTile(tile.x, tile.y + 1);
    evalTileIfNeeded(south, color);

    Tile west = getTile(tile.x - 1, tile.y);
    evalTileIfNeeded(west, color);

    Tile east = getTile(tile.x + 1, tile.y);
    evalTileIfNeeded(east, color);
}

private static void evalTileIfNeeded(Tile tile, GlobalValues.Colors color) {
    if (tile != null && !tile.selected) {
        evalTile(tile, color);
    }
}

    if(this.x==0 && this.y == 0)
        this.selected = true;
    else
        this.selected = false;

This is in your Tile constructor. You can simplify this to

this.selected = (this.x == 0 && this.y == 0);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.