2
\$\begingroup\$

For an assignment, I have created a solver for the Skyscraper Puzzle:

Each puzzle consists of an N×N grid with some clues along its sides. The object is to place a skyscraper in each square, with a height between 1 and N, so that no two skyscrapers in a row or column have the same number of floors. In addition, the number of visible skyscrapers, as viewed from the direction of each clue, is equal to the value of the clue. Note that higher skyscrapers block the view of lower skyscrapers located behind them.

The puzzle and the ordinances are hard coded into the program. I would like feedback on efficiency and readability. Any and all suggestions are welcome!

SkyscraperPuzzle.java

package Skyscraper;


/**
 * Write a description of class SkyscraperPuzzle here.
 *
 * @author David White
 * @version (a version number or a date)
 */

public class SkyscraperPuzzle {

    private int[][] puzzle = {
        {1, 4, 3, 2},
        {2, 3, 4, 1},
        {4, 2, 1, 3},
        {3, 1, 2, 4}
    };
    private Ordinance[][] ords = {
        {new Ordinance(3, Direction.NORTH), new Ordinance(1, Direction.NORTH), new Ordinance(2, Direction.NORTH), new Ordinance(3, Direction.NORTH)}, //NORTH
        {new Ordinance(2, Direction.SOUTH), new Ordinance(4, Direction.SOUTH), new Ordinance(2, Direction.SOUTH), new Ordinance(1, Direction.SOUTH)}, //SOUTH
        {new Ordinance(3, Direction.EAST), new Ordinance(2, Direction.EAST), new Ordinance(2, Direction.EAST), new Ordinance(1, Direction.EAST)}, //EAST
        {new Ordinance(2, Direction.WEST), new Ordinance(3, Direction.WEST), new Ordinance(1, Direction.WEST), new Ordinance(2, Direction.WEST)} //WEST
    };

    /**
     * Write a constructor to initialize and populate the puzzle and list of ordinances
     * so that the puzzle may be checked to see if it is (correctly) solved.
     */
    public SkyscraperPuzzle() {
        //Puzzle is populated above
        //Ords is populated above
    }

    /**
     * returns the puzzle
     */
    public int[][] getPuzzle() {
        return this.puzzle;
    }

    /**
     * Retuns a 1D array that respresents row r of puzzle
     */
    public int[] getRow(int row) {
        return puzzle[row];
    }

    /**
     * Write a method that returns a 1D array that represents column c of puzzle
     */
    public int[] getColumn(int column) { // row by column
        int[] arr = new int[this.puzzle.length];
        for(int i = 0; i < this.puzzle.length; i++) {
            arr[i] = this.puzzle[i][column];
        }
        return arr;
    }

    /**
     * Write a method that returns true if the 1D array parameter does not contain duplicates
     */
    public boolean noDuplicates(int[] arr) {
        for(int i = 0; i < arr.length - 1; i++) {
            for(int j = i + 1; j < arr.length - 1; j++) {
                if(arr[i] == arr[j]) {
                    return false;
                }
            }
        }
        return true;
    }

    /**
     * Write a method that returns true if each value from 1 to the number of rows (or columns, they
     * are the same), is contained in the 1D array
     * 
     * if 5x5
     * then 1 2 3 4 5 should all be in array
     */
    public boolean containsOneToN(int[] arr) {
        int[] array = new int[arr.length];
        int count = 0;
        for(int i = 0; i < array.length; i++) {
            array[i] = i + 1;
        }
        for(int i = 0; i < arr.length; i++) {
            for(int j = 0; j < array.length; j++) {
                if(arr[i] == array[j]) {
                    count += 1;
                    array[j] = 0;
                }
            }
        }
        return count == arr.length;
    }

    /**
     * Write a method that traverses the 2D array, ords, and returns true if each and
     * every Ordinance requirement is met
     */
    public boolean meetsOrdinances() {
        for(int i = 0; i < this.puzzle.length; i++) {
           /* Yet to be implemented */ 
        }
        return false; //used to allow compile
    }

    /**
     * Write a method that returns true if the puzzle is a correctly solved Skyscraper puzzle
     */
    public boolean isSolved() {
        /* Each row and column */
        for(int i = 0; i < this.puzzle.length; i++) {
            if(!noDuplicates(this.puzzle[i]) || !noDuplicates(getColumn(i))) {
                return false;
            }
            if(!containsOneToN(this.puzzle[i]) || !containsOneToN(getColumn(i))) {
                return false;
            }
            if(!meetsOrdinances()) {
                return false;
            }
        }
        return true;
    }
}

Direction.java

package Skyscraper;


/**
 * Enumeration class Direction - write a description of the enum class here
 *
 * @author David White
 * @version (version number or date here)
 */
public enum Direction {
    NORTH, /* Above the 2D array */ 
    SOUTH, /* Below the 2D array */
    EAST, /* Right of the 2D array */
    WEST /* Left of the 2D array */
}

Ordinance.java

package Skyscraper;


/**
 * Write a description of class Ordinance here.
 *
 * @author David White
 * @version (a version number or a date)
 */
public class Ordinance {

    private int num;
    private Direction direction;

    public Ordinance(int num, Direction direction) {
        this.num = num;
        this.direction = direction;
    }

    public int getNum() {
        return this.num;
    }

    public Direction getDirection() {
        return this.direction;
    }
}

The PuzzleTest.java is used to make sure the methods I've written work in the way they're intended to.

PuzzleTest.java

package Skyscraper;


/**
 * Write a description of class PuzzleTest here.
 *
 * @author David White
 * @version (a version number or a date)
 */
public class PuzzleTest {

    public static void main(String[] args) {

        SkyscraperPuzzle p = new SkyscraperPuzzle();
        int[][] puzzle = p.getPuzzle();

        /*
         * Outputs entire puzzle
         */
        for(int[] row : puzzle) {
            for(int item : row) {
                System.out.print(item + " ");
            }
            System.out.println();
        }
        System.out.println();

        /*
         * Test getColumn method
         * Output should be
         * 1
         * 4
         * 7
         * Status: Successful
         */
        int[] col = p.getColumn(0);
        for(int i = 0; i < col.length; i++) {
            System.out.println(col[i]);
        }
        System.out.println();

        /*
         * Test noDuplicates method
         * Output should be
         * true
         * Status: Successful
         */
        int[] arr = {1, 2, 3, 4, 5};
        System.out.println(p.noDuplicates(arr));
        System.out.println();
        /*
         * Test containsOneToN method
         * Output should be
         * true
         * Status: Successful
         */
        int[] arr2 = {1, 2, 3, 4, 5};
        System.out.println(p.containsOneToN(arr2));

        /*
         * Test meetsOrdinances method
         * Output should be
         * true
         * Status: To be tested
         */

    }

}
\$\endgroup\$

closed as off-topic by 200_success, Sᴀᴍ Onᴇᴌᴀ, Austin Hastings, Heslacher, Vogel612 Mar 5 at 17:13

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – Austin Hastings, Heslacher, Vogel612
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ @200_success Yes, just edited the post to add the description. \$\endgroup\$ – Linny Mar 2 at 4:51
  • 1
    \$\begingroup\$ How can this code ever work if meetsOrdinances says Yet to be implemented? \$\endgroup\$ – Roland Illig Mar 2 at 5:16
  • \$\begingroup\$ A puzzle solver by definition gets only the clues and then figures out the arrangement of the skyscrapers. I cannot find any code that does this. \$\endgroup\$ – Roland Illig Mar 2 at 5:25
  • \$\begingroup\$ @RolandIllig This assignment in particular has both the skyscrapers and ordinances hardcoded, and just checks if the ordinances match the skyscraper. No user input is involved. And I'm just inquiring about the state of the code now, assuming that meetOrdinances works as expected. \$\endgroup\$ – Linny Mar 2 at 6:37
2
\$\begingroup\$

Initialization

The hard-coded nature of the SkyscraperPuzzle is a mystery.

I'd move the hard-coded values from SkyscraperPuzzle to PuzzleTest, and add arguments to the constructor to pass in the puzzle/ordinances.

No Duplicates

You have an off-by-one error in this function. While the outer loop runs from 0 ≤ i < N-1, the inner loop should run from i+1 ≤ j < N. You terminate the inner loop one value too early.

Contains 1 To N

This function is more complicated than it needs to be. You are creating an array[] with the values from 1 to N. Then you are looping over the array and testing against the values in array[j]. But we know that array[j] will contain j+1, so we really don't need the array at all. You can just search for the required values in succession:

public boolean containsOneToN(int[] arr) {
    for (int j = 1; j <= arr.length; j++) {
        int count = 0;
        for(int i = 0; i < arr.length; i++) {
            if (arr[i] == j) {
                count += 1;
            }
        }
        if (count != 1)
            return false;
    }
    return true;
}

Is Solved

  • You are accessing puzzle[i] directly; you should be using getRow(i).
  • You are calling getColumn(i) multiple times; instead of creating a new object representing the column each time, why not save the column (and row) in a local variable at the start of the loop?

this

In most cases, this. is unnecessary, and can (should) be removed. The only place it is actually necessary is the Ordinance constructor, and even that could be fixed by renaming the constructor parameters.

\$\endgroup\$
  • 2
    \$\begingroup\$ Some people like writing this. to make it clear that it's an instance variable and not a local variable. \$\endgroup\$ – 200_success Mar 4 at 6:38
  • \$\begingroup\$ @200_success Then this. needs to be added to where it is missing, like to puzzle in getRow(). What about to calls to methods? Shouldn’t this. be added in isSolved() to the calls to noDuplicates(), containsOneToN(), and getColumn() to make it clear these are methods on this object? Adds 15 extra characters to that line. At what point does the “clarity” become “noise”? Rhetorical. I feel it is a terrible “style” and should be squashed as soon as possible, but I know I can’t impose my sense of style on others. (But I can still try.) \$\endgroup\$ – AJNeufeld Mar 4 at 14:22
  • \$\begingroup\$ @AJNeufeld There's no avocation for adding it to methods. There's no need to differentiate between instance methods and local methods because the latter does not exist. Some people use prefixes like "_" for instance variables, "this." is the same thing. (Arguably more readable.) \$\endgroup\$ – Xtros Mar 5 at 0:43

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