5
\$\begingroup\$

I am trying to practice OOP and TDD concepts, I have written this code for Mars rover challenge from marsrovertechchallenge .

Can you please review my code from my GitHub repository?

Or you can review the four files below.

This would be really helpful for me to improve my OOP skills.

The MarsRoverCode.java contain the main program which runs the code.

RoverPosition.java contains all the logic with error handling. I think I should structure this in a better way but I don't know what to improve and how.

I created some Test Driven Test Cases using JUnit framework. I used parameterized methods for which I don't know if it is the best way or not . I created two classes one for the direct test cases, and one for the exception handling:

MarsRoverTests.java

MarsRoverTestsNegativeSenarios.java

Please let me know what you think this will be very helpful for me to optimize my code.

MarsRoverCode.java

package codechallenge;

import java.util.Scanner;

public class MarsRoverCode {
    public static void main(String[] args) {
        System.out.println("please enter the pleteu dimentions");
        Scanner input = new Scanner(System.in);
        String plateauDimentions = input.nextLine();

        System.out.println("please enter the first rover position and instructions.");
        String firstRoverPosition = input.nextLine();
        String firstRoverInstructions = input.nextLine();

        System.out.println("please enter the second rover position and instructions.");
        String secondRoverPosition = input.nextLine();
        String secondRoverInstructions = input.nextLine();
        input.close();  
        RoverPosition roverPositionInstruction=new RoverPosition();
        roverPositionInstruction.roversFinalPositionAndDirection(plateauDimentions, firstRoverPosition, firstRoverInstructions, secondRoverPosition, secondRoverInstructions);
    }
}

RoverPosition.java

package codechallenge;


public class RoverPosition {

    public String roversFinalPositionAndDirection(String plateuDimentionsInput, String rover1Position, String rover1Instructions, String rover2Position, String rover2Instructions) {
        validateInputsAreNotEmptyOrNegative(plateuDimentionsInput, rover1Position, rover1Instructions);
        validateInputsAreNotEmptyOrNegative(plateuDimentionsInput, rover2Position, rover2Instructions);
        validateRoversStartPositionNotIntersected(rover1Position, rover2Position);
        validatePositionOfRoverCannotBeOutsidePlateu(plateuDimentionsInput, rover1Position);
        validatePositionOfRoverCannotBeOutsidePlateu(plateuDimentionsInput, rover2Position);
        String finalPosition1 = roverPositionAndDirection(plateuDimentionsInput, rover1Position, rover1Instructions);
        String finalPosition2 = roverPositionAndDirection(plateuDimentionsInput, rover2Position, rover2Instructions);
        String finalPositionRover1Rover2=finalPosition1+"\n"+finalPosition2;
        validatePositionOfRoverCannotBeOutsidePlateu(plateuDimentionsInput, finalPosition1);
        validatePositionOfRoverCannotBeOutsidePlateu(plateuDimentionsInput, finalPosition2);
        validateRoverDirection(rover1Position);
        validateRoverDirection(rover2Position);
        validateRoversPositionToNotIntersect( finalPosition1, finalPosition2,rover2Position,rover1Position);

        return finalPositionRover1Rover2;
    }



    private String roverPositionAndDirection(String plateuDimentionsInput, String roverPosition, String roverInstructions) {
        String [] directionArr={"N","E","S","W"};
        String roverInitialDirection=roverPosition.split(" ")[2];
        int[] arr = extractXAndYAxisOfRovers(roverPosition);
        int roverX=arr[0];
        int roverY=arr[1];
        int directionIndex=0;
        directionIndex = getInitialDirectionIndex(directionArr, roverInitialDirection, directionIndex);
        for (int s=0;s<roverInstructions.length();s++){
            String currentDirection=directionArr[directionIndex];
            if(s==0&&roverInstructions.charAt(s)=='M'){
                if(currentDirection.equals("N"))
                    roverY++;
                else if(currentDirection.equals("S"))
                    roverY--;
                else if(currentDirection.equals("E"))
                    roverX++;
                else if(currentDirection.equals("W"))
                    roverX--;
            }
            directionIndex = moveDirectionRL(roverInstructions, directionArr, directionIndex, s);
            currentDirection=directionArr[directionIndex];
            char nextChar = 0;
            if(s!=roverInstructions.length()-1)
                nextChar=roverInstructions.charAt(s+1);

            if(nextChar=='M'){
                if(currentDirection.equals("N"))
                    roverY++;
                else if(currentDirection.equals("S"))
                    roverY--;
                else if(currentDirection.equals("E"))
                    roverX++;
                else if(currentDirection.equals("W"))
                    roverX--;
            }
        }
        String finalPosition=roverX+" "+roverY+" "+directionArr[directionIndex];
        return finalPosition;
    }



    private int getInitialDirectionIndex(String[] directionArr, String roverInitialDirection, int directionIndex) {
        for(int i=0;i<directionArr.length;i++){
            if(directionArr[i].equals(roverInitialDirection)){
                directionIndex=i;
                break;
            }
        }
        return directionIndex;
    }



    private int moveDirectionRL(String roverInstructions, String[] directionArr, int directionIndex, int s) {
        if(roverInstructions.charAt(s)=='R'){
            if(directionIndex==directionArr.length-1)
                directionIndex=0;
            else
                directionIndex++;
        }
        else if (roverInstructions.charAt(s)=='L'){
            if(directionIndex==0)
                directionIndex=directionArr.length-1;
            else
                directionIndex--;
        }
        return directionIndex;
    }
    private void validateRoversStartPositionNotIntersected(String rover1Position, String rover2Position) {
        int[] arr1 = extractXAndYAxisOfRovers(rover1Position);
        int[] arr2 = extractXAndYAxisOfRovers(rover2Position);
        int rover1X=arr1[0];
        int rover1Y=arr1[1];
        int rover2X=arr2[0];        
        int rover2Y= arr2[1];
        if(rover1X==rover2X&&rover1Y==rover2Y){ 
            throw new IllegalArgumentException("Error: Both Rovers cannot start in the same position.");
        }
    }
    private int[] extractXAndYAxisOfRovers( String roverPosition) {
        int roverX=0;
        int roverY=0;
        roverX=Integer.parseInt(roverPosition.split(" ")[0]);
        roverY=Integer.parseInt(roverPosition.split(" ")[1]);
        int arr[]= new int[4];
        arr[0]=roverX;
        arr[1]=roverY;
        return arr;
    }
    private void validateRoversPositionToNotIntersect(String finalPosition1,String finalPosition2,String rover2Position, String rover1Position) {
        int[] arr2 = extractXAndYAxisOfRovers(rover2Position);
        int rover2X=arr2[0];
        int rover2Y= arr2[1];

        int rover1FinalX=Integer.parseInt(finalPosition1.split(" ")[0]);
        int rover1FinalY=Integer.parseInt(finalPosition1.split(" ")[1]);
        int rover2FinalX=Integer.parseInt(finalPosition2.split(" ")[0]);
        int rover2FinalY=Integer.parseInt(finalPosition2.split(" ")[1]);
        if((rover1FinalX==rover2X &&rover1FinalY==rover2Y)||(rover2FinalX==rover1FinalX&&rover2FinalY==rover1FinalY))
        {
            throw new IllegalArgumentException("Error: Rovers positions cannot intersect.");
        }
    }
    private void validateRoverDirection(String roverPosition) {
        if(!roverPosition.contains("N")&&!roverPosition.contains("S")&&!roverPosition.contains("E")&&!roverPosition.contains("W"))
        {
            throw new IllegalArgumentException("Error: Rover position must be N S W or E.");
        }
    }
    private void validatePositionOfRoverCannotBeOutsidePlateu(String plateuDimentionsInput, String roverPosition) {
        int[] arr = extractXAndYAxisOfRovers(roverPosition);
        int roverX=arr[0];
        int roverY=arr[1];

        int plateuX=Integer.parseInt(plateuDimentionsInput.split(" ")[0]);
        int plateuY=Integer.parseInt(plateuDimentionsInput.split(" ")[1]);

        if(roverX>plateuX||roverY>plateuY)
        {
            throw new IllegalArgumentException("Error: The position of the rover cannot be outside the dimentions of the plateu");
        }
    }
    private void validateInputsAreNotEmptyOrNegative(String plateuDimentionsInput, String roverPosition,String roverInstructions) {
        if(roverPosition.isEmpty()||roverPosition.contains("-"))
            throw new IllegalArgumentException("Error: Rover position cannot be empty or contain any negative number and the position must be N S W or E.");
        else if(plateuDimentionsInput.isEmpty()||plateuDimentionsInput.contains("-"))
            throw new IllegalArgumentException("Error: Plateu dimentions cannot be empty or contain any negative number.");
        else if(roverInstructions.isEmpty())
            throw new IllegalArgumentException("Error: Instructions cannot be empty.");
    }
}

MarsRoverTest.java

package codechallenge;

import static org.junit.Assert.*;

import java.util.Arrays;
import java.util.Collection;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;

@RunWith(Parameterized.class)
public class MarsRoverTests {

    RoverPosition marsRover=new RoverPosition();
    private String plateuDimentionsInput;
    private String rover1Position;
    private String rover1Instructions;
    private String rover2Position;
    private String rover2Instructions;
    private String expectedoutput;

    public MarsRoverTests(String plateuDimentionsInput,String rover1Position,String rover1Instructions,String rover2Position,String rover2Instructions, String expectedoutput) {
        this.plateuDimentionsInput=plateuDimentionsInput;
        this.rover1Instructions=rover1Instructions;
        this.rover1Position=rover1Position;
        this.rover2Instructions=rover2Instructions;
        this.rover2Position=rover2Position;
        this.expectedoutput = expectedoutput;
    }

    @Parameters
    public static Collection<Object[]> testConditions(){
        return Arrays.asList(new Object[][] {
             { "5 5","1 2 N","LMLMLMLMM","3 3 E","MMRMMRMRRM", "1 3 N\n5 1 E" },
             { "5 5","3 3 E","MMRMMRMRRM","1 2 N","LMLMLMLMM", "5 1 E\n1 3 N" },
             { "1000 1000","10 7 S","MMMMMMRLMLLL","3 3 E","MMRMMRMRRM", "10 0 W\n5 1 E" },
             { "20 20","3 3 W","MMRMMRMRRMS","10 7 S","MMMMMMRLMLLL", "1 5 W\n10 0 W" }
          });
    }

    @Test
    public void testRoverPosition() {
          assertEquals(expectedoutput, marsRover.roversFinalPositionAndDirection(plateuDimentionsInput,rover1Position,rover1Instructions,rover2Position,rover2Instructions));
    }

}

MarsRoverTestsNegativeSenarios.java

package codechallenge;

import static org.junit.Assert.*;

import java.util.Arrays;
import java.util.Collection;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;


@RunWith(Parameterized.class)
public class MarsRoverTestsNegativeSenarios {

    RoverPosition marsRover=new RoverPosition();

    private String plateuDimentionsInput;
    private String rover1Position;
    private String rover1Instructions;
    private String rover2Position;
    private String rover2Instructions;
    private String expectedoutput;

    public MarsRoverTestsNegativeSenarios(String plateuDimentionsInput, String rover1Position,
            String rover1Instructions, String rover2Position,
            String rover2Instructions, String expectedoutput) {
        this.plateuDimentionsInput = plateuDimentionsInput;
        this.rover1Position = rover1Position;
        this.rover1Instructions = rover1Instructions;
        this.rover2Instructions=rover2Instructions;
        this.rover2Position=rover2Position;
        this.expectedoutput = expectedoutput;
    }

    @Parameters
    public static Collection<Object[]> testConditions(){
        return Arrays.asList(new Object[][] {
             { "5 5","3 3 X","MMRMMRMRRM","3 4 E","MMRMMRMRRM", "Error: Rover position must be N S W or E." },
             { "-5 -5","3 3 E","MMRMMRMRRM","3 4 E","MMRMMRMRRM", "Error: Plateu dimentions cannot be empty or contain any negative number." },
             { "5 5","-3 3 E","MMRMMRMRRM","3 3 E","MMRMMRMRRM", "Error: Rover position cannot be empty or contain any negative number and the position must be N S W or E."},
             { "5 5","3 3 E","MMRMMRMRRM","-3 3 E","MMRMMRMRRM", "Error: Rover position cannot be empty or contain any negative number and the position must be N S W or E."},
             { "10 10","20 3 E","MMRMMRMRRM","3 3 E","MMRMMRMRRM", "Error: The position of the rover cannot be outside the dimentions of the plateu" },
             { "10 10","3 3 E","MMRMMRMRRM","20 3 E","MMRMMRMRRM", "Error: The position of the rover cannot be outside the dimentions of the plateu" },
             { "10 10","10 3 E","MMRMMRMRRM","3 3 E","MMRMMRMRRM", "Error: The position of the rover cannot be outside the dimentions of the plateu" },
             { "10 10","10 3 E","","3 3 E","MMRMMRMRRM", "Error: Instructions cannot be empty." },
             { "10 10","","MMRMMRMRRM","3 3 E","MMRMMRMRRM", "Error: Rover position cannot be empty or contain any negative number and the position must be N S W or E." },
             { "","10 3 E","MMRMMRMRRM","3 3 E","MMRMMRMRRM", "Error: Plateu dimentions cannot be empty or contain any negative number." },
             { "5 5","1 2 N","LMLMLMLMM","1 2 E","MMRMMRMRRM", "Error: Both Rovers cannot start in the same position." },
             { "5 5","1 2 N","LMLMLMLMM","1 3 N","MMRMMRMRRM", "Error: Rovers positions cannot intersect." },
             { "5 5","1 2 N","LMLMLMLMM","3 3 E","MMRMMRMRRMLMMLMMMM", "Error: Rovers positions cannot intersect." }
          });
    }

    @Test(expected=IllegalArgumentException.class)
    public void testRoverPositionNegativeSenarios() {
        try
           {
             assertEquals(expectedoutput, marsRover.roversFinalPositionAndDirection(plateuDimentionsInput,rover1Position,rover1Instructions,rover2Position,rover2Instructions));
           }
           catch(IllegalArgumentException re)
           {
              assertEquals(expectedoutput, re.getMessage());
              throw re;
            }
    }

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

You did a good job. Program does what it should and you are validating your values, which is important. As for the issues I must say that this program is not object oriented. As in not at all. It is one big procedure with some sub-procedures. While this might be ok when the program is some kind of one-time effort that is not supposed to change, in the real world programs and their requirements tend to change. This change is what OOP can help you manage.

First issue that strikes the eyes is what is called Stringly Typed programming(Also called primitive obsession. One of so called design smells). It is something you will encounter very often. The problem is, that even though the Java is strongly typed language people often encode logic into Strings and other primitive types. Instead of encoding everything as a String you should introduce types that will represent those pieces of logic within your codebase.

Looking at the challenge description there are a couple concepts you could start with. Obvious ones are Instructions, Position of the rover which works with its sub-concepts a Coordinate and a Direction.

I quickly went through the code and extracted all position related code into a Position class which represents a position of a rover and produces a new position based on the incoming instruction. Notice that it is still quite ugly piece of code. This is because it does too many things. I.E. it does not have a single responsibility.

class Position {
    private final int x;
    private final int y;
    private final char direction;

    Position(final int x, final int y, final char direction) {
        this.x = x;
        this.y = y;
        this.direction = direction;
    }

    public Position move(final char instruction) {
        final int x;
        final int y;
        final char direction;
        if (instruction == 'M') {
            direction = this.direction;
            if(this.direction == 'N') {
                y = this.y + 1;
                x = this.x;
            } else if(this.direction == 'S') {
                y = this.y - 1;
                x = this.x;
            } else if(this.direction == 'E') {
                x = this.x + 1;
                y = this.y;
            } else if(this.direction == 'W') {
                x = this.x - 1;
                y = this.y;
            } else {
                throw new IllegalArgumentException("Unknown direction");
            }
        } else if (instruction == 'R') {
            x = this.x;
            y = this.y;
            if (this.direction == 'N') {
                direction = 'E';
            } else if (this.direction == 'S') {
                direction = 'W';
            } else if (this.direction == 'E') {
                direction = 'S';
            } else if (this.direction == 'W') {
                direction = 'N';
            } else {
                throw new IllegalArgumentException("Unknown direction");
            }
        } else if (instruction == 'L') {
            x = this.x;
            y = this.y;
            if (this.direction == 'N') {
                direction = 'W';
            } else if (this.direction == 'S') {
                direction = 'E';
            } else if (this.direction == 'E') {
                direction = 'N';
            } else if (this.direction == 'W') {
                direction = 'S';
            } else {
                throw new IllegalArgumentException("Unknown direction");
            }
        } else {
            throw new IllegalArgumentException("Unknown instruction");
        }
        return new Position(x, y, direction);
    }
}

If you extract these additional responsibilities into their respective classes you could end up with something like this:

class Position {
    private final Coordinate coordinate;
    private final Direction direction;

    Position(final Coordinate coordinate, final Direction direction) {
        this.coordinate = coordinate;
        this.direction = direction;
    }

    public Position move(final Instruction instruction) {
        final Direction direction = this.direction.turn(instruction);
        final Coordinate coordinate;
        if (instruction == Instruction.Move) {
            coordinate = this.coordinate.move(direction);
        } else {
            coordinate = this.coordinate;
        }
        return new Position(coordinate, direction);
    }
}

class Coordinate {
    private final int x;
    private final int y;

    Coordinate(final int x, final int y) {
        this.x = x;
        this.y = y;
    }
    public Coordinate move(final Direction direction) {
        final int x;
        final int y;
        if(direction == Direction.North) {
            y = this.y + 1;
            x = this.x;
        } else if(direction == Direction.South) {
            y = this.y - 1;
            x = this.x;
        } else if(direction == Direction.East) {
            x = this.x + 1;
            y = this.y;
        } else if(direction == Direction.West) {
            x = this.x - 1;
            y = this.y;
        } else {
            throw new IllegalArgumentException("Unknown direction");
        }
        return new Coordinate(x, y);
    }

}

enum Instruction {
    Move('M'),
    Left('L'),
    Right('R');

    private static final Map<Character, Instruction> LOOKUP = new HashMap<>();

    private final Character symbol;

    Instruction(final Character symbol) {
        this.symbol = symbol;
    }

    static {
        for (final Instruction i : Instruction.values()) {
            LOOKUP.put(i.symbol, i);
        }
    }

    public static Instruction of(final Character symbol) {
        final Instruction instruction = LOOKUP.get(symbol);
        if (instruction == null) {
            throw new IllegalArgumentException("Unknown instruction: '" + symbol + "'!");
        }
        return instruction;
    }

}

enum Direction {
    North('N') {
        @Override
        public Direction turn(final Instruction instruction) {
            final Direction direction;
            if (instruction == Instruction.Left) {
                direction = West;
            } else if (instruction == Instruction.Right) {
                direction = East;
            } else {
                direction = North;
            }
            return direction;
        }
    },
    South('S') {
        @Override
        public Direction turn(final Instruction instruction) {
            final Direction direction;
            if (instruction == Instruction.Left) {
                direction = East;
            } else if (instruction == Instruction.Right) {
                direction = West;
            } else {
                direction = South;
            }
            return direction;
        }
    },
    East('E') {
        @Override
        public Direction turn(final Instruction instruction) {
            final Direction direction;
            if (instruction == Instruction.Left) {
                direction = North;
            } else if (instruction == Instruction.Right) {
                direction = South;
            } else {
                direction = East;
            }
            return direction;
        }
    },
    West('W') {
        @Override
        public Direction turn(final Instruction instruction) {
            final Direction direction;
            if (instruction == Instruction.Left) {
                direction = South;
            } else if (instruction == Instruction.Right) {
                direction = North;
            } else {
                direction = West;
            }
            return direction;
        }
    };

    private static final Map<Character, Direction> LOOKUP = new HashMap<>();

    private final Character symbol;

    Direction(final Character symbol) {
        this.symbol = symbol;
    }

    static {
        for (final Direction d : Direction.values()) {
            LOOKUP.put(d.symbol, d);
        }
    }

    public abstract Direction turn(final Instruction instruction);

    public static Direction of(final Character symbol) {
        final Direction direction = LOOKUP.get(symbol);
        if (direction == null) {
            throw new IllegalArgumentException("Unknown direction: '" + symbol + "'!");
        }
        return direction;
    }
}

These are just examples of what can be done. I haven't tested if everything works and there might be some typos.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

Welcome to our community. First of all your roverPositionAndDirection() method, and MarsRoverTestsNegativeSenarios() and MarsRoverTests() constructors have to many parameters. Use up to two or if you absolutely need at most three, but mostly only just one. This is needed for the code to be more clear. Maybe you should use a custom data object to be used as a parameter for these methods, some kind of RoverCommand class that contains String plateuDimentionsInput, String rover1Position, String rover1Instructions, String rover2Position, String rover2Instructions.

For this kind of code best use switch rather than if statements for more readability.

if(currentDirection.equals("N"))
     roverY++;
else if(currentDirection.equals("S"))
     roverY--;
else if(currentDirection.equals("E"))
     roverX++;
else if(currentDirection.equals("W"))
     roverX--;

And here I see code duplication.

directionIndex = getInitialDirectionIndex(directionArr, roverInitialDirection, directionIndex);
        for (int s=0;s<roverInstructions.length();s++){
            String currentDirection=directionArr[directionIndex];
            if(s==0&&roverInstructions.charAt(s)=='M'){
                if(currentDirection.equals("N"))
                    roverY++;
                else if(currentDirection.equals("S"))
                    roverY--;
                else if(currentDirection.equals("E"))
                    roverX++;
                else if(currentDirection.equals("W"))
                    roverX--;
            }
            directionIndex = moveDirectionRL(roverInstructions, directionArr, directionIndex, s);
            currentDirection=directionArr[directionIndex];
            char nextChar = 0;
            if(s!=roverInstructions.length()-1)
                nextChar=roverInstructions.charAt(s+1);

            if(nextChar=='M'){
                if(currentDirection.equals("N"))
                    roverY++;
                else if(currentDirection.equals("S"))
                    roverY--;
                else if(currentDirection.equals("E"))
                    roverX++;
                else if(currentDirection.equals("W"))
                    roverX--;
            }
        }

You should make a method that is uses to calculate any direction, and not to write the same code multiple times.

In if statement it is preferred to use boolean methods, again for more clear code.

So instead of:

if(roverX>plateuX||roverY>plateuY)
        {
            throw new IllegalArgumentException("Error: The position of the rover cannot be outside the dimentions of the plateu");
        }

You should have:

if(!isOnThePlateu())
{
    throw new IllegalArgumentException("Error: The position of the rover cannot be outside the dimentions of the plateu");
}

private boolean isOnThePlateu()
{
     return roverX>plateuX||roverY>plateuY;
}

Overall your code looks far to complex for it's simple task. Hope this helps you to improve yourself in coding.

I suggest you use GitHub if you don't already use it, and than in the Marketplace page you should subscribe to CodeBeat, Codacy and BetterCodeHub apps for automated code review. It is free of charge for public repositories. It is very helpful.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

Console Isolation

You've done a good job separating the console from the actual logic of your code. This small thing makes your code much easier to reuse in different applications.

Parameterised Tests

Parameterised tests can be great for running related tests against test data. However, there's a risk that they can obscure what it is that you're trying to test. So, looking at one of your test cases:

{ "10 10","","MMRMMRMRRM","3 3 E","MMRMMRMRRM", "Error: Rover position cannot be empty or contain any negative number and the position must be N S W or E." }

The error message at the end gives you a hint what to look for, however if the test fails, you then have to try and understand which bit(s) of the parameter block are relevant to the failure / test case. A suite of smaller, more targeted tests can be both easier to understand and can encourage you to break down your application into smaller parts.

As has been been mentioned, there's various ways that the application could be broken down. One approach might be to create a CardinalVector class to represent a movement direction/size along the primary compass directions. One way of implementing it might be:

class CardinalVector {
    public final int x;
    public final int y;
    public final String direction;

    private final String rotateRightDirection;
    private final String rotateLeftDirection;

    private final static Map<String, CardinalVector> cardinalDirections = Map.of(
            "N", new CardinalVector("N", 0, 1, "E", "W"),
            "E", new CardinalVector("E", 1, 0, "S", "N"),
            "S", new CardinalVector("S", 0, -1, "W", "E"),
            "W", new CardinalVector("W", -1, 0, "N", "S"));

    private CardinalVector(String direction, int x, int y, 
                           String rotateRightDirection, String rotateLeftDirection) {
        this.direction = direction;
        this.x = x;
        this.y = y;
        this.rotateRightDirection = rotateRightDirection;
        this.rotateLeftDirection = rotateLeftDirection;
    }

    public static CardinalVector of(String direction) {
        return cardinalDirections.get(direction);
    }

    public CardinalVector rotated(String rotationDirection) {
        return rotationDirection.equals("R") ? of(rotateRightDirection) 
                                             : of(rotateLeftDirection);
    }
}

As this class represents a smaller subset of the functionality, it's possible to create some more fine-grained unit-tests. For example, when creating a vector for a given direction (NSEW), are the x/y movement fields set correctly:

@ParameterizedTest
@MethodSource("direction_expectedX_expectedY")
void of_direction_expectedX_expectedY(String direction, int expectedX, int expectedY) {
    CardinalVector north = CardinalVector.of(direction);
    assertThat(north.x).isEqualTo(expectedX);
    assertThat(north.y).isEqualTo(expectedY);

}

static Stream<Arguments> direction_expectedX_expectedY() {
    return Stream.of(
            arguments("N", 0, 1),
            arguments("E", 1, 0),
            arguments("S", 0, -1),
            arguments("W", -1, 0)
    );
}

I try to keep my test names following something along the lines of method being tested (of), condition being tested (direction), desired outcome (expectedX, expectedY). By keeping the arguments for the parameterised test in the same order, it can make the test results easier to understand. So, with a test to validate that from a given starting direction, rotating results in a different direction:

@ParameterizedTest
@MethodSource("fromDirection_rotationDirection_expectedDirection")
void rotated_fromDirection_rotationDirection_expectedDirection(String fromDirection,
                                                               String rotationDirection,
                                                               String expectedDirection) {
    CardinalVector rotatedDirection = CardinalVector.of(fromDirection)
                                                    .rotated(rotationDirection);
    assertThat(rotatedDirection).isEqualTo(CardinalVector.of(expectedDirection));
}

static Stream<Arguments> fromDirection_rotationDirection_expectedDirection() {
    return Stream.of(
            arguments("N", "R", "E"),
            arguments("N", "L", "W"),
            arguments("E", "R", "S"),
            arguments("E", "L", "N"),
            arguments("S", "R", "W"),
            arguments("S", "L", "E"),
            arguments("W", "R", "N"),
            arguments("W", "L", "S")
    );
}

In my test runner the output looks like this:

test results

You can see that the test name helps to understand each of the inputs / results for the parameterised test runs below.

Some other things to think about

  • The problem description you link to talks about a squad of rovers. So, whilst the the example input/output is for two rovers, I would expect a solution to be able to handle a different number of rovers (1, 3 etc). At the moment, you've got a RoverPosition class that actually tracks the position + instructions for two different rovers, along with the plateau. To add another Rover, we'd need to add another two parameters to the roversFinalPositionAndDirection method, which quickly gets unwieldy.

  • Method names should describe what it is the method does, consequently you'd expect some kind of 'action'. This isn't always the case, for example roverPositionAndDirection. This sounds like the name of a variable that contains the rover's current position and direction, whereas it actually calculates the final position of the rover. This is misleading.

  • Unused parameters. Don't pass things to methods that you don't need. So, for example roverPositionAndDirection never uses its plateuDimentionsInput parameter.
  • Redundant variables. Rather than declaring a variable and assigning it a value then immediately returning it, it's often better to just return the assigned value (if the function is named correctly then it should be obvious what's being returned). So, for example, instead of:

    String finalPosition=roverX+" "+roverY+" "+directionArr[directionIndex];
    return finalPosition;
    

    Consider

    return roverX+" "+roverY+" "+directionArr[directionIndex];
    
  • Consistency is key... Most modern IDEs are able to format your code for you. Java tends to favour more spacing than some other languages, for me personally, I just like consistency... so rather than:

    int rover2X=arr2[0];
    int rover2Y= arr2[1];
    

    Pick a consistent spacing and use it everywhere, or get your IDE to do it for you. Generally, the preference is:

    int rover2X = arr2[0];
    
  • Array declarations in java should be: int[] arr = new int[4];, rather than int arr[]= new int[4];

  • moveDirectionRL, it's great that you've created a method to encapsulate the logic for turning the rover. I would have perhaps gone with rotate or turn to make it clearer what it was responsible for. It would also have been good to do a similar thing to encapsulate the logic for actually moving the rover in it's current direction.

  • Plateau safety... You check if the rover position is outside the top/right, however you don't check if it goes off the bottom/left, so negative positions don't throw an exception. You also only check the rover is outside of the plateau at the beginning and end. So, it's acceptable for the rover to wonder off outside the limits, as long as it comes back. This would obviously be dangerous for an actual rover.

| improve this answer | |
\$\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.