# C# Toy Robot Simulator

I did this coding exercise for a potential employer who failed it on the basis of unspecified style and structure issues and would appreciate any thoughts on what flaws this contains, in particular in relation to style, structure or any other important software design principles.

Program.cs

namespace ToyRobotSimulator
{
using System;
public static class Program
{
//Reads console input and executes command
public static void Main()
{
bool run = true;

var handler = new CommandHandler(new Robot());

while (run)
{
try
{
}
catch (Exception e)
{
Console.WriteLine("Invalid Input: " + e.Message);
}
}
}
}
}


CommandHandler.cs

namespace ToyRobotSimulator
{
using System;
public class CommandHandler : ICommandHandler
{
private IRobot _rob;
private bool _placeHasBeenExecuted = false;

public CommandHandler(IRobot rob)
{
_rob = rob;
}

//Handles command. Requires valid placement to occur before executing any other command
public bool HandleCommand(string[] command)
{
if (command == null) { throw new ArgumentNullException("command is null"); }

switch (command[0])
{
case DataConstants.PlaceName:
var placed = command.Length == 4 && _rob.Place(new PlacementRequest
{
X = Helpers.GetNumberFromString(command[1]),
Y = Helpers.GetNumberFromString(command[2]),
F = Helpers.GetDirectionFromString(command[3])
});

Helpers.RunCommandIfTrue(() => _placeHasBeenExecuted = placed, _placeHasBeenExecuted == false);
break;
case DataConstants.MoveName:
Helpers.RunCommandIfTrue(() => _rob.Move(), _placeHasBeenExecuted);
break;
case DataConstants.LeftName:
Helpers.RunCommandIfTrue(() => _rob.Left(), _placeHasBeenExecuted);
break;
case DataConstants.RightName:
Helpers.RunCommandIfTrue(() => _rob.Right(), _placeHasBeenExecuted);
break;
case DataConstants.ReportName:
Helpers.RunCommandIfTrue(() => _rob.Report(), _placeHasBeenExecuted);
break;
default:
break;
}

return _placeHasBeenExecuted;
}
}
}


Robot.cs

namespace ToyRobotSimulator
{
using System;
public class Robot : IRobot
{
private int? _x;
private int? _y;
private Direction? _f;

//Sets _x, _y and _f values if request contains valid data and returns a bool to indicate success or failure
public bool Place(PlacementRequest req)
{
var placed = false;

if (req != null && Helpers.IsValidPlacement(req.X) && Helpers.IsValidPlacement(req.Y) && req.F != null)
{
_x = req.X;

_y = req.Y;

_f = req.F;

placed = true;
}

return placed;
}

//Moves robot one unit in direction faced
public void Move()
{
switch (_f)
{
case Direction.NORTH:
Helpers.RunCommandIfTrue(() => _y++, Helpers.IsValidPlacement(_y + 1));
break;
case Direction.SOUTH:
Helpers.RunCommandIfTrue(() => _y--, Helpers.IsValidPlacement(_y - 1));
break;
case Direction.EAST:
Helpers.RunCommandIfTrue(() => _x++, Helpers.IsValidPlacement(_x + 1));
break;
case Direction.WEST:
Helpers.RunCommandIfTrue(() => _x--, Helpers.IsValidPlacement(_x - 1));
break;
default:
break;
}
}

//Calls turn with direction as increment of _f
public void Left()
{
Turn(() => (int)_f + 1);
}

//Calls turn with direction as decrement of _f
public void Right()
{
Turn(() => (int)_f - 1);
}

//If value less than zero default, otherwise calculate direction based on modulo
private void Turn(Func<int> newDirection)
{
_f = (newDirection() < 0) ? Direction.EAST : (Direction)(newDirection() % 4);
}

//Outputs comma-separated string with _x,_y and _f values
public void Report()
{
Console.WriteLine(\$"Output: {_x},{_y},{_f}");
}
}
}


Helpers.cs


namespace ToyRobotSimulator
{
using System;
public static class Helpers
{
public static int? GetNumberFromString(string input)
{
int? result = null;
int res;
var success = int.TryParse(input, out res);
return success ? res : result;
}

public static Direction GetDirectionFromString(string input)
{
Object direction;
Enum.TryParse(typeof(Direction), input, out direction);
return (Direction)direction;
}

public static bool IsValidPlacement(int? a)
{
var startIndex = 0;

var endIndex = 4;

return a >= startIndex && a <= endIndex;
}

public static string[] GetArrayFromSplitInput(string input)
{
return input?.Split(new string[] { " ", "," }, StringSplitOptions.None);
}

public static void RunCommandIfTrue(Action action, bool condition)
{
if (condition && action != null)
{
action.Invoke();
}
}
}
}


Adding the repo for the full project for context, but most of the logic is in the code above: https://github.com/dennis87532/ToyRobotSimulator.

Edit: These are the requirements for the app:

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.



I will try to focus on things that have not been said, but I apologise in advance if I have repeated something that has been mentioned in any previous answers.

Separating your code into Constatnts, Enums and Interfaces folders probably isn't particularly helpful. Especially when you have only one or two files in each folder.

Similarly naming a folder src isn't particularly helpful. Aside from the fact that src is a contraction (it would make more sense to call it Source in full), you have source files outside your src folder, which sends mixed messages to your end users and confuses matters. If you have a 'source' folder then it should contain all of your source code, not just half of your source code.

DataConstants is too generic a name - you're not going to stuff all your constants into a single class (or at least you shouldn't be). Instead your command names should be in a class specifically designated as CommandNames.

GetNumberFromString:

Firstly the name is misleading. You should specify that the function only parses integers rather than generic 'numbers' lest anyone using your API think that decimal points are acceptable. A better name would be ParseInt or ParseIntAsNullable.

The result variable is redundant. You should get rid of it. You could in fact simplify the whole function as thus:

public static int? GetNumberFromString(string text)
{
int result;
return int.TryParse(text, out result) ? result : null;
}


Or, if you have access to C# 7.0 or later then it's a one-liner:

public static int? GetNumberFromString(string text)
{
return int.TryParse(text, out int result) ? result : null;
}


GetDirectionFromString:

Does this actually compile for you? According to the documentation (and my compiler), Enum.TryParse only has two overloads, and neither match the arguments you're trying to feed it.

Additionally, you aren't reporting what happens if the parse fails - you're swallowing/ignoring the error. You should either throw an exception like a Parse function does (e.g. Enum.Parse) or you should turn this into a proper TryParse function that returns a bool indicating success and passes the enumeration out as an out parameter.

Also rather than lumping this in with a load of generic Helpers, it might be wiser to put is in a specific static class named DirectionUtils (see Helpers vs Utils). Said class could even be kept in the same file as Direction given how related the two are and how minimalist the helper class would be.

Lastly rather than GetDirectionFromString, this function should simply be named ParseDirection or Parse.

So essentially you should end up with something like:

public static class DirectionUtils
{
public static Direction Parse(string input)
{
return Enum.Parse<Direction>(input);
}

public static bool TryParse(string input, out Direction direction)
{
return Enum.TryParse<Direction>(input, out direction);
}
}


However, note that all this seemingly does is forward to the Enum versions, so perhaps you should just stick to using those directly.

IsValidPlacement:

I'd actually recommend turning this into a PlacementValidator object, so that you could easily change which placements are considered valid if the requirements changed.

E.g.

public class PlacementValidator
{
public PlacementValidator(int min, int max)
{
this.Min = min;
this.Max = max;
}

public int Min { get; private set; }

public int Max { get; private set; }

public bool IsValid(int position)
{
return ((position >= Min) && (position <= Max));
}
}


Then of course this would have to be weaved into the rest of your code. I would recommend putting this kind of validation either in the input validation or the command handler rather than the robot. It makes more sense for the robot to simply be a dumb puppet/bag-of-data that doesn't validate anything, because that makes it more flexible.

RunCommandIfTrue:

Completely redundant, and adds to the confusion by making the condition the last argument. Just use an if statement instead.

HandleCommand:

Something nobody else seems to have mentioned: you check if command is null, but you don't bother to check if command.Length > 0 before trying to access command[0], which would net you an out of bounds error if someone passed in an array with a length of 0.

Your Helpers functions seem to be quite unrelated and should probably be kept under separate APIs. E.g. GetNumberFromString should be grouped with GetArrayFromSplitInput because they're both very general, but GetDirectionFromString and IsValidPlacement are more specialist.

Your use of RunCommandIfTrue is actually obscuring your code. While I don't agree that functional style code has no place in C#, this is certainly not the place where it belongs.

This seems to me like a case of "I have a hammer syndrome" or "look what I can do syndrome" - the desire to show off something one has learned either without knowing where to make use of it or without having a suitable place to use it. This isn't unusual, near enough everybody experiences it at some point.

Your code would be much clearer and probably more efficient if you simply used an if statement.

More crucially, the fact that _placehasBeenExecuted might be false when Move, Left, Right or Report are selected seems to me like it might be an error condition that you should be reporting properly through use of a specific exception (either an InvalidOperationException or a user-defined exception such as InvalidCommandException or RobotNotPlacedException).

You also appear to be lacking errors for an unrecognised command and a place command with an unsuitable number of arguments.

Putting all these things together, a naive improvement would be:

public void HandleCommand(string[] command)
{
if (command == null)
throw new ArgumentNullException("command");

if (command.Length == 0)
throw new ArgumentException("command is empty");

switch (command[0])
{
case CommandNames.Place:
if(command.Length < 4)
throw new CommandParameterException(string.Format("Not enough parameters. Expected 4, got {0}", command.Length));

int x;
if(!int.TryParse(command[1], out x))
throw new CommandParameterException(string.Format("Place parameter 0 is not a proper integer: {0}", command[1]));

int y;
if(!int.TryParse(command[2], out y))
throw new CommandParameterException(string.Format("Place parameter 1 is not a proper integer: {0}", command[2]));

Direction direction;
if(!DirectionUtils.TryParse(command[3], out direction))
throw new CommandParameterException(string.Format("Place parameter 2 is not a proper direction: {0}", command[3]));

var placementRequest = new PlacementRequest() { X = x, Y = y, F = direction };

_placeHasBeenExecuted = _rob.Place(placementRequest);
break;
case CommandNames.Move:
if(_placeHasBeenExecuted)
_rob.Move();
else
throw new RobotNotPlacedException("Attempt to use the robot before it has been placed");
break;
case CommandNames.Left:
if(_placeHasBeenExecuted)
_rob.Left();
else
throw new RobotNotPlacedException("Attempt to use the robot before it has been placed");
break;
case CommandNames.Right:
if(_placeHasBeenExecuted)
_rob.Right();
else
throw new RobotNotPlacedException("Attempt to use the robot before it has been placed");
break;
case CommandNames.Report:
if(_placeHasBeenExecuted)
_rob.Report();
else
throw new RobotNotPlacedException("Attempt to use the robot before it has been placed");
break;
default:
throw new UnrecognisedCommandException(string.Format("Unrecognised command: {0}", command[0]));
}
}


However, as a number of these things are technically problems with the input data rather than the code itself, it may actually be preferable to separate the input validation from the processing logic and actually store the commands in a format that cannot be invalid so that HandleCommand (see @IEatBagels's answer for one possible solution to this, and see here for more info on the command pattern).

At the very least, it makes sense to translate the user's input into some kind of CommandType enum rather than operating on text, that way you can more easily separate format errors (e.g. 'x is not an integer') from logic errors (e.g. 'the robot cannot move because it hasn't been placed').

Robot.Turn:

There's actually two places where I think this is going wrong.

Firstly, using a Func here is nothing but a waste of time and resources. It acheives absolutely nothing.

Secondly, you should be hiding the ugly casting that's going on. Rather than making Left and Right cast an enum to an int, you should hide that away in a function in the newly added DirectionUtils class. E.g.

public static class DirectionUtils
{
public static Direction Next(Direction direction)
{
return (Direction)(((int)direction + 1) % 4);
}

public static Direction Previous(Direction direction)
{
return (direction == Direction.NORTH) ? Direction.EAST : (Direction)((int)direction - 1);
}
}


And then you can scrap Turn in favour of:

public void Left()
{
_f = DirectionUtils.Next(_f);
}

public void Right()
{
_f = DirectionUtils.Previous(_f);
}


(Which arguably should be called TurnLeft and TurnRight. Methods should ideally be verbs.)

Your commenting style needs improvement

• Either you comment all methods or none
• This is generally not a good rule of thumb, but for coding tasks in a job interview setting can show consistency
• If you comment a method use "DocComment" style, i.e.
///<summary>
/// Moves robot one unit in direction faced
///</summary>
public void Move()
{
...
}

• If you comment on specific lines don't say what the line does (this should be obvious from the code) but why you implemented it the way you did
//Calls turn with direction as increment of _f     <- useless comment
public void Left()
{
Turn(() => (int)_f + 1);
}

• Only use comments if there is absolutely no other way to express it in code

You use code constructs that are not fit for the purpose

• The CommandHandler.cs contains a switch with a single case and the code in there is not even poperly indented. Also a case should IMHO not contain more than two or three lines. If you need to handle more, extract a method

Naming is hard

You should double check your naming.

public static bool IsValidPlacement(int? a)
{
var startIndex = 0;
var endIndex = 4;

return a >= startIndex && a <= endIndex;
}


doesn't look bad, but consider the following changes

public static bool IsValidPlacement(int placementIndex)
{
const int minPlacementIndex = 0;
const int maxPlacementIndex = 4;

return placementIndex >= minPlacementIndex
&& placementIndex <= maxPlacementIndex;
}

• Even if you don't touch the vars make it explicitly clear to others that these values cannot be changed.
• Keep the wording, don't use start and end when they refer to a min and max
• Do not accept int? when you know that you absolutely must have a value here.
• Thank you for your comments, i will definitely reconsider naming, when to use nullable variables and commenting in the future. One thing though, the CommandHandler contains 6 cases including default, not 1, (the lack of indentation probably makes it hard to notice) and they all have 1-2 lines except the top one. – user3155083 Oct 11 '19 at 11:03
• I really missed the cases due to non-existant spacing. – Markus Deibel Oct 11 '19 at 11:24
• "Either you comment all methods or none". That's a horrible requirement. Can you name a single large open source project on GitHub that actually does that? All those internal obvious getters and setters? Obvious internal helper methods and one-liners? I mean I've seen code bases where someone had that genius idea which then ended up with wonderfully helpful comments such as /* The Product Id */ public int Productid {get;}. Document the important stuff and the public interfaces, but everything? Pointless. – Voo Oct 12 '19 at 20:07
• I partially agree generally comment everything is not a good rule of thumb but I wanted to make the point that the selection of what was commented seemed random and in a coding assignment for a job interview you should IMHO chose consistency and would be one way to achieve it. I'll clarify the answer. – Markus Deibel Oct 13 '19 at 19:19

Collected thoughts:

Your Helpers class is weird. It mixes e.g. command line input (GetArrayFromSplitInput) with a bad pattern that you don't need, that is, the RunCommandIfTrue method: just use an if statement where you need it instead of using higher-order functions! You just make your code unnecessarily harder to read at the call sites of RunCommandIfTrue.

What's worse, your Helpers class also contains business logic (IsValidPlacement), which is a big no-no. Down that road lie God objects and the death of software architecture. This mixing of responsibilities is actually probably the biggest issue with your code, as it creates the impression that you gave no thought to future-proofing your design. It's the S in SOLID!

There's also basically your entire Main function. You have

bool run = true;
[...]
while (run)
{


but there's no place where run is ever set to false, making the construct useless. Also, this line:

handler.HandleCommand(Helpers.GetArrayFromSplitInput(Console.ReadLine()));


is considerably too busy. Separate state-changing or IO actions should almost always be in separate statements, not be crammed into a single statement like this. This is particularly true here. Your decision to cram it all into one line forces you to keep the entire statement inside a single try-block, even though the exception you are trying to catch can only happen within the GetArrayFromSplitInput method. This can cause your catch-block to catch exceptions that it is not supposed to and then misrepresent the error:

catch (Exception e)
{
Console.WriteLine("Invalid Input: " + e.Message);
}


Any error in your HandleCommand routine will be reported as an invalid input. The same is true if Console.ReadLine is called on a broken pipe. That is (potentially) a bug! Always keep try blocks as small as at all possible.

Speaking of try blocks, one of your methods represents success or failure with a boolean return value. This isn't the 1970's anymore, if your method fails to execute, throw an exception! A reminder here that other programmers might forget to check for the boolean return value when calling your method.

Then there's snippets like this:

//Calls turn with direction as increment of _f
public void Left()
{
Turn(() => (int)_f + 1);
}


Without knowing how the Turn method works internally, I am left guessing what this might be doing. And when looking at that Turn method, it is a lot of complexity for a one-liner:

//If value less than zero default, otherwise calculate direction based on modulo
private void Turn(Func<int> newDirection)
{
_f = (newDirection() < 0) ? Direction.EAST : (Direction)(newDirection() % 4);
}


This is so much mental overload just to clamp a value in the [0, 3] range that it is not worth the DRY here. Rule of thumb: copying a line of code once (here: the clamping logic in the Left and Right functions) is probably okay, it's the third instance that tells you to start looking for a solution. Even then, it's better for readability if the solution doesn't involve random lambdas and higher-order functions!

All in all, the code looks very much like you were trying to be "clever" when writing it. This is a habit that you should work on. Remember the old adage: it is twice as hard to debug code as it is to write code, and by corollary, if you employ your full cleverness to write it, you are unable to debug it! Start writing stupider code, with more emphasis on KISS and clearly delineating the responsibilities of the classes you're writing. Even in a coding interview, the goal should not be to "show off" how well you understand higher-order logic, but to show that you are able to write correct and maintainable code that is, first and foremost, also easy to read for other programmers. That is an important skill when working in a team!

• Hey Colin, great first answer. Let's hope we'll see more of you around – IEatBagels Oct 11 '19 at 18:37
• Thank you for your points on exception handling and placing business logic in the helper, I clearly missed SRP on the class level. RunCommandIfTrue is there to emulate when (from ie Clojure). Turn performs a ternary operation and takes a function - i dont see how this or lambdas and HOF in general is complex and not KISS. If i kept the Turn logic in ie Left(), it would be : _f = ((int)_f+1) < 0) ? Direction.EAST : (Direction)((int)_f+1 % 4); How would you do this part better ? – user3155083 Oct 12 '19 at 2:21
• You shouldn't emulate when from Clojure when not writing Clojure code. C# is not a functional language. Of course, you can "write Perl in any language", and probably also "write LISP in any language", but it is not idiomatic and should be avoided. Write clear imperative code in C# (and clear functional code in Clojure). Your suggestion for the Left code is fine. – Colin Emonds Oct 12 '19 at 15:01
• By the way, even if you do want to write a Turn function for DRYness, you do not need Func<int>, you can just pass in int. Not only does this make your code less needlessly functional, this also fixes a potential bug in your current design: since you evaluate the Func<int> argument twice, if a caller passes in an effectful function or another function which might return two different directions on two separate calls, the return value is undefined. – Colin Emonds Oct 12 '19 at 15:01
• I don't agree that FP style should always be avoided in C#. I view C# as "a multi-paradigm language with an increasingly strong functional component" (this author is more eloquent than me: hackernoon.com/…). A Microsoft MVP blogs about FP in C# weblogs.asp.net/dixin/functional-csharp-fundamentals. – user3155083 Oct 13 '19 at 4:29

## Styles and conventions

• The using should always be at the top of your file.
• Variable names like _f means...? Facing? Face? Force (obviously not, but see where I'm going?)? Don't hesitate to use meaningful variable names. Instead of _f, the variable could be named direction since it's a Direction object.
• The indentation of the switch is real bad. But I'd suspect it's because of how you pasted it here, not how your code looks. If the switch in the IDE looks like this, change it because it makes it impossible to read quickly.

## OOP / Best practices

• If I use a Robot object, I'd expect to be able to see what's its position. What I mean is that _x should be X {public get; private set;}. The same goes for the other properties.
• In the Helpers class, startIndex and endIndex should be constants, not variables.
• You shouldn't catch Exception. If you catch a specific exception, it shows you understand what could go wrong in the code, that's great.
• Your Main "never finishes".
• You sometimes check for null input, sometimes not. You should be consistent (a.k.a always use it, in my opinion) when accessing a public method.
• I don't think it's the CommandHandler's responsibility to decide if the robot can move or not (if the placement wasn't made). The Robot class itself should be able to ignore a movement if it isn't placed (or if it causes it to fall). This would also simplify a lot of your checks.
• The whole Command pattern invites a Factory pattern. The advantage of doing this is that your CommandHandler class doesn't have the responsibility of knowing all the possible commands and the commands can be easily tested/mocked.
• You can also take this "further" and have a Robot class that only maintains the state of the robot and Commands classes where you make the robot move. Once again, this makes testing easier. Like this :
public class Robot
{
public int? X { get; set; }
public int? Y { get; set; }
public Direction? Direction { get; set; }

public bool IsPlaced() => X == null || Y == null || Direction == null;
}

public interface ICommand
{
public void Execute(Robot robot);
public bool Validate(Robot robot);
}

public class MoveCommand : ICommand
{
public void Execute(Robot robot) { /*Here you can make the robot move*/ }
public bool Validate(Robot robot) { /*Here you can validate if the command is valid for a robot*/ }
}

public class CommandFactory
{
public ICommand Create(string command)
{
string[] args = Helpers.GetArrayFromSplitInput(command);

switch (args[0])
{
case "move":
return new MoveCommand();
//...
}
}
}

public class CommandHandler : ICommandHandler
{
private readonly CommandFactory commandFactory;

//I'm skipping the ctor, but I'm sure you get the point

public void Handle(string commandString)
{
ICommand command = commandFactory.Create(commandString);

if (!command.Validate(robot)) return;

command.Execute(robot);
}
}

• I normally put the using statements at the top, but i used StyleCop for this in and it has a rule that says the opposite - SA1200. The case indentation is in the code, also due to a StyleCop edit - however in hindsight I realise I misunderstood the rule that made that warning go away so my fault. I will try to learn from your points, Thank you for the feedback – user3155083 Oct 12 '19 at 1:22
• Is it good style in CommandHandler::Handle to have both the command string and the command object share the variable name command? Is it an application of KISS in the same method to not simply combine the last two lines to if(command.Validate(robot)) command.Execute(robot);? – Lutz Lehmann Oct 12 '19 at 9:58
• "The using should always be at the top of your file." There's several code guides and a style cop rule that disagree with that commandment. I don't see any reason why anybody should care about that as long as it's consistent. – Voo Oct 12 '19 at 20:10
• @LutzL lol, no it's not normal, that's my bad. – IEatBagels Oct 15 '19 at 12:21
• "The Robot class itself should be able to ignore a movement if it isn't placed (or if it causes it to fall)" - What if we wanted to change the behavior so the robot could fall off the table? I don't think this logic belongs in the robot class itself as it could potentially violate open/closed.I think you have sort of addressed this though in a way with your last bullet point. – mbx-mbx Oct 15 '19 at 12:57