2
\$\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;
            }
    }

}
New contributor
Sara Elmenshawy is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
\$\endgroup\$
0
\$\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 Marketing 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.

\$\endgroup\$

Your Answer

Sara Elmenshawy is a new contributor. Be nice, and check out our Code of Conduct.

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.