Toy Robot Simulator

Question

I know this code worked as I've tested it with the case tests commented below in the code, and even added a few of my own, but the developers who were testing me came back with the following critiques:

1. All in one file, no attempt to modularise or separate out concerns
2. No tests at all. You could possibly forgive an entry level submission for 0% tests but seniors should be demonstrating their value
3. Everything is declared as let variables which suggests they can be overridden. I would assume most of them should be consts. Again, senior should demonstrate this
4. The submission can be difficult to read with the excessive comments and everything altogether

I included their original tests, as well as three of my own, commented out within the code below. I'm not sure whether these were ignored or dismissed. Could anyone shed some light as to how this code could have been improved? If I get another coding test, I'd like to have some insight as to what is expected in terms of code formatting with respect to these sorts of comments.

Toy Robot Simulator

Description

• The application is a simulation of a toy robot moving on a square tabletop, of dimensions 5 units x 5 units.
• There are no other obstructions on the table surface.
• The robot is free to roam around the surface of the table, but must be prevented from falling to destruction. Any movement that would result in the robot falling from the table must be prevented, however further valid movement commands must still be allowed.

Create an application that can read in commands of the following form:

PLACE X,Y,F
MOVE
LEFT
RIGHT
REPORT

• PLACE will put the toy robot on the table in position X,Y and facing NORTH, SOUTH, EAST or WEST.
• The origin (0,0) can be considered to be the SOUTH WEST most corner.
• The first valid command to the robot is a PLACE command, after that, any sequence of commands may be issued, in any order, including another PLACE command. The application should discard all commands in the sequence until a valid PLACE command has been executed.
• MOVE will move the toy robot one unit forward in the direction it is currently facing.
• LEFT and RIGHT will rotate the robot 90 degrees in the specified direction without changing the position of the robot.

• REPORT will announce the X,Y and F of the robot. This can be in any form, but standard output is sufficient.

• A robot that is not on the table can choose the ignore the MOVE, LEFT, RIGHT and REPORT commands.

• Input can be from a file, or from standard input, as the developer chooses.
• Provide test data to exercise the application.

Constraints

• The toy robot must not fall off the table during movement. This also includes the initial placement of the toy robot.
• Any move that would cause the robot to fall must be ignored.

Example Input and Output

Example a

PLACE 0,0,NORTH
MOVE
REPORT

Expected output:

0,1,NORTH

Example b

PLACE 0,0,NORTH
LEFT
REPORT

Expected output:

0,0,WEST

Example c

PLACE 1,2,EAST
MOVE
MOVE
LEFT
MOVE
REPORT

Expected output

3,3,NORTH

Deliverables

The Ruby source files, the test data and any test code.

It is not required to provide any graphical output showing the movement of the toy robot.

// Toy Robot!

/* A few caveats for this solution
    1) Input is said to be of a text format, we will make this an array with each element containing a command
        e.g. ["PLACE 1,2,NORTH", "MOVE", "LEFT", "MOVE", "REPORT"]
    2) I'm doing this in NodeJS (JavaScript)
    3) To run this one could run this directly in the Node terminal, or copy and paste it into JSBin.com and execute it there; 
        or reference it in an HTML file; so in saying that, the executed input will be located at the bottom of the functional definition.

*/

/* CODE STARTS */

"use strict"
let Robot = function(aCommands) {

    let iTableX = 5; // Width of Table
    let iTableY = 5; // Height of Table
    let aDir = ["NORTH","EAST","SOUTH","WEST"]; // Directions
    let sRobotDir = 0; // Sets the default based off the above array
    let aCommandSplit = [];

    // Sets default setting for robot's coordinates X, Y
    let iRobotX = 0;
    let iRobotY = 0;
    let bRobotPlaced = false; // Determines if the robot has been placed on the board...
    let aTokens = []; // Sets up a basic array to hold our tokens

    // Can I tell you I'm already enjoying programming this? :D

    /* Let's set up some basic rules here
    1) We split each element into a command and optional options (options only available with PLACE command)
    2) We iterate through each array element, anything but a starting PLACE command is ignored.
    3) As we proceed through each element, a switch command will assess the value of the command, and then takes an appropriate action.
    */

    // Tokenizing engine
    for(let iLoop = 0, iLen = aCommands.length; iLoop < iLen; iLoop++) {
        aCommandSplit = aCommands[iLoop].toUpperCase().split(" "); // Initiates primary split, ensuring all commands are upper case
        if(aCommandSplit.length == 2) {
            aCommandSplit[1] = aCommandSplit[1].split(","); // split parameters
        } else {
            aCommandSplit[1] = []; // defafult
        }
        aTokens.push(aCommandSplit);            
    }

    // Invalid Command
    let invalidCommand = function(item) {
        console.log(">> INVALID COMMAND: '"+item.join(" ")+"' IGNORED");
    };

    // Checks if value is below zero
    let isBelowZero = (val) => (val < 0);

    // Checks if value is equal to or above a certain limit
    let isBeyondTableLimit = (val,limit) => (val >= limit);

    // Checks if string represents a positive value
    let isPosInteger = function (str) {
        var n = Math.floor(Number(str));
        return n !== Infinity && String(n) === str && n >= 0;
    };

    // Checks to see if the string is a direction in the direction array
    let isDirection = (str) => !!~aDir.indexOf(str);

    // Returns the index of the direction string
    let whichDirection = (str) => aDir.indexOf(str);

    // Validates the parameters for placement
    let checkPlaceParams = (arr) => (
            (arr != []) &&                              // array isn't empty
            (arr.length == 3) &&                        // array contains three elements
            isPosInteger(arr[0]) &&                     // element 0 is a positive integer or zero
            isPosInteger(arr[1]) &&                     // element 1 is a positive integer or zero
            (typeof(arr[2]) == "string") &&             // element 2 is a string
            isDirection(arr[2]) &&                      // element 2 is a direction
            !isBelowZero(+arr[0]) &&                    // element 0 is equal to zero or above
            !isBelowZero(+arr[1]) &&                    // element 1 is equal to zero or above
            !isBeyondTableLimit(+arr[0], iTableX) &&    // element 0 is not bigger than the table size
            !isBeyondTableLimit(+arr[1], iTableY)       // element 1 is not bigger than the table size
        );

    // Sets the placement
    let place = function(item) {
        if(checkPlaceParams(item[1])) {
            iRobotX = +item[1][0];
            iRobotY = +item[1][1];
            sRobotDir = whichDirection(item[1][2]);
            bRobotPlaced = true;
        }
    };

    // Rotates the robot left or right.
    let rotate = function(turn) {
        let newDir = (sRobotDir + ((turn=="LEFT")?3:1))%4;
        sRobotDir = newDir;
    };
    // Checks to see if the robot can move in a certain direction
    let canMove = function() {
        switch(sRobotDir) {
            case 0: // North
                return (!isBeyondTableLimit(iRobotY+1, iTableY));
                break;
            case 1: // East
                return (!isBeyondTableLimit(iRobotX+1, iTableX));
                break;
            case 2: // South
                return (!isBelowZero(iRobotY-1));
                break;
            case 3: // West
                return (!isBelowZero(iRobotX-1));
                break;
        }
    }
    // Moves the robot
    let move = function() {
        if(sRobotDir % 2 == 1) { // If east or west
            if(sRobotDir == 1) { // if east
                iRobotX++;
            } else {
                iRobotX--;
            }
        } else {
            if(sRobotDir == 0) { // if north
                iRobotY++;
            } else {
                iRobotY--;
            }
        }
    };

    // Reports position and direction facing
    let report = function() {
        console.log([iRobotX,iRobotY,aDir[sRobotDir]].join(","));
    };

    // Step through the tokens
    aTokens.forEach(function(item, index) {
        switch (item[0]) {
            case "PLACE":
                place(item);
                break;
            case "LEFT":
            case "RIGHT":
                bRobotPlaced ? rotate(item[0]) : invalidCommand(item);
                break;
            case "MOVE":
                (bRobotPlaced && canMove()) ? move() : invalidCommand(item);
                break;
            case "REPORT":
                bRobotPlaced ? report() : invalidCommand(item);
                break;
            default:
                invalidCommand(item);
                break;
        }
    });
};  

/* CODE ENDS */

/* EXECUTE BLOCK STARTS */

    // Original Test Cases
    // Robot(["PLACE 0,0,NORTH","MOVE","REPORT"]);  // 0,1,NORTH
    // Robot(["PLACE 0,0,NORTH","LEFT","REPORT"]); // 0,0,WEST
    // Robot(["PLACE 1,2,EAST","MOVE","MOVE","LEFT","MOVE","REPORT"]);  // 3,3,NORTH

    // My Test Cases
    // Robot(["PLACE 1,2,NORTH", "MOVE", "LEFT", "MOVE", "REPORT"]);      // Expected result: 0,3,WEST
    // Robot(["MOVE","PLACE 0,3,WEST","MOVE","RIGHT","MOVE","MOVE","REPORT"]); // Expected result: 0,4,NORTH
    /* NOTE: Robot should ignore first MOVE command because it hasn't been PLACE-d yet, and then ignore the second MOVE command because it can't go any further west, be able to move on the third MOVE command, but then ignore the last MOVE command because it can't go any further north. */
    //Robot(["PLACE 2,2,NORTH","KEFT","NOVE","MOVE","REPORT"]);
    /* Should ignore the "KEFT" command as it can't rotate "KEFT", then ignore the "NOVE" command, but obey the "MOVE" command. 
        Final result: 2,3,NORTH */

/* EXECUTE BLOCK ENDS */

Answer 1

Aside from the given critiques (which are all correct) I have following notes:

• Prefixing variable names with its type (Hungarian notation) is generally considered bad style. (BTW, what does the s in sRobotDir stand for?)
• Aside from using let instead of const, constants such as (iTableX, iTableY or aDir should be defined outside the main function (which would require modularization to avoid them becoming global variables). Also it would be worth considering making iTableX and iTableY configurable, so that different sized tables can be used at run-time.
• Tokenization is too fragile for my taste. It should either should be able to handle superfluous spaces, or be more strict on what it accepts and throw more/earlier errors if it finds something invalid.
• Having the helper functions in the middle of main function breaks the reading flow. I'd place them at the end or outside the function (again requiring modularization). Also using the conventional function statement instead of lambda notation would make them more readable.
• The function place only needs item[1] as its parameter, and item isn't a good variable name here.
• You have a few places where you use == instead of ===.
• I'm not sure if isBelowZero and isBeyondTableLimit are a good idea. I'd go for something like:

function createLimitsCheck(min, max) {
   return value => value >= min && value < max;
} 
const checkLimitsX = createLimitsCheck(0, iTableX);
const checkLimitsY = createLimitsCheck(0, iTableY);

• !!~ in isDirection is unnecessarily cryptic.
• isPosInteger is over-engineered. Considering you'll need the integer value (for which you just the unary + operator anyway) and check it against the table limits later, I'd drop it altogether.
• checkPlaceParams is also over-engineered with multiple unnecessary tests:
  • arr != [] can never return false, because it compares object references and not contents.
  • I don't believe arr[2] can ever be anything else than a string. And even if it would be isDirection would then return false. (BTW, typeof is an operator not a function, so don't use brackets: typeof arr[2].)
  • isBelowZero is unnecessery together with isPosInteger which already checked that.
  • You repeat the conversion to numbers here and back in place again.

Personnally I'd rewrite checkPlaceParams as follows:

function checkPlaceParams(params) {
   const x = +params[0];
   const y = +params[1];
   const dir = whichDirection(params[2]);
   if (checkLimitsX(x) && checkLimitsY(y) && dir > -1) {
       return [x, y, dir];
   }
   return; // undefined
 }

and place as

function place(params) { // only pass item[1]
   const validatedParams = checkPlaceParams(params || []) // avoid passing undefined
   if (validatedParams) {
       [iRobotX, iRobotY, sRobotDir] = validatedParams;
       bRobotPlaced = true;
   } else {
       console.log( ... );
   }
}