Is error checking like this redundant?

[Mr D]

Is using error checking like this bad? I just want to make sure if something goes wrong that I know exactly what it is as I am designing a very large game.

#include "Actor.h"
#include <iostream>
#include <stdexcept>
using std::invalid_argument;
using std::cin;
using std::cout;
using std::endl;
using std::string;

Actor::Actor()
{

}

void Actor::set_coordinates(const int x, const int y)
{
    if ( x >= MAP_BOUNDARY && y >= MAP_BOUNDARY )
    {
        xCoord = x;
        yCoord = y;
    }
    else
    {
        throw invalid_argument( "Attempt to set cartesian points out of bounds." );
    }
}

int Actor::get_x()
{
    return xCoord;
}

int Actor::get_y()
{
    return yCoord;
}

void Actor::move_north()
{
    if ( xCoord > MAP_BOUNDARY )
    {
        xCoord--;
    }
    else
    {
        throw invalid_argument( "Attempt to move North, out of bounds." );
    }
}

void Actor::move_east()
{
    if ( yCoord < MAP_SIZE )
    {
        yCoord++;
    }
    else
    {
        throw invalid_argument( "Attempt to move East, out of bounds." );
    }
}

void Actor::move_south()
{
    if ( xCoord < MAP_BOUNDARY )
    {
        xCoord++;
    }
    else
    {
        throw invalid_argument( "Attempt to move South, out of bounds." );
    }
}

void Actor::move_west()
{
    if ( yCoord > MAP_BOUNDARY )
    {
        yCoord--;
    }
    else
    {
        throw invalid_argument( "Attempt to move West, out of bounds." );
    }
}

[booradley60]

What happens when xCoord == MAP_BOUNDARY? You can't move north or south anymore. Same with yCoord and east-west movement. (Intuitively, I would have thought x should represent east-west position and y would represent north-south position...)

Can you explain more what MAP_BOUNDARY respresents? Does it mean all points fall on one side of the boundary? If so, you would only have exception cases for two of your four types of directional moves. If MAP_BOUNDARY represents the southern and western edge (like, say, quadrant I of a cartesian plane) then you would only have to check the boundary condition when moving either south or west.

[AbstractionAnon]

Without knowing where 0,0 is in your map system, it's hard to tell if your logic is what you intended. In most simple games, a world consts of [n][n] tiles. If that's the case in your game, then your logic seems reversed.

Line 52: Don't you mean MAP_BOUNDARY?

As far as your move functions, I would tend to think of those as bool functions. Returns true, if the move was made in that direction, returns false, if you can't move in that direction. Throwing an exception simply because you reached the edge of the map is generally overkill.

In set_coordinates, I would agree that throwing an exception is appropriate. You should be checking that the coordinates are valid before calling it.

As a general point of style, I don't like to see exceptions thrown in else clauses. I prefer to see the exception thrown in the then portion of the if statement. It makes for cleaner code.

void Actor::set_coordinates(const int x, const int y)
{  if ( x < MAP_BOUNDARY || y < MAP_BOUNDARY )
        throw invalid_argument( "Attempt to set cartesian points out of bounds." );
    // error conditions checked.  Continue inline
    xCoord = x;
    yCoord = y;
}

Note: The above example assumes the existing boundary checking logic you're using, although I'm not sure it's what you meant.

[JLBorges]

> I just want to make sure if something goes wrong that I know exactly what it is

During the development phase, asserting invariants would be more convenient.
Writing a shim class would eliminate a lot of boiler-plate code.

Something along these lines, perhaps:

#include <iostream>
#include <stdexcept>
#include <cassert>

template < typename T > struct bounded_range
{

    constexpr bounded_range( const T& lb, const T& ub, const T& v = T() )
        : lbound(lb), ubound(ub), value( checked(v) ) {}

    constexpr operator const T& () const { return value ; }

    bounded_range<T>& operator= ( const T& v ) { value = checked(v) ; return *this ; }

    // operator ++ etc.

    bounded_range<T>& reset( const T& lb, const T& ub )
    { lbound = lb ; ubound = ub ; checked(value) ; return *this ; }

    bounded_range<T>& reset( const T& lb, const T& ub, const T& v )
    { lbound = lb ; ubound = ub ; value = checked(v) ; return *this ; }

    private:
        T lbound ;
        T ubound ;
        T value ;

        constexpr const T& checked( const T& v )
        { return v < lbound || v >= ubound ? ( assert(false), throw std::out_of_range("out of range"), v ) : v ; }
};

using brint = bounded_range<int> ;

int main()
{
    try
    {
        brint i( 10, 20, 15 ) ;
        std::cout << i << '\n' ;
        i = i + 3 ;
        std::cout << i << '\n' ;
        i = i + 2 ;
        std::cout << i << '\n' ;
    }
    catch( const std::out_of_range& ) { std::cerr << "out of range\n" ; }

    try { brint j( 10, 20, 25 ) ; std::cout << j << '\n' ; }
    catch( const std::out_of_range& ) { std::cerr << "out of range\n" ; }
}

http://coliru.stacked-crooked.com/a/fcdd55d70d2878fc

For some more ideas, see: http://rk.hekko.pl/constrained_value/

[Mr D]

Map size is 0. Map boundary is 10. The map is two dimensional. From my understanding of arrays 0 0 is the top left element, bottom right is 9 9. Also the first element decides row, my x, and y decides column. Am i correct it all of this?

[AbstractionAnon]

Map size is 0

In that case, line 52 makes no sense. You're testing yCoord < 0, then increment yCoord. yCoord should never be negative. If yCoord is positive (normal), then you raise an exception.

Your other boundary checks seem equally flawed. Example: move_north()
If xCoord > 10, then decrement xCoord. xCoord should never be > 10. If it is, that's when you want to raise an exception. If xCoord is < 10, then you raise an exception. That's not what you want.

As I mentioned earlier, I would make your move functions bool functions.

bool Actor::move_north()
{  if (xCoord == 0)
    {  cout << "At edge of map.  Can't move North" << endl;
        return false;   // Move not done
    }
    xCoord--;
    return true;    //  Move completed 
}