10
\$\begingroup\$

I am in my first year of formally learning computer programming in high school (we are using Java). One of the tools we used was Karel J. Robot, which can be downloaded here. Although we finished using Karel after a semester, I felt that there was a lot more I could do with it. I decided to use some of what we are now covering in the class and implement a coordinate system and other improvements. This may also become part of a larger project where I manipulate robot(s) on the plane to perform other tasks as part of an interpreter.

Here is the code I wrote:

//This class creates methods to govern the basic movements of the robot.
//The robot moves on a Cartesian plane with origin (1,1) and turns at 90˚ angles.
//Created 02/08/2016 by Trevor B. (Stack Exchange user Eridan)

import kareltherobot.Robot;

public class Commands extends Robot {
    private int xCoordinate;
    private int yCoordinate;
    private Direction currentDirection;
    private int beeperCount;

    public Commands(int street, int avenue, Direction direction, int beepers) {
        super(street, avenue, direction, beepers);

        xCoordinate = avenue;
        yCoordinate = street;
        currentDirection = direction;
        beeperCount = beepers;
    }

    //MOVEMENT

    public void move(int steps) {
        for(int i = 0; i < steps; i++) {
            super.move();
        }

        if(facingNorth()) {
            yCoordinate += steps;
        }
        else if(facingSouth()) {
            yCoordinate -= steps;
        }
        else if(facingEast()) {
            xCoordinate += steps;
        }
        else if(facingWest()) {
            xCoordinate -= steps;
        }
    }

    public void move() {
        move(1);
    }

    public void turnLeft(int leftTurns) {
        for(int i = 0; i < leftTurns; i++) {
            super.turnLeft();
        }
    }

    public void turnLeft() {
        turnLeft(1);
    }

    public void turnRight() {
        turnLeft(3);
    }

    public void turnAround() {
        turnLeft(2);
    }

    //DIRECTIONS

    public void faceNorth() {
        while(!facingNorth()) {
            turnLeft();
        }
        currentDirection = North;
    }

    public void faceSouth() {
        while(!facingSouth()) {
            turnLeft();
        }
        currentDirection = South;
    }

    public void faceEast() {
        while(!facingEast()) {
            turnLeft();
        }
        currentDirection = East;
    }

    public void faceWest() {
        while(!facingWest()) {
            turnLeft();
        }
        currentDirection = West;
    }

    //COORDINATE CHANGE

    public void coordinateChange(int xDestination, int yDestination) {
        int xDifference = xDestination - xCoordinate;
        int yDifference = yDestination - yCoordinate;

        if(xDifference > 0) {
            faceEast();
        }
        else if(xDifference < 0) {
            faceWest();
        }
        move(xDifference);

        if(yDifference > 0) {
            faceNorth();
        }
        else if(yDifference > 0) {
            faceSouth();
        }
        move(yDifference);
    }

    //BEEPER MANAGEMENT

    public void putBeeper(int drops) {
        for(int i = 0; i < drops; i++) {
            super.putBeeper();
        }
        beeperCount -= drops;
    }

    public void putBeeper() {
        putBeeper(1);
    }

    public void pickBeeper(int picks) {
        for(int i = 0; i < picks; i++) {
            super.pickBeeper();
        }
        beeperCount += picks;
    }

    public void pickBeeper() {
        pickBeeper(1);
    }

    public void putAllBeepers() {
        while(anyBeepersInBeeperBag()) {
            putBeeper();
        }
    }

    public void pickAllBeepers() {
        while(nextToABeeper()) {
            pickBeeper();
        }
    }
}

Is there anything I can do to improve this code? Please keep in mind that I'm a beginner to programming, so I apologize for any glaring failures to follow proper programming conventions.

\$\endgroup\$
  • 2
    \$\begingroup\$ Could you tell us which exercise this code is intended to answer? \$\endgroup\$ – 200_success Feb 9 '16 at 2:55
  • \$\begingroup\$ @200_success This code is not meant to answer any particular exercise; I am doing this just for fun. \$\endgroup\$ – Arcturus Feb 9 '16 at 2:58
  • 1
    \$\begingroup\$ Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Feb 9 '16 at 23:55
  • \$\begingroup\$ @Vogel612 I have linked the updated code below. Thank you for sharing that. \$\endgroup\$ – Arcturus Feb 10 '16 at 0:04
11
\$\begingroup\$

Commands is a poor name for this class. It's a robot of some kind. Better names would be EnhancedRobot or GridAwareRobot or CartesianRobot.

What is the point of the currentDirection member variable? It's a write-only variable, and is therefore useless.

Reusing code via inheritance is always tricky business, especially here, where the Robot class that you are extending is a bit of a poorly documented black box.

The way you use super calls is confusing. For example, a call to Commands.move(2) chains to Robot.move(). However, to ensure that the zero-parameter .move() method on a Commands object works consistently, you also override Commands.move() to chain to move(1). If you were to add another level of inheritance on top of this, it could get very difficult to trace what calls what, especially with overriding and overloading involved.

In addition, you also have to be concerned that the superclass methods to move, turn, and pick/drop beepers don't always succeed the way you would expect. Indeed, a robot can enter a "crashed" state if it tries to drop a beeper when the bag is empty, or if it runs out of bounds, or if it runs into a wall (see the World class). Since the Robot offers no way to detect and recover from such errors after the fact, you must proceed with extreme caution with every action. This is especially important since your Commands and the underlying Robot are keeping track of the state independently, and your information could get out of sync if you assume that everything has gone smoothly.

The Robot's capabilities are actually deliberately limited to one primitive action at a time to force you to build reliable state machines in these exercises. By writing a move(int steps) method, you are, in a sense, missing the point of these algorithm-design exercises. What should happen if, say, Karel runs into a wall after 2 steps of a move(5)? Karel has no remote-sensing capabilities, so your options are:

  1. Let it crash, and fail the exercise. ☹
  2. Move 2 steps, detect that the front is not clear, avoid telling the Robot superclass to move the third step, and somehow give your caller a way to query the position of the robot and deduce that the move(5) only got partially executed. This is better than crashing, but puts your user in the nasty position of figuring out what to do with a partially-executed command.

    And how would the user figure out what state Karel is in after a partially-executed command? You'll notice that the language you are allowed to use in these exercises is a restricted subset of Java: you aren't supposed to assign or compare variables (except in a counting for-loop).1 In a sense, you have already cheated in writing this code, by assigning variables. But with this strategy, you would also be forcing your user to cheat too.

  3. Move 2 steps, detect that the front is not clear, turn around 180°, move 2 steps, and turn around 180° again. This is known as atomically failing. This kind of behaviour would restore sanity to your user's algorithms: at least you would be assured that after a move(5), the robot is in one of two states (either five steps ahead, or not moved at all).
  4. Endow your Commands class with a canMove(int steps) method to emulate remote-sensing capability, which you implement by carefully walking out and walking back. Then, if the user calls move(5) and crashes, it's not your fault.

You'll probably realize that none of these strategies is palatable, and do your exercises using a dumb Robot after all.


1 Based on the reference in Karel the Robot Learns Java, Appendix A

\$\endgroup\$
  • \$\begingroup\$ I'll go through these 1-by-1 and propose fixes. Class renamed to BetterRobot. currentDirection exists because I want to be able to track every movement that the robot makes, and print it if necessary. However, looking at it again, I think it should be set to public. The duplicate methods will be deleted. Karel has booleans beepersInBeeperBag and frontIsClear to check if there are any beepers on the current spot or if there is a wall in front. These can be done with while loops. \$\endgroup\$ – Arcturus Feb 9 '16 at 23:05
  • \$\begingroup\$ Thanks for your feedback. I think I have implemented your suggestions. I have posted it on GitHub. \$\endgroup\$ – Arcturus Feb 9 '16 at 23:52
  • \$\begingroup\$ An updated version of the code can be found here. \$\endgroup\$ – Arcturus Feb 10 '16 at 0:04
  • 1
    \$\begingroup\$ Your revised code isn't better. You've missed my point that direction is useless and should be eliminated; public is worse. If turning off is an acceptable response to failure, then the way you've implemented it still leads to desynchronized state: a turned-off BetterRobot still kinda tries to act on commands. \$\endgroup\$ – 200_success Feb 10 '16 at 0:48
  • \$\begingroup\$ I'm trying to have available a way to be able to print the current direction of the robot. Should I change currentDirection back to private and create a getCurrentDirection method? Also, I'm not sure which version you saw above, but wouldn't break; in the current version stop the robot from trying to execute commands, in addition to turnOff preventing it from actually executing any commands? \$\endgroup\$ – Arcturus Feb 10 '16 at 15:54

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.