HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

First Android game: changing colors of neighboring squares

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
androidsquarescolorsgamefirstchangingneighboring

Problem

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 tiles = new ArrayList();

public static void init() {
List tiles = Field.tiles;
int total = GlobalValues.total;
int column = GlobalValues.column;
int row = total / column;
for (int i = 0, c = 0, r = 0; i 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

Solution

public static void init() {
    List 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;

Code Snippets

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);
        }
    }
}
for (int i = 0, c = 0, r = 0; i < total; i++, c++) {
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);
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);
            //...
        }
    }
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);
            //...
        }
    }

Context

StackExchange Code Review Q#118899, answer score: 2

Revisions (0)

No revisions yet.