Robot game in C++

Question

I have a very basic "game" where a robot is placed on a small 5x5 board, and commands are given to it. To start, the robot must be placed on the board, then you may command it to move, or rotate left and right. I have tried to use object oriented concepts, but it was not a requirement for my task. I think I have done an okay job, and I would love to know what I have done incorrectly.

Here are a few "requirements" given to me:

1. The "place" command takes an x, y position, and a direction or NORTH, EAST, SOUTH, or WEST.
2. No other commands may be run unless the robot has been placed
3. Move will move the robot forward by 1, in the direction it is facing.
4. LEFT and RIGHT will rotate the robot in their respective directions
5. REPORT will print the current position, and facing direction of the robot
6. Place is a valid command, even after it has already been placed
7. Inputs that will cause the robot to fall off the board are to be rejected or ignored
8. Invalid or "junk" input is to be ignored
9. The robot will take commands as plaintext, the robot must take this text and run respective instructions given that they are valid

Example input/output:

PLACE 1,3,WEST
MOVE
REPORT
Output: 0,3,WEST

and here is my robot.cpp

#include "robot.h"
#include "tabletop.h"
#include "helpers.h"
namespace ToyRobot
{
 //used as an interface to supply instructions as text, robot will perform
 bool Robot::command(std::string instruction)
 {
 std::istringstream iss(instruction);
 std::vector<std::string> tokens{ std::istream_iterator<std::string>{iss}, std::istream_iterator<std::string>{} };
 if (this->commandList.count(tokens[0]) > 0) //is the first token, a valid command?
 {
 //command is valid
 if (tokens[0] == "PLACE")
 { //check if there are valid arguments
 if (tokens.size() < 2)
 {
 std::cout << "Error! Not enough arguments for 'PLACE'\n";
 return false;
 }
 else
 {
 try
 {
 uint8_t arg1 = std::stoi(split(tokens[1], ",")[0]);
 uint8_t arg2 = std::stoi(split(tokens[1], ",")[1]);
 std::string arg3 = split(tokens[1], ",")[2];
 this->place(arg1, arg2, arg3);
 }
 catch (...)
 {
 return false;
 }
 return true;
 }
 }
 else if (tokens[0] == "MOVE")
 {
 this->move();
 }
 else if (tokens[0] == "LEFT" || tokens[0] == "RIGHT")
 {
 this->rotate(tokens[0]);
 }
 else if (tokens[0] == "REPORT")
 {
 this->printStatus();
 }
 return true;
 }
 else
 return false;
 }
 //checks if a given position is valid (used only by other methods)
 bool Robot::isValidPosition(uint8_t x, uint8_t y)
 {
 if (x < 0 || x > TABLETOP_MAX_X || y < 0 || y > TABLETOP_MAX_Y)
 return false;
 else
 return true;
 }
 //places robot, ignores invalid positions
 bool Robot::place(uint8_t x_place_pos, uint8_t y_place_pos, std::string facingDirection)
 {
 if (x_place_pos < 0 || x_place_pos > TABLETOP_MAX_X || y_place_pos < 0 || y_place_pos > TABLETOP_MAX_Y)
 return false;
 if (this->facingDirections.count(facingDirection) == 0) //check if given facing direction was valid
 return false;
 this->x_pos = x_place_pos;
 this->y_pos = y_place_pos;
 this->facingDirection = this->facingDirections[facingDirection];
 this->placed = true;
 return true;
 }
 //moves robot forward by one, ignored invalid movements
 bool Robot::move()
 {
 if (this->placed)
 {
 uint8_t sim_x = this->x_pos;
 uint8_t sim_y = this->y_pos;
 //simulate movement
 if (facingDirection == 0)
 sim_y += 1;
 else if (facingDirection == 1)
 sim_x += 1;
 else if (facingDirection == 2)
 sim_y -= 1;
 else if (facingDirection == 3)
 sim_x -= 1;
 if (isValidPosition(sim_x, sim_y))//if it was valid, set and return true
 {
 this->x_pos = sim_x;
 this->y_pos = sim_y;
 return true;
 }
 else //invalid move (would be out of bounds)
 return false;
 }
 else //not placed
 return false;
 }
 //rotates robot given a direction string
 bool Robot::rotate(std::string direction)
 {
 if (this->placed)
 {
 uint8_t sim_direction = this->facingDirection;
 if (direction == "LEFT")
 sim_direction = (sim_direction + 3) % 4; //rotate left
 else if (direction == "RIGHT")
 sim_direction = (sim_direction + 1) % 4; //rotate right
 else
 return false; //invalid input
 this->facingDirection = sim_direction;
 return true;
 }
 else //not placed
 return false;
 }
 void Robot::printStatus()
 {
 if (this->placed)
 std::cout << int(this->x_pos) << ',' << int(this->y_pos) << ',' << (this->reversedDirections[this->facingDirection]) << "\n";
 else
 std::cout << "Robot is not yet placed on the tabletop!\n";
 }
}

robot.h

#pragma once
#include "stdafx.h"
namespace ToyRobot
{
 class Robot
 {
 private:
 bool placed = false;
 uint8_t x_pos = NULL;
 uint8_t y_pos = NULL;
 uint8_t facingDirection = NULL;
 const std::unordered_set<std::string> commandList = { "PLACE","MOVE","LEFT","RIGHT","REPORT" };
 std::unordered_map <std::string, int> facingDirections
 = { {"NORTH", 0}, {"EAST", 1},
 {"SOUTH", 2}, {"WEST", 3} };
 std::unordered_map <int, std::string> reversedDirections
 = { {0, "NORTH"}, {1, "EAST"},
 {2, "SOUTH"}, {3, "WEST"} };
 bool isValidPosition(uint8_t, uint8_t);
 public:
 Robot() //constructor
 {
 }
 bool command(std::string);
 bool place(uint8_t, uint8_t, std::string);
 bool move();
 bool rotate(std::string);
 void printStatus();
 };
}

helpers.cpp

#include "stdafx.h"
#include "helpers.h"
//python's "split" function, implemented in C++. returns a vector of split std::strings by a specified delimiter
std::vector<std::string> split(const std::string& in, const std::string& delim)
{
 using std::string;
 using std::vector;
 string::size_type start = in.find_first_not_of(delim), end = 0;
 vector<string> out;
 while (start != in.npos)
 {
 end = in.find_first_of(delim, start);
 if (end == in.npos)
 {
 out.push_back(in.substr(start));
 break;
 }
 else
 {
 out.push_back(in.substr(start, end - start));
 }
 start = in.find_first_not_of(delim, end);
 }
 return out;
}

helpers.h

#pragma once
#include "stdafx.h"
std::vector<std::string> split(const std::string& in, const std::string& delim);

tabletop.h (there is no tabletop.cpp, as there is no need for it)

#pragma once
#include "stdafx.h"
constexpr auto TABLETOP_MAX_X = 4;
constexpr auto TABLETOP_MAX_Y = 4;
//0,0 is south west corner https://i.imgur.com/pm2XVHx.png
//Tabletop is never used, but it is here if required
class Tabletop
{
private:
 const uint8_t x_len = TABLETOP_MAX_X;
 const uint8_t y_len = TABLETOP_MAX_Y;
public:
};

and finally my stdafx.h

#pragma once
#include <iostream>
#include <string>
#include <unordered_map>
#include <unordered_set>
#include <sstream>
#include <algorithm>
#include <iterator>

My robot header and implementation is in a namespace, as it is to be compiled into a library.

How is my project structure? Thanks

Answer 1

I see a number of things that may help you improve your program.

Use the required #includes

The code uses std::vector and std::string which means that it should #include <vector> and <string>. It was not difficult to infer, but it helps reviewers if the code is complete.

Use 0 instead of NULL for values that are not pointers

The value 0 is a numeric quantity zero, but the value NULL is an implementation-defined null-pointer constant. It is not guaranteed to have the value 0. For that reason, non-pointer values should be initialized to 0 and not NULL.

Think carefully about signed vs. unsigned

The code currently contains this function:

bool Robot::isValidPosition(uint8_t x, uint8_t y)
{
 if (x < 0 || x > TABLETOP_MAX_X || y < 0 || y > TABLETOP_MAX_Y)
 return false;
 else
 return true;
}

If x is an unsigned quantity, it will not be possible that x < 0. For that reason, the statement can be shortened to this:

if (x > TABLETOP_MAX_X || y > TABLETOP_MAX_Y)

However, there's really no need for an explicit if statement here. Instead, just return the appropriate boolean value:

return x <= TABLETOP_MAX_X && y <= TABLETOP_MAX_Y;

Don't write this->

Within member functions this->data is redundant. It add visual clutter and does not usually aid in understanding. So for example, we have the existing place function:

bool Robot::place(uint8_t x_place_pos, uint8_t y_place_pos, std::string facingDirection)
{
 if (x_place_pos < 0 || x_place_pos > TABLETOP_MAX_X || y_place_pos < 0 || y_place_pos > TABLETOP_MAX_Y)
 return false;
 if (this->facingDirections.count(facingDirection) == 0) //check if given facing direction was valid
 return false;
 this->x_pos = x_place_pos;
 this->y_pos = y_place_pos;
 this->facingDirection = this->facingDirections[facingDirection];
 this->placed = true;
 return true;

}

I'd write it like this instead:

bool Robot::place(uint8_t x_place_pos, uint8_t y_place_pos, std::string dir)
{
 if (isValidPosition(x_place_pos, y_place_pos) && facingDirections.count(dir)) {
 x_pos = x_place_pos;
 y_pos = y_place_pos;
 facingDirection = facingDirections[dir];
 placed = true;
 } else {
 placed = false;
 }
 return placed;
}

Note that I've used the previously mentioned isValidPosition() function and also explicitly set member variable placed to false if the PLACE command has failed. This is different behavior from the original code, so you'll have to decide which you like better.

Use const where practical

The printStatus and member function of Robot does not alter the underlying object and therefore should be declared const. This will require also changing from reversedDirections[facingDirection] to reversedDirections.at(facingDirection).

Sanitize user input better

The code does not really do a very good job sanitizing user input. For example, if the user enters the command "PLACE 2, 3, WEST", the program crashes because it expects that there will be no spaces except after the word "PLACE". That's not very robust.

Think of the user

When the user enters a command such as "PLACE 2,3" omitting the direction, the program just says "Error! Not enough arguments for 'PLACE'" which is technically true, but not very informative to the user. The message could, for example, show a valid sample command instead.

Don't use exceptions for non-exceptional events

Exceptions are for exceptional events and should be used for error handling only. Having the user input incorrect command arguments is not exceptional and should be handled as part of the normal program flow and not with an exception.

Reconsider the class design

The Robot class has multiple unordered_map structures to deal with compass directions. First, if they exist at all, they should probably be static const since all Robot instances would use the same directions. An alternative would be to have a Direction class that would handle the translations to and from text to uint8_t.

Don't write an empty constructor

The Robot class uses in-class initializers which is good practice, but that means that the default constructor will be automatically generated, so you could either omit it entirely (which I'd prefer) or write Robot() = default; if you want to make it even more explicitly clear.

Pass const string reference

The third argument to place ought to pass the string by reference to avoid creation of a temporary and can be const. This is shown in the following suggestion.

Name parameters in function prototypes

The function prototype for place currently looks like this:

bool place(uint8_t, uint8_t, std::string);

However, it would be better to name the parameters so that it would give a reader of the code a better idea of how to use the function. I'd change it to this:

bool place(uint8_t x, uint8_t y, const std::string& dir);

Check return values

The place(), move(), etc. commands all return a bool to indicate success, but the program never uses those values. I'd suggest using the return value to give the human controlling the Robot some feedback about what it is or is not doing.

Eliminate "magic values"

The values of WEST, NORTH, RIGHT etc. are sprinkled through the code, but they really ought to be named constants instead, and specifically a named constant static member. If you have a C++17 compliant compiler, std::string_view would be just the thing to use for these.

Use include guards

While many compilers support the use of #pragma once it is not standard and therefore not portable. Use an include guard instead. See SF.8.

Make header files self-contained

The helpers.h file contains stdafx.h but that's not very easily maintained because if the contents of stdafx.h are changed, it implicitly changes the effect of this file as well. Instead, you could be explicit about what's actually needed for the interface. I would write that file like this:

#ifndef SPLIT_H
#define SPLIT_H
#include <vector>
#include <string>
std::vector<std::string> split(const std::string& in, const std::string& delim);
#endif // SPLIT_H

I would also name it split.h to be much more suggestive as to the contents. See SF.11.

Eliminate unused class

Because, as you correctly note, the Tabletop class is not used, it should be removed from the project.

• Cherona: Thank you so much for your review, but what do you mean "The values of 10000 and 9999 are sprinkled through the code", I don't think I have them anywhere.
• Edward: Oops! That was a cut-and-paste error from another review. I've fixed it now.
• Cherona: Is there a data structure in C++ that I can merge these two into one? Like I wish to access both by the "key" and get the other respective value cdn.discordapp.com/attachments/448454862880112651/…
• Edward: Since there are only four data points, I'd put them into a [std::array] hidden inside a Direction class, private within the Robot class. Going from number to string is just an array access. Going the other direction could simply use a linear search.
• 1201ProgramAlarm: You forgot to mention that #include "stdafx.h" should not be in a .h file, since it is used to precompile headers, but should be in all the .cpp files.