Three-in-a-row board game

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 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

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.

-
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.

-
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.

-
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

StackExchange Code Review Q#104015, answer score: 8