23
\$\begingroup\$

This is a follow up on my previous question:

A grid and a menu walked into a program

  • I integrated classes into the mix.
  • The program now asks how big you want the grid to be.

import java.util.Arrays;
import java.util.Random;
import java.util.Scanner;


class NumberGrid {
    private static final int RANDOM_NUMBER = 10;
    static Random rand = new Random();
    public int numberColumns;
    public int numberRows;
    int [][] grid;
    public NumberGrid (int chosenNumberRows,int chosenNumberColumns ){
        numberColumns = chosenNumberColumns;
        numberRows = chosenNumberRows;
        grid = new int [numberRows][numberColumns];
    }

    private static int randomInt(int from, int to) {
        return rand.nextInt(to - from + 1) + from;
    }

    public void amountOfSpecificNumbers() {
        int[] numbers = new int[11];
        for (int y = 0; y < numberRows; y++) {
            for (int x = 0; x < numberColumns; x++) {
                int value = grid[y][x];
                numbers[value]++;
            }
        }
        for (int i = 1; i < numbers.length; i++) {
            System.out.println(" " + numbers[i] + " " + i + "s" );
        }
    }

    public void sumOfColumns() {
        int sumOfColumns[] = new int[numberColumns];
        for (int x = 0; x < numberColumns; x++) {
            for (int y = 0; y < numberRows; y++) {
                sumOfColumns[x] += grid[y][x];
            }
        }
        System.out.println(Arrays.toString(sumOfColumns));
    }

    public void sumOfRows() {
        int sumOfRows[] = new int[numberRows];
        for (int y = 0; y < numberRows; y++) {
            for (int x = 0; x < numberColumns; x++) {
                sumOfRows[y] += grid[y][x];
            }
        }
        System.out.println(Arrays.toString(sumOfRows));
    }

    public void newField() {
        for (int x = 0; x < numberColumns; x++) {
            for (int y = 0; y < numberRows; y++) {
                int randomNumber = (randomInt(1, RANDOM_NUMBER));
                grid[y][x] = randomNumber;
            }
        }
    }

    public void showField() {
        for (int y = 0; y < numberRows; y++) {
            for (int x = 0; x < numberColumns; x++) {
                if (grid[y][x] < 10) {
                    System.out.print(" " + grid[y][x] + " ");
                } else {
                    System.out.print(grid[y][x] + " ");
                }
            }
            System.out.println();
        }
    }

    public static int readInt(Scanner scanner){
        String prompt = ("Pleas enter number 1, 2, 3, 4, 5, or 6");
        System.out.println(prompt);
        while(!scanner.hasNext("[1-6]")) {
            System.out.println(prompt);
            scanner.next();
        }
        return scanner.nextInt();
    }

}

class GridColumns {
    public static int readInt(Scanner scanner){
        String prompt = ("Pleas enter the number of columns you want");
        System.out.println(prompt);
        while(!scanner.hasNextInt()) {

            System.out.println("That's not a number");
            System.out.println(prompt);
            scanner.next();
        }
        return scanner.nextInt();
    }
}

class GridRows{
    public static int readInt(Scanner scanner){
        String prompt = ("Pleas enter the number of rows you want");
        System.out.println(prompt);
        while(!scanner.hasNextInt()) {

            System.out.println("That's not a number");
            System.out.println(prompt);
            scanner.next();
        }
        return scanner.nextInt();
    }
}

public class TenXTen {


    public static void main(String[] args) {

        Scanner scanner = new Scanner(System.in);
        NumberGrid numberGrid = new NumberGrid(GridRows.readInt(scanner),GridColumns.readInt(scanner));

        numberGrid.newField();
        numberGrid.showField();
        while(true) {
            System.out.println("What do you want to do?");
            System.out.println("1. Get a new field");
            System.out.println("2. Show current field");
            System.out.println("3. Count the numbers in the current field");
            System.out.println("4. Sum all rows");
            System.out.println("5. Sum all columns");
            System.out.println("6. Exit program");

            int choice = numberGrid.readInt(scanner);

            switch (choice) {
                case 1:
                    numberGrid.newField();
                    numberGrid.showField();
                    break;
                case 2:
                    numberGrid.showField();
                    break;
                case 3:
                    numberGrid.amountOfSpecificNumbers();
                    break;
                case 4:
                    numberGrid.sumOfRows();
                    break;
                case 5:
                    numberGrid.sumOfColumns();
                    break;
                case 6:
                    return;
                }
            }
        }
    }
\$\endgroup\$
  • 9
    \$\begingroup\$ Ok this is it. I'm taking a note right here, in prevision of the "Best of Code Review 2015 - Best Title Category" ;) \$\endgroup\$ – Mathieu Guindon Jan 21 '15 at 12:38
  • 3
    \$\begingroup\$ @Mat'sMug bonus points for having the author name it themselves, rather than us renaming the question \$\endgroup\$ – Pimgd Jan 21 '15 at 13:01
13
\$\begingroup\$

Field Visibility

Restrict field visibility as much as possible. Use private final wherever you can (the compiler will let you know if you can't).


Using this. in constructor

In your constructor, you can use this. to assign your fields. This will allow you to use the same parameter names as field names. For example:

public NumberGrid(int numberRows, int numberColumns) {
    this.numberColumns = numberColumns;
    this.numberRows = numberRows;
    this.grid = new int[numberRows][numberColumns];
}

Use your constants

In amountOfSpecificNumbers():

int[] numbers = new int[11];

make that

int[] numbers = new int[RANDOM_NUMBER + 1];

Unnecessary parenthesis

You are using an unnecessary extra set of parenthesis on these lines:

int randomNumber = (randomInt(1, RANDOM_NUMBER));
String prompt = ("Pleas enter number 1, 2, 3, 4, 5, or 6");
String prompt = ("Pleas enter the number of columns you want");

Those can become:

int randomNumber = randomInt(1, RANDOM_NUMBER);
String prompt = "Pleas enter number 1, 2, 3, 4, 5, or 6";
String prompt = "Pleas enter the number of columns you want";

Scattered readInt methods

There is currently NumberGrid.readInt, GridColumns.readInt, and GridRows.readInt.

  • The NumberGrid.readInt method is not related to NumberGrid directly, it is more a method used to control the program flow. I would put this method in the TenXTen class. I would name this method readMenuChoice

  • GridColumns.readInt and GridRows.readInt does not need to be inside their own classes. They also belong in the TenXTen class. I would name these methods as readColumnsCount and readRowsCount (or use @Pimgd's suggestion and merge those two methods into one - but still put them as static methods inside TenXTen).

\$\endgroup\$
25
\$\begingroup\$
private static final int RANDOM_NUMBER = 10;

XKCD random number

Everyone knows the true random number is 4.

Less jokingly, what's the RANDOM_NUMBER for? Consider adding comments to explain the purpose of your variables... or better yet renaming them (to RANDOM_GRIDVALUE_MAX, perhaps).

static Random rand = new Random();
public int numberColumns;
public int numberRows;
int [][] grid;

You're missing visibility modifiers on some of your variables. Is this intentional?

Additionally, rand could be final, because it's only set once and then never set again. Perhaps (hint: there are) some of the other variables could be final too.


public NumberGrid (int chosenNumberRows,int chosenNumberColumns ){

private static int randomInt(int from, int to) {

Check your IDE for consistent syntax formatting. As an example, for Eclipse:

Ctrl + Shift + F

Or, in the main menu > Source > Format


String prompt = ("Pleas enter number 1, 2, 3, 4, 5, or 6");

Typo ("Pleas" -> "Please").


public void showField() {
    for (int y = 0; y < numberRows; y++) {
        for (int x = 0; x < numberColumns; x++) {
            if (grid[y][x] < 10) {
                System.out.print(" " + grid[y][x] + " ");
            } else {
                System.out.print(grid[y][x] + " ");
            }
        }
        System.out.println();
    }
}

You're printing SPACE + cell + SPACE if it's less than 10, or cell + SPACE if it isn't.

This was rather unclear to me... until I realized it's for padding.

I recommend moving the if statement to a separate function such as printNumberSpacePadded(int number, int toLength).


class GridColumns {
    public static int readInt(Scanner scanner){
        String prompt = ("Pleas enter the number of columns you want");
        System.out.println(prompt);
        while(!scanner.hasNextInt()) {

            System.out.println("That's not a number");
            System.out.println(prompt);
            scanner.next();
        }
        return scanner.nextInt();
    }
}

class GridRows{
    public static int readInt(Scanner scanner){
        String prompt = ("Pleas enter the number of rows you want");
        System.out.println(prompt);
        while(!scanner.hasNextInt()) {

            System.out.println("That's not a number");
            System.out.println(prompt);
            scanner.next();
        }
        return scanner.nextInt();
    }
}

Hello duplication.

Meet readInt:

public static int readInt(Scanner scanner, String inputMessage, String errorMessage) {
    System.out.println(inputMessage);
    while(!scanner.hasNextInt()) {
        System.out.println(errorMessage);
        System.out.println(inputMessage);
        scanner.next();
    }
    return scanner.nextInt();
}

Stick it somewhere, maybe in a class ScannerUtils or InputHandler (the latter one, I would make responsible of all input).

But wait, it doesn't support this version:

public static int readInt(Scanner scanner){
    String prompt = ("Pleas enter number 1, 2, 3, 4, 5, or 6");
    System.out.println(prompt);
    while(!scanner.hasNext("[1-6]")) {
        System.out.println(prompt);
        scanner.next();
    }
    return scanner.nextInt();
}

For that, simply add a String validInputPattern or something like that.


I don't think you understood classes yet. Or maybe you did, but only in a functional sense.

You've got classes that contain no variables and don't have any non-static methods. Because of that, they're little more than utility boxes...

NumberGrid is good, though.


Lastly... That menu at the end, I would rewrite it to use lambda's and some other clever trickery so that I can say something like this:

addMenuItem("Show current field", () -> numberGrid.showField());

This would then add the "2" in front of it on it's own.

Perhaps it's a bad idea, but I think it's worth a try. I don't like how case 5: means numberGrid.sumOfColumns() because there's a double duplication here...

System.out.println("5. Sum all columns"); the 5 duplicates case 5:, and System.out.println("5. Sum all columns"); the "Sum all columns" duplicates numberGrid.sumOfColumns();. Now this form of duplication isn't bad, it's just that as you add lines between the menu and the actions, it becomes harder to verify that the code is actually working as intended.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.