Three-in-a-row board game

Question

I don't truly understand how OOP works and It's stopping me from creating good programs! Below I have a program that will check to see if there are 3 tiles from a gridview in a row.

Here's a screenshot of the game.

Basically you're not supposed to get 3 of the same colours in a row! But at the moment I've been adding in the basic functionality. So eventually Im going to add in methods to restore the gameboard to grey, start a timer to see how quickly they can fill the board up without getting 3 in a row, etc. So this is why Im really wanting to understand how else I could write this part of my program!

At the moment it's all in the one class (game.java) which I don't think is very OO. The Items() class will just set the tile to a different colour!

package com.example.assignment;

import android.app.Activity;
import android.content.Intent;
import android.os.Bundle;
import android.util.Log;
import android.view.Menu;
import android.view.MenuItem;
import android.view.View;
import android.widget.AdapterView;
import android.widget.GridView;
import android.widget.ImageView;
import android.widget.Toast;
import android.widget.AdapterView.OnItemClickListener;

public class Game extends Activity {
    GridView gridView;
    Item[] gridArray = new Item[16];
    ImageAdapter iAdapter;
    /*
     * Here is the amount of collumns in the app - This is used to get the y and
     * x position
     */
    int columncount = 4;

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

        // 4X4 array
        for (int i = 0; i < 16; i++) {
            gridArray[i] = new Item(R.drawable.grey, "grey", i);
        }
        GridView gridview = (GridView) findViewById(R.id.gridview);
        iAdapter = new ImageAdapter(this, gridArray);
        gridview.setAdapter(iAdapter);
        gridview.setOnItemClickListener(new OnItemClickListener() {
            public void onItemClick(AdapterView<?> parent, View v,
                    int position, long id) {

                /*
                 * Here is where we get the clicked position Simply divide the
                 * position by the columncount (9) to get the x and do the same
                 * for the y but instead change the divide to a modulus which
                 * will get the remainder note: use this for the assignment
                 */

                int x = position / columncount;
                int y = position % columncount;

                int c = gridArray[position].nextColor();

                // Run the checkWinner method that will check if
                checkWinner();

                ((ImageView) v).setImageResource(c);
                /*
                 * If you want to display the collumns/rows not based on a 0
                 * index then use this toast
                 * Toast.makeText(getApplicationContext(),(x+1) + " " +
                 * (y+1),Toast.LENGTH_SHORT).show(); All this does
                 */
                Toast.makeText(getApplicationContext(), x + " " + y,
                        Toast.LENGTH_SHORT).show();

            }
        });
    }

    @Override
    public boolean onCreateOptionsMenu(Menu menu) {
        // Inflate the menu; this adds items to the action bar if it is present.
        getMenuInflater().inflate(R.menu.main, menu);
        return true;

    }

    public void checkWinner() {

        // Check for horizontal loss
        for (int i = 0; i < 16; i += 4) {

            // Check for white
            if ((gridArray[i].getRDC() == R.drawable.white
                    && gridArray[i + 1].getRDC() == R.drawable.white && gridArray[i + 2]
                    .getRDC() == R.drawable.white)
                    || (gridArray[i + 1].getRDC() == R.drawable.white
                            && gridArray[i + 2].getRDC() == R.drawable.white && gridArray[i + 3]
                            .getRDC() == R.drawable.white)) {
                Toast.makeText(getApplicationContext(),
                        "THREE WHITE IN A ROW HORIZONTALLY", Toast.LENGTH_SHORT)
                        .show();
                Log.d("MyApp", "3 in row horizontally WHITE");
            }

            // Check for red
            if ((gridArray[i].getRDC() == R.drawable.red
                    && gridArray[i + 1].getRDC() == R.drawable.red && gridArray[i + 2]
                    .getRDC() == R.drawable.red)
                    || (gridArray[i + 1].getRDC() == R.drawable.white
                            && gridArray[i + 2].getRDC() == R.drawable.red && gridArray[i + 3]
                            .getRDC() == R.drawable.red)) {
                Toast.makeText(getApplicationContext(),
                        "THREE RED IN A ROW HORIZONTALLY", Toast.LENGTH_SHORT)
                        .show();
                Log.d("MyApp", "3 in row horizontally RED");
            }

        }

        // Check for vertical loss
        for (int i = 0; i <= 7; i++) {
            /*
             * This for loop is looping through each column and checking if 3 drawables are in a row
             */

            //Check for white loss
            if (gridArray[i].getRDC() == R.drawable.white
                    && gridArray[i + 4].getRDC() == R.drawable.white
                    && gridArray[i + 8].getRDC() == R.drawable.white) {
                Toast.makeText(getApplicationContext(),
                        "THREE WHITE IN A ROW VERTICALLY", Toast.LENGTH_SHORT).show();
                Log.d("MyApp", "3 in row vertically WHITE");
            }

            //Check for red loss
            if (gridArray[i].getRDC() == R.drawable.red
                    && gridArray[i + 4].getRDC() == R.drawable.red
                    && gridArray[i + 8].getRDC() == R.drawable.red) {
                Toast.makeText(getApplicationContext(),
                        "THREE RED IN A ROW VERTICALLY", Toast.LENGTH_SHORT).show();
                Log.d("MyApp", "3 in row vertically RED");
            }
        }
    }

    @Override
    public boolean onOptionsItemSelected(MenuItem item) {
        Intent i;
        switch (item.getItemId()) {
        case R.id.homeMenuItem:
            i = new Intent(this, Main.class);
            startActivity(i);
            return true;
        case R.id.settingMenuItem:
            i = new Intent(this, Setting.class);
            startActivity(i);
            return true;
        case R.id.highscoreMenuItem:
            i = new Intent(this, Highscores.class);
            startActivity(i);
            return true;
        case R.id.gamesLabel:
            i = new Intent(this, Game.class);
            startActivity(i);
            return true;
        case R.id.helpLabel:
            i = new Intent(this, help.class);
            startActivity(i);
            return true;
        }
        return false;
    }
}

Answer 1

About the way you write your code...

There's a type of array that you can use for a grid already built into the language. It's the 2D array:

// What you see below is a 2D array of size 4x4.
Item[][] grid = new Item[4][4];

Read up on them.

int columncount = 4; becomes obsolete, check out Array.size.

// 4X4 array
for (int i = 0; i < 16; i++) {
    gridArray[i] = new Item(R.drawable.grey, "grey", i);
}

Two things: First, don't hardcode the sizes like that, but if you've read up on Array.size and what I mentioned earlier, you should already have a solution for this. Second, don't leave comments like // 4x4 array. They're at least redundant since the code shows (or at least should) provide this information clearly, and at worst misleading as time passes and the code changes without the comments catching up.

Make a blacklist of words that should trigger flags when they appear in comments: magic numbers (unless explaining an ugly hack), temporal indicatives (e.g. "this is called after X"), previews of future changes (e.g. "this will be removed in a future commit"), and TODOs. I might have forgotten some, but generally if you see something like this, it's probably outdated and misleading. If you're writing something like this, you need to take extra care so it doesn't become outdated and misleading. It's best to use anything other than clean code as a last resort.

 /*
 * Here is where we get the clicked position Simply divide the
 * position by the columncount (9) to get the x and do the same
* for the y but instead change the divide to a modulus which
* will get the remainder note: use this for the assignment
*/

Too much useless info, this can be easily derived from code.

You need to get into the habit of extracting methods and functions. onCreatecan have setUpGrid, setUpGridView extracted, at least.

@Override
protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.game);
    setUpGrid();
    setUpGridView();
}

In checkWinner you're duplicating a lot of ugly code. Note how the only difference between checking for white winning and red winning is the color. Extract the check into a function

boolean playerWithColorMeetsWinningCondition(Color color) {
     ...
}

and call it like this:

if (playerWithColorMeetsWinningCondition(R.drawable.white)) {
      //white wins.
}

your conditions there are horribly long because you've hardcoded the sizes and checking each element manually. I'm going to stop here even though there's a lot more to say.

Pick up a Java book before attempting to do anything I mentioned at least and don't let it go until you finish all of the exercises in it, and learn everything it teaches. It will pay off, since you seem to lack a basic understanding of programming here.

Be sure to come back with a revised version of your game after you're done learning so that we can have another round at it and praise you for your progress :)

---

2nd part, but you probably don't want to know this since it looks like you're doing an assignment.

With the risk of seeming like overengineering a simple game...

One thing that you should focus on when writing OO code is to make sure you split your logic into a collection of classes that have very well defined and ideally single responsibilities. As you stated, throwing everything inside one class like you did is not very clean to work with. Imagine that the instances of your classes are things that are experts at specific things, and must work together to achieve one goal. While at it, you should learn about SOLID.

Here's the usual architecture that I adopt whenever I deal with simple board games. It should give you an idea about how to lay down your code. Read the 2nd part first though.

1. The board's view. Its responsibility is to output things to the human player and send his input down the line. This is just an exact representation of the current board state. No logic, no fancy stuff, just something that's in sync, in some way, with the internal representation for the board. What you call Game should be called GameActivity and should represent your view. Hint: adapters like the one you're currently using make it easy to sync the view with the model (the internal board representation). Take a look at MVC and other similar patterns, it's easy to understand and fits boardgames well.

2. A class called Game that ties all other subcomponents and defines how they interact with each other; it also contains the actual game loop. So, its responsibility is to get the actual game running and let the game components do their thing.

3. A class called GameBoard which is the internal representation of the game board. It only keeps track of, say, the grid, and the state of each cell in it. Think of it as your array of Items wrapped up in a class providing nice ways to inspect and alter the state. Responsibility: to keep track of the game board's current state.

4. An interface for players, call it Player. Game will also keep track of the current Players (classes which implement Player). Different types of players will have different behaviors but all will implement the same interface. HumanPlayer will take action when user input occurs. AIPlayer will take action after it finishes thinking about the current GameBoard and what the next best move would be. Many MockPlayers would be a great way to unit test your game by covering specific situations and behaviors only. Game decides their turn taking order and so on. Responsibility: to establish a common set of operations that take place whenever a player is supposed to take action. Each implementer provides its own way of taking action. Here's one:

   class AIPlayer implements Player
   {
         ...
         @Override
         public void MakeMove(GameBoard currentBoard)
         {
               Action bestAction = PickBestAction(currentBoard);
               // You want some form of callback here. 
               // The human player will call it on GUI input.
               // You might also want to make the AI "think" on a separate
               // thread.
               playerListener.playerReadyToTakeAction(this, bestAction);
         }
   }
   

5. You want to encapsulate the basic actions that can be performed on your GameBoard. These include actions that alter the board, or just inspect it (query). For example, Color ColorForTileAt(...) is an inspector, while void ChangeColorForTileAt(...) is a mutator. Players use these actions on the board given to them. They're the only way to work on the board, and you want them to be decoupled from the players so that you can test actions in isolation.

Answer 2

It looks like Alex M. is already writing a good review, but I just want to add a "trick" I use when coding some project which has some graphical output.

I write my code in such a way that the code can easily be ported from one interface to another. For example, it could be made to run on Android, Swing, JavaFX, or even just a text grid in the console. In order to do that you have to decouple your game logic from the API of the interface. You have to define your game logic as separate classes.