6
\$\begingroup\$

I've just completed the 3rd task of the 1st assignment offered by Stanford's programming methodologies (see the full resources for it here too).

I just need some helpful feedback on how my code can be better (decomposition-wise) and whether or not my code is comprehensible and readable.

I'm also unsure about the pre-conditions and post-conditions. Can someone please elaborate more on that? Is a pre-condition basically where the program is at when attempting the method and the post-condition just a result of the actual method?

I need all the criticism and feedback I can get.


/* File: CheckboardKarel.java
 * ---------------------------
 * This program will basically go through an entire row and land checkers on every 2nd cell.
 * This process will be alternated on each row.
 */

import stanford.karel.*;

public class CheckerboardKarel extends SuperKarel {

    public void run() {
        while (facingWest() || facingEast()) {
            if (frontIsBlocked()) {
                turnKarelNorth();
            } //end if
            transverseRow();
            transverseEvenRow();
        } //end while
    }

    /*
     * Method: turnKarelNorth
     *  ---------------------- 
     * Pre-condition: The program is in a 8x1 (1 column) world and Karel has no choice but to turn north
     * Post-condition: This makes Karel turn left and face north
     */
    private void turnKarelNorth() {
        putBeeper();
        turnLeft();
        moveUpwards();
    }

    /*
     * Method: moveUpwards
     *  ---------------------- 
     *  This makes Karel lay down the beepers every 2 moves
     */


    private void moveUpwards() {
        while (facingNorth()) {
            move();
            move();
            putBeeper();
        }
    }
    /*
     * Method: transverseRow
     *  ---------------------- 
     *  Pre-Condition:Karel is facing east and is ready to place the beeprs on the first odd row
     *  Post-Condition: Karel places all the beepers every 2 steps on the first row
     */

    private void transverseRow() {
        while (facingEast()) {
            putBeeper();
            move();
            isFrontClear();
            isFrontBlocked();
        } // end while
    }

    /*
     * Method: isFrontClear (if statements)
     *  ---------------------- 
     *  This checks whether Karel is being obstructed.
     *  She takes anther move if the front is clear.
     *  Otherwise, if she is blocked, she executes placeBeeperOnNextRow
     */

    private void isFrontClear() {
        if (frontIsClear()) {
            move();
        } // end if
        else if (frontIsBlocked()) {
            placeBeeperOnNextRow();
        } // end else
    }

    /*
     * Method: isFrontBlocked (if statement)
     *  ---------------------- 
     *  This checks whether Karel is being obstructed.
     *  She puts the beeper and places a final beeper on the row.
     */

    private void isFrontBlocked() {
        if (frontIsBlocked()) {
            putBeeper();
            placeLastBeeperOnRow();
        }
    }

    /*
     * Method: placeLastBeeperOnRow (if statement)
     *  ---------------------- 
     *  This checks if Karel is facing east
     */

    private void placeLastBeeperOnRow() {
        if (facingEast()) {
            ascendOnOddColumn();
        }
    }
    /*
     * Method: placeBeeperOnNextRow (if)
     *  ---------------------- 
     *  This method makes Karel ascend at the end of an even column only if she is facing east
     */

    private void placeBeeperOnNextRow() {
        if (facingEast()) {
            ascendOnEvenColumn();
        }
    }

    /*
     * Method: ascendOnEvenColumn
     *  ---------------------- 
     *  This method makes Karel climb up the row and place a beeper on the new row
     */

    private void ascendOnEvenColumn() {
        turnLeft();
        move();
        putBeeper();
        turnLeft();
        move();
    }

    /*
     * Method: ascendOnOddColumn 
     * ---------------------- 
     * This method makes Karel
     * climb up the row and place a beeper on the new row
     */

    private void ascendOnOddColumn() {
        turnLeft();
        move();
        turnLeft();
    }

    /*
     * Method: transverseEvenRow
     *  ---------------------- 
     *  Pre-condition: Karel lays the checkers on the first row and is now on the 2nd row
     *  Post-condition: Karel lays the checkers alternately on every 2 steps
     */

    private void transverseEvenRow() {
        while (facingWest()) {
            move();
            putBeeper();
            isFrontClear();
            isFrontBlockedForEvenRow();
        }
    }
    /*
     * Method: isFrontBlockedForEvenRow
     *  ---------------------- 
     *  This checks whether Karel (on an even row) is being blocked
     *  It is in between the steps to help Karel move step-by-step without hitting a wall (which would break the loop)
     *   
     *  */

    private void isFrontBlockedForEvenRow() {
        if (frontIsBlocked()) {
            turnRight();
            move();
            turnRight();
        } //end if
    }

}

\$\endgroup\$
4
\$\begingroup\$

Overall, your code looks pretty good! I'd say if you want a more thorough code review of your abilities, you should write up some code that doesn't simply call these external methods. It's hard to get a good feel of whether you know good programming/Java habits when all of your methods only call other methods that have been written for you.

With that said, my primary qualm is with your method names. In general, you should always design your methods with the idea that someone else will be using them in the future. That means they should be as self-explanatory and intuitive as possible.

For example:

private void turnKarelNorth() {
    putBeeper();
    turnLeft();
    moveUpwards();
}

As a developer, I would assume that the turnKarelNorth() method did exactly that -- turned her North. But it doesn't actually do that explicitly; you only know that it will always turn her North because of the specific problem presented in the PDF. What it actually does is turn her left, which may or may not be North-facing depending on her current state.

For this reason, I would also extract the moveUpwards() call from this method. You can't guarantee when it's called that she will actually be facing "upwards", which again makes your design a bit inconsistent and much less flexible. Also, the method name turnKarelNorth() implies a directional adjustment, not a positional adjustment. In other words, a developer would assume that this just shifted where Karel was facing rather than actually moving her to another grid location.

Consistency is a big concern as well with method names. You have turnKarelNorth(), but then you have moveUpwards(). Choose one: either cardinal directions or up-down/left-right, but don't mix and match. It makes your code that much harder to follow.

private void isFrontClear() {
    if (frontIsClear()) {
        move();
    } // end if
    else if (frontIsBlocked()) {
        placeBeeperOnNextRow();
    } // end else
}

To follow Java convention, any method that begins with "is" should return a boolean. This is a standard which again allows for consistency across applications and across the language as a whole. Again, the name implies getting a piece of information, not actually affecting anything (as you do when you call move()).

Everything I've said applies to most of your code. So to summarize, when designing methods:

  • Make sure the method name reflects what the method actually does;
  • Make your method as flexible as possible to make your API more powerful and prevent the need for complete rewrites in the future;
  • Have your method do one thing, and do it well;
  • Be as consistent as you possibly can within your API; and
  • Follow Java naming conventions

Also, as a side note, I think you meant traverse rather than transverse.

\$\endgroup\$
  • 1
    \$\begingroup\$ To elaborate on the Java convention, any method whose name begins with is or has should return a boolean and have no visible side effects. \$\endgroup\$ – 200_success Jan 7 '14 at 21:19
  • \$\begingroup\$ That was really insightful ! Thanks for the great feedback. \$\endgroup\$ – user3025998 Jan 7 '14 at 22:09
3
\$\begingroup\$

There are a several strategies for generating paths that the robot could take to create the pattern of beepers. Judging from the robot's final position in the two illustrated examples, a boustrophedon path along rows is correct. That's a good start.

Since this is an exercise in creating a state machine without any variables, the only good way to encode the state information in the orientation of the robot. (There are hackish ways as well, but let's not get into that.) Therefore, it is crucial to think about what the state of the robot can be before and after each function call.

You often move() without first checking whether such a move would fall off the edge of the board. Since every move should be preceded by a check, I would define an advance() function:

/**
 * Advance along a boustrophedon path.
 *
 * Precondition: Karel is either
 *  (a) facing north, and the path is complete, or
 *  (b) facing east, and can advance east, or
 *  (c) facing east at the eastern edge, and needs to move one
 *      square north then turn to face west, or
 *  (d) facing west, and can advance west, or
 *  (e) facing west at the western edge, and needs to move one
 *      square north then turn to face east.
 *
 * Postcondition: Karel is either
 *  (A) facing north, and the path is complete, or
 *  (B) facing east, or
 *  (C) facing west.
 */
private void advance() {
    if (frontIsClear()) {
        move();
    } else if (facingEast()) {
        turnLeft();
        if (frontIsClear()) {
            move();
            turnLeft();
        }
    } else if (facingWest()) {
        turnRight();
        if (frontIsClear()) {
            move();
            turnRight();
        }
    }
}

Once that function is defined, the run() function is trivial, and you should need no other functions. Since it is a homework question, I'll omit the solution.


Addendum

Just as important as seeing what to do is realizing what not to do. Perhaps surprisingly, it turns out to be unhelpful to define a function for traversing a row. Why? Think about what it means to lay out a checkerboard pattern — describe the task in half a dozen words. The "edge of the board" is not really part of that description.

Now, consider what traversing a row entails — it means you move forward until you hit an eastern or western edge, then make a U-turn onto the next row. Then what? Should your next action involve laying down a beeper or not? The answer is, it depends! That's why there is so much code in your question: you have to handle eastbound and westbound cases of odd-length and even-length rows. The fundamental design issue is that the row-traversal functions terminate with a condition (hitting the edge of the board) that is irrelevant to the task (laying down a beeper on every other square).

\$\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.