# Finding average of eight immediate neighbors of a matrix

I have this basic Java code to find average of eight immediate neighbors of a matrix.

Is there any way to simplify or merge any part of it, or can I refactor it?

I'm a beginner in Java programming and am trying to improve myself.

// CODE STSRT
import java.util.Scanner;
import java.lang.*;

class Matrixer
{

double[][] matrix, computedMatrix;
final int rows, cols;

public Matrixer(int N, int M, double[][] imatrix)
{
rows = N;
cols = M;
matrix = imatrix;
computedMatrix = new double[N][M];
}

public void computeAverages()
{
for (int i = 1; i < rows - 1; i++)
{
for (int j = 1; j < cols - 1; j++)
{
computedMatrix[i][j] = cellNeighborsAverage(i, j);
}
}
}

private double cellNeighborsAverage(int row, int col)
{
// Ignore center cell And tack Neighbors
double sum = matrix[row - 1][col - 1] + matrix[row - 1][col]
+ matrix[row - 1][col + 1] + matrix[row][col - 1]
+ matrix[row][col + 1] + matrix[row + 1][col - 1]
+ matrix[row + 1][col] + matrix[row + 1][col + 1];
return sum / 8;
}

public void printComputedMatrix()
{
for (int i = 0; i < rows; i++)
{
for (int j = 0; j < cols; j++)
{
System.out.printf("%.0f", computedMatrix[i][j]);
System.out.print(" ");
}
System.out.println();
}
}

public static void main(String[] args) throws NullPointerException {
//This part for defined the size if matrix
int R,C;
Scanner rc = new Scanner(System.in);
System.out.println("Enter the number of matrix's rows :");
R = rc.nextInt();
Scanner cc = new Scanner(System.in);
System.out.println("Enter the number of matrix's columns :");
C = cc.nextInt();
//End of defined part

// condition part To see if columns and rows not greater than 2 stop execution
if(R<3){System.out.println("ERROR! \n Columns and rows should be greater than 2");
}else{
// condition part To see if columns not equal rows stop execution
if(R!=C){
System.out.println("ERROR! \n The number of rows should equal number of columns");
}
else{

Scanner sc = new Scanner(System.in);
double[][] inputMatrix = new double[R][C];
try {

System.out.println("Enter The Elementt of matrix ("+R+"x"+C+")");
for (int i = 0; i < R; i++) {
for (int j = 0; j < C; j++) {

inputMatrix[i][j] = sc.nextInt();
}
}
Matrixer mx = new Matrixer(R, C, inputMatrix);
System.out.println(" average of eight immediate neighbors is \n");
mx.computeAverages();
mx.printComputedMatrix();

} catch (NullPointerException e) {
e.printStackTrace();
}
} } //End of conditions part
}}

//END OF THE CODE


Welcome to CodeReview!

# Class naming

Maybe it's a term I'm not familiar with but Matrixer does not seem to ring a bell. Is it supposed to identify a "creator of matrixes"? Is it the name of your project? If you haven't made a conscious decision for this name, I would suggest turning it into something "universally" descriptive like MatrixCalculator (pretty much anything that contains the essence of what your class is supposed to achieve).

# Indentation

Several places are indented incorrectly/inconsistently. Use a tab (4 spaces) for each new block level.

I would also layout code like your cellNeighborsAverage like this:

double sum = matrix[row - 1][col - 1] + matrix[row - 1][col]
+ matrix[row - 1][col + 1] + matrix[row][col - 1]
+ matrix[row][col + 1] + matrix[row + 1][col - 1]
+ matrix[row + 1][col] + matrix[row + 1][col + 1];


It's just a bit easier to read.

Likewise for your main method: the indentation is all over the place; maybe something went wrong when copy-pasting it here? Keep your if statements in this format:

if(condition) {
doSomething();
} elseif (anotherCondition) {
doAnotherThing();
} else {
doNothing();
}


# Parameter naming

Your constructor takes variables N and M in a scientific-oriented context this is an acceptable practice so it will depend on how you view this code but standard conventions state that variables use the lowerCamelCase convention.

I would also make their meaning clearer: N could become rows and M would be columns.

Likewise your parameter is named imatrix. What does that i signify? You can simply call it matrix and use this.matrix to refer to the instance variable inside the constructor body.

I would also distinguish between matrix and computedMatrix. Computed what exactly? Maybe sourceMatrix and averagedMatrix are more appropriate terms?

# Method naming

cellNeighborsAverage doesn't really tell me much about what goes on in there. Something about the neighbours average, that's for sure. But what exactly does it do? Does it calculate it? Return it? Do I have to set it?

Conventions state that your method names are built up like [verb][subject] which would become getAverageFromNeighbours.

# NullPointerException

The NPE is something you should always prevent and guard against instead of throwing/catching it at runtime. You know the flow of your program and thus you should be able to determine where a NPE could occur. Guard yourself against this by validating parameters, instantiating fields inline when you can, avoid returning null, etc.

# Scanners

You only need one Scanner on your System.in which you re-use to get input.

Right now you're working with void methods that print the information directly to the console. This will bring problems when you decide to publish your library (class) online and let people use it. What if that person now wants to write the results to a textfile instead of the console?

Likewise for unit testing this will become troublesome.

Therefore I suggest that you return the results from your methods instead of printing to the console inside Matrixer itself; let the main class decide what happens with your results.

• I'm sorry but that's not something you should ask from another person. I have given you several pointers on how you can improve your code, it's up to you to apply them. – Jeroen Vannevel May 5 '14 at 21:54

Everything Jeroen Vannevel said, plus the following:

# Use guard clauses

Instead of using if/else { /* the rest of the program */ } structures, write out those conditions as guard clauses, more like if(notGood) { System.err.println("bad"); return; } or, preferably, if(notGood) { System.err.println("bad"); System.exit(1); } where 1 is returned as the exit status to indicate error.

# Don't ask for two dimensions if it's a square

You ask for two dimensions to the array, but then error out if they are not equal. Better to ask once and indicate to the end user that it will be used for both dimensions.

# Don't pass in parameters you can otherwise derive directly

Your Matrixer constructor takes two (poorly named) parameters N and M, in addition to the array to work from. N and M appear to represent the dimensions of the array, but these dimensions can be extracted directly from the length property of the imatrix array and first subarray. Since we know the array must be square, we can just use the array length for both. (We assume what we are getting is correct because we are the ones providing the input. If we are not the ones providing the input array, we should provide a checking mechanism to ensure proper function.)

There are a few issues with your comments. Some comments are extraneous and redundant, such as // condition part To see if columns and rows not greater than 2 stop execution, which literally says exactly the same thing as the code. In this case, let the code speak for itself, as it is sufficiently self-explanatory. Other comments seem to divide the code into sections. It is often better to make those divisions explicit in the code instead, for example by making it a separate method. Yet other comments have spelling errors. Comments are for communication with other human beings, and will be used especially where the code cannot be clear enough. Proper spelling and grammar are especially important to ensure that communication is as clear as possible.