Simple exercice representing a slidepuzzle I've been helping out a student with an exercice, typed this up rapidly and thought it would be the ideal opportunity to improve my own programming style and approach to problem solving.

In this exercice a grid represents one of those slide puzzles. The patterns have been replaced by numbers. The zero represents the missing piece which allows for movement. There is no gameplay as of yet and the aim is to simply swap the missing piece around as would be done before the start of a game.


How could I make the code more concise?
What would be a good way to cut up the code into functions?
What other things could be improved to approach a decent programming style in Java?




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

public class Puzzle {

    public static void main(String[] args) {

        System.out.println("Please enter dimensions of the puzzle.");

        System.out.println("Enter length (&gt;0) ");

        Scanner sc = new Scanner(System.in);
        int width = sc.nextInt();

        System.out.println("Enter height (&gt;0)");

        int height = sc.nextInt();

        int[][] puzzle = new int[width][height];

        populatePuzzle( puzzle );

        System.out.println("Enter position of missing piece");

        int position = sc.nextInt();

        placeMissingPiece( position, puzzle );

        System.out.println( "Missing piece is at " + Arrays.toString( findNumber( puzzle, 0 ) ) );

        displayPuzzle( puzzle );

        int number = 0;

        while( true ){
            System.out.println("Enter the number to be swapped around");

            number = sc.nextInt();

            swapElementValues( puzzle, findNumber( puzzle, 0 ), findNumber( puzzle, number ) );

            displayPuzzle( puzzle );
        }
    }

    public static void swapElementValues( int[][] puzzle, int[] missing, int[] target ){

        int tempValue = puzzle[ missing[0] ][ missing[1] ];

        puzzle[ missing[0] ][ missing[1] ] = puzzle[ target[0] ][ target[1] ];
        puzzle[ target[0] ][ target[1] ] = tempValue;
    }

    public static int[] findNumber( int[][] puzzle, int number ){

        for( int i = 0; i &lt; puzzle.length; i++ ){
            for( int j = 0; j &lt; puzzle[0].length; j++ ){
                if( puzzle[i][j] == number ){
                    int[] foundCoords = new int[2];

                    foundCoords[0] = i;
                    foundCoords[1] = j;

                    return foundCoords;
                }
            }
        }

        return null;
    }

    public static int getRandomInteger(){

        Random randomGenerator = new Random();

        return randomGenerator.nextInt(99) + 1;
    }

    public static void populatePuzzle( int[][] puzzle ){

        for( int i = 0; i &lt; puzzle.length; i++ ){
            for( int j = 0; j &lt; puzzle[0].length; j++ ){                
                puzzle[i][j] = getRandomInteger();
            }
        }

    }

    public static void displayPuzzle( int[][] puzzle ){

        for( int i = 0; i &lt; puzzle.length; i++ ){
            for( int j = 0; j &lt; puzzle[0].length; j++ ){
                System.out.print( puzzle[i][j] + "," );
            }

            System.out.println( "" ); // Carriage return
        }

    }

    public static void placeMissingPiece( int elementNumber, int[][] puzzle ){

        int counter = 0;

        for( int i = 0; i &lt; puzzle.length; i++ ){
            for( int j = 0; j &lt; puzzle[0].length; j++ ){
                counter++;

                if( counter == elementNumber ){
                    puzzle[i][j] = 0;
                }
            }
        }

    }
}
System.out.println("Enter height (&gt;0)");

        int height = sc.nextInt();


This pattern repeats itself several times in your code. Turn it into a method, e.g. (assuming the Scanner is turned into a static field):

private static int promptForInt(String message) {
    System.out.println(message);
    return sc.nextInt();
}


-

    public static int getRandomInteger(){

        Random randomGenerator = new Random();

        return randomGenerator.nextInt(99) + 1;
    }


Creating a new Random for each call makes no sense from a performance perspective. The object should be created exactly once.

    public static void populatePuzzle( int[][] puzzle ){

    public static void displayPuzzle( int[][] puzzle ){

    public static void placeMissingPiece( int elementNumber, int[][] puzzle ){


You're not thinking object-oriented here. The above pattern is a really obvious indication that each method should belong to a Puzzle class. That way your main method can concern itself with the core logic of your program instead of the implementation details of the puzzle. E.g.:

Puzzle puzzle = new Puzzle(); // .populate() should probably be called from the constructor
// ...
puzzle.placeMissingPiece(position);
// ...
puzzle.display();
// ...
puzzle.swapElementValues(0, number); // findNumber() called as a private method
// etc...