Find longest sequence horizontally, vertically or diagonally in Connect Four game

I'm new to programming and also to Java and working on problems in Intro to Java by Robert Sedgewick. Here is my question:


Connect Four: Given an N-by-N grid with each cell either occupied by
an 'X', an 'O', or empty, write a program to find the longest sequence
of consecutive 'X's either horizontal, vertically, or diagonally. To
test your program, you can create a random grid where each cell
contains an 'X' or 'O' with probability 1/3.

With many modifications, I came up with some code, which I feel is not efficient. Can someone help make my code more efficient?

public class connectfour {

public static void main(String[] args) {

int N=Integer.parseInt(args[0]),t=0;

String[][] a=new String[N][N];
for(int i=0;i=0;i--){
            for(int j=0;j0&&y<N-1;x--,y++)
                    {if(a[x][y]!=a[x-1][y+1])
                        break;
                    length++;
                    if(t<length) t=length;
                    }
                }
            }
        }

  System.out.println("the length of longest sequence of X in the above array: "+t);

  }
  }

I want to post my improved code according to instructions given above. I implemented the algorithm suggested by Simon André Forsberg. It is working for all cases.

```
public class connectfour2 {
public static void main(String[] args) {

int N = Integer.parseInt(args[0]), highestconsecutive = 0;

String string1, string2 = "X", string3;

String[][] board = new String[N][N];

for(int i = 0; i = 0 ; x++, y-- )

{
string1 = board[x][y];

if(string1.equals(string2))
length++;

else
length=0;

if(highe

Introduction

Hi and Welcome to Code Review! There are number of things that you can learn today.

First I would like to say that it is nice that your algorithm works. Thanks for providing a compilable example, that helps a lot.

Now, secondly, I have to disappoint you: I will not simply "give you the better code". I don't really think you would learn so much from that. However, I can help you in figuring out what things needs to be improved in your current code.
List of improvements
YOUR CODE IS NOT READABLE!

Sorry for shouting but I am very serious. The importance of code readability cannot be emphasized enough. The readability of your code is severely off.

-
Indentation: Your indentation is not consistent. Each { should be followed by one extra indentation step, and each } should remove one indentation step.

-
Spacing: It seems like you are always using as few spaces as possible. If you are having serious storage problems and are running out of bytes, then... no, I wouldn't understand it even then...

Compare

else if(r<0.66)a[i][j]="O";

with

else if(r < 0.66) a[i][j] = "O";

And compare

for(int x=i,y=j,length=1;x<N-1&&y<N-1;x++,y++)

with

for (int x = i, y = j, length = 1; x < N - 1 && y < N - 1; x++, y++)

Spacing is good for you. One space after each comma, one after semicolon, one before & after = and &&. Makes things so much readable. OrhowwouldyoulikeitifIwrotemyreviewlikethis,withoutusingspacesatall? (I hope that example made things clear why spacing is good).

Please read up on the Java coding conventions, all these things are mentioned there.

Once you have learned how to improved those, if you are using an IDE such as Eclipse, which I hope that you do - if you are not I really suggest that you download Eclipse now. Press Ctrl + Shift + F in Eclipse to make it format for you. If you are using NetBeans, press Alt + Shift + F.

• Variable names: All (except one) of your variable names is only one character. Try to have self-documenting variable names. What is the variable used for? row and col could be better names than i and j. t could be called maximumFoundLength.

String comparison

It is more or less pure luck that your code works at all. Thanks to Java only creating one String instance for "X" and such, it works with comparing your Strings with ==. However, if you would have any user input, this wouldn't work. == compares object references, to compare Strings correctly in Java you should use the .equals method. Please see the Stack Overflow question "How do I compare Strings in Java?"
Limited number of possible values --> Enum

Since the possible values of your board is very limited, you can use an enum instead of a String to store the value of the positions.

public enum BoardValue {
   X, O, EMPTY; // _ for empty positions
   @Override
   public String toString() {
      return this == EMPTY ? "." : this.name();
   }
}

Now make your String[][] a into BoardValue[][] board, which will make it much better to use in the long run.
Classes, Methods and Objects

Java is an object-oriented programming language. I think you can learn a lot by reading Oracle's Java Lesson: Classes and Objects. I would suggest that you make your game board into a class. Your current String[][] a=new String[N][N]; should be a field in the class.

This class could then have several methods:

• void randomize(): Randomizes new data in to the board.

• void output(): Print the information to System.out.

• int findConsecutive(String lookingFor): Scan the board for the largest consecutive of the specified String.

Your algorithm

Your current algorithm works by looping through the entire two-dimensional board and for each position it does the following:

• Remember the value of the current position, we can call this "current"

• Loop through the rest of this row/column/diagonal

• When you encounter a position that is not equal to "current", you break this loop.

This is highly inefficient because you are checking each tile way too many times than you need. Instead, you should treat each row/column/diagonal like an individual line of positions. Consider this algorithm:

• searchingFor is the value you want to search for ("X" or "O")

• Initialize the value consecutive to 0.

• Initialize the value highestConsecutive to 0.

• For each row/column/diagonal, loop through the positions in the line and check for searchingFor

• When you encounter this value, you do the following:

• increase consecutive by 1.

• If consecutive is more than highestConsecutive, set highestConsecutive to the value of consecutive.

• If the value did not match, reset consecutive to 0.

• Once the loop is finished, highestConsecutive is the highest consecutive number for this current row/column/diagonal.

Regarding diagonals, you can loop through those by starting at a position on the edge of the board and do the loop once in a straig

StackExchange Code Review Q#41815, answer score: 23