Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite
1

I have a code which was not refactored at all. I refactored it to some extent, but stuck at a point where I cannot think of anything further.

Tractor.java:

package com.farm;

public class Tractor implements MethodsInterface {

    private int[] position;

    private int[] field;

    private String orientation;

    public Tractor(){
        position  = new int[]{0,0};
        field = new int[]{5,5};
        orientation = "N";
    }

    public void move(String command) {
        if(command=="F"){
            moveForwards();
        }else if(command=="T"){
            turnClockwise();
        }
    }

    private void moveForwards() {
        if(orientation=="N"){
            position = new int[]{position[0], position[1]+1}; 
        } else if(orientation == "E"){ 
            position = new int[]{position[0]+1, position[1]}; 
        } else if(orientation == "S"){ 
            position = new int[]{position[0], position[1]-1}; 
        }else if(orientation == "W"){ 
            position = new int[]{position[0]-1, position[1]}; 
        } 
        if(position[0] > field[0] || position[1] > field[1]) {
            try {
                throw new TractorInDitchException();
            } catch (TractorInDitchException e) {
                e.printStackTrace();
            }
        }

    }

    private void turnClockwise() {
        if(orientation=="N"){
            orientation = "E";
        }else if(orientation == "E"){
            orientation = "S";
        }else if(orientation == "S"){
            orientation = "W";
        }else if(orientation == "W"){
            orientation = "N";
        }
    }


    public int getPositionX() {
        return position[0];
    }

    public int getPositionY() {
        return position[1];
    }

    public String getOrientation() {
        return orientation;
    }
}

TractorInDitchException.java:

package com.farm;

public class TractorInDitchException extends Exception{

}

MethodsInterface.java:

package com.farm;

public interface MethodsInterface {

    public int getPositionX();
    public int getPositionY();
    public String getOrientation();
}

What else could be refactored?

share|improve this question

2 Answers

up vote 7 down vote accepted

  1. Instead of magic constants like "N", "S" etc. use an enum which contains named constants and their values are checked on compile time.

    public enum Orientation {
  NORTH, SOUTH, WEST, EAST
}

    With plain Strings it's too easy to set an invalid value to the orientation field.

  2. You can move the turnClockwise method to an enum:

    public enum Orientation {
  NORTH {
    @Override
    public Orientation turnClockwise() {
      return EAST;
    }
  },
  SOUTH {
    @Override
    public Orientation turnClockwise() {
      return WEST;
    }
  },
  WEST {
    @Override
    public Orientation turnClockwise() {
      return NORTH;
    }
  },
  EAST {
    @Override
    public Orientation turnClockwise() {
      return SOUTH;
    }
  };

  public abstract Orientation turnClockwise();

}

    It increases cohesion and replaces conditional with polymorphism.

    (I'd also try to create a Position class with x and y coordinates and move the statements from the moveForwards method to this class like goNorth(), goWest() etc. Then a new Position moveForward(Position current) method in the Orientation enum would call one of the Position.go*() methods.)

  3. I'd change this array:

    position = new int[] {0, 0};

    to two ints:

    private int positionX;
private int positionY;

    Indexes here (0 and 1) are magic constants.

  4. The moveForwards and the the Tractor class looks unfinished. Noone writes the field array.

  5. This:

    try {
  throw new TractorInDitchException();
} catch (final TractorInDitchException e) {
  e.printStackTrace();
}

    could be written as

    new TractorInDitchException().printStackTrace();

    without the try-catch structure. A logger framework (for example SLFJ and Logback) would be better here.

  6. What should happen when the tractor reach the border of the field? (When the index is negative or greater than or equal to the size of the array.)

share|improve this answer
+1 for a great thorough answer. Further to #2, I'd move the Orienation enum out of the Tractor class so that is available for the any future classes that may need it (such as FieldCar or SheepDog). – Robert Gowland Jan 24 '12 at 14:19
Thanks, @RobertGowland! I agree. Actually, I don't like inner classes too much, so one file per class/enum/interface is the default for me :-) – palacsint Jan 24 '12 at 14:59
Is the tractor being in the ditch really an exception case while running the code? In other words, it seems proper to use the State concept then forcing the VM to dump the current stack if the tractor moves somewhere that causes problems. – Andrew Finnell Jan 26 '12 at 20:25
up vote 5 down vote

First, the code compares Strings using ==. This will compare the reference rather than the contents of the String. Use String.compareTo(String) or String.compareToIgnoreCase(String).

Better yet, do not use a String as an argument in your move() method. It would be ideal to use an enum, but a char will do as well.
The same holds for your variable orientation.

MethodsInterface is not a very descriptive name. It seems to describe the location and orientation of objects, so consider a more descriptive name. Maybe SpatialInfo.

Position is an array of int's.. it seems to be 2-dimensional, so consider the use of Point. In fact, I would prefer a Point or Position class above an array whether you use 2D or 3D.

I'm not sure why your TractorInDitchException is an Exception. Exceptions are used for handling exceptional situations in program flow. I imagine that your tractor ending up in a ditch, although not an everyday situation on your simulated farm, should be covered by the regular program flow.
There's not much point in having a try-catch block around a single throw operation anyway, when the only point of the try-catch block is to catch the very Exception that you're deliberately throwing.
A message to the user, and perhaps some kind of flag on the Tractor object that it is in the ditch, would be more useful.
If all you want is the stack trace, you can use new Throwable().printStackTrace() (see Javadoc).

HTH.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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