Skip to main content
corrected review to talk about std::string_view
Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

The values of 10000WEST and, 9999NORTH, RIGHT etc. are sprinkled through the code, but they really ought to be a named constantconstants 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.

The values of 10000 and 9999 are sprinkled through the code, but they really ought to be a named constant instead, and specifically a named constant static member.

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.

Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

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 10000 and 9999 are sprinkled through the code, but they really ought to be a named constant instead, and specifically a named constant static member.

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.