C++ Removing vector elements with find_first_not_of(char*)

Question

I'm making a chess game and trying to add valid positions to a vector. The specific piece I'm working on is the knight, and some of the positions I've hardcoded are off the board, depending on the knight's position on the board. My coordinate system uses A-H for rows and 0-8 for columns.

I have defined a character array of valid characters (A-H and 0-8) and I'm using find_first_not_of to identify, and remove coordinate pairs that are invalid. For example:

Valid: A1

Invalid: ?3 - remove this

Question: Why is it that my function removes some coordinate pairs that are invalid and don't fit the pattern, but not others? For example, using position A2 as input, @4 and @0 are successfully removed. However the remaining available positions are C3, C1, B4, ?3, B0, ?1.

If the input is H2, then F3, F1, G0, G4 are available positions and J3, J1, I4, I0 are successfully removed, giving the desired result of only valid positions.

Problem: My output is correct some of the time and incorrect other times given the same method of removing invalid positions.

Parent class Piece.cpp:

char Piece::valids[16] = {
    'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H',
    '0', '1', '2', '3', '4', '5', '6', '7'
 };

void Piece::removeInvalids(vector<string>& v)
{
    for (short i = 0; i < v.size(); i++)
    {
        string s = v.at(i);
        size_t found = s.find_first_not_of(valids);
        if (found != string::npos)
        {
            cout << v.at(i) << endl;
            swap(v.at(i), v.back());
            v.pop_back();
        }
    }
}

Child class Knight.h:

vector<string> getAvailPositions(Piece **all)
{
    vector<string> v;
    stringstream ss;
    ss << static_cast<char>(position[0] + 2)
            << static_cast<char>(position[1] + 1);
    v.push_back(ss.str());
    stringstream ss2;
    ss2 << static_cast<char>(position[0] + 2)
            << static_cast<char>(position[1] - 1);
    v.push_back(ss2.str());
    stringstream ss3;
    ss3 << static_cast<char>(position[0] + 1)
            << static_cast<char>(position[1] + 2);
    v.push_back(ss3.str());
    stringstream ss4;
    ss4 << static_cast<char>(position[0] - 1)
            << static_cast<char>(position[1] + 2);
    v.push_back(ss4.str());
    stringstream ss5;
    ss5 << static_cast<char>(position[0] + 1)
            << static_cast<char>(position[1] - 2);
    v.push_back(ss5.str());
    stringstream ss6;
    ss6 << static_cast<char>(position[0] - 1)
            << static_cast<char>(position[1] - 2);
    v.push_back(ss6.str());
    stringstream ss7;
    ss7 << static_cast<char>(position[0] - 2)
            << static_cast<char>(position[1] - 1);
    v.push_back(ss7.str());
    stringstream ss8;
    ss8 << static_cast<char>(position[0] - 2)
            << static_cast<char>(position[1] + 1);
    v.push_back(ss8.str());
    removeInvalids(v);
    return v;
}

Please let me know if any changes to this post should be made for you to better assist me, thank you.

Answer 1

You should rethink your design--it's complicated and inefficient. How about this:

typedef std::array<char, 2> Position;

vector<Position> Knight::getAvailPositions() const
{
    vector<Position> v;
    v.reserve(8);
    for (char a : {2, -2}) {
        for (char b : {1, -1}) {
            v.emplace_back(position[0] + a, position[1] + b);
            v.emplace_back(position[0] + b, position[1] + a);
        }
    }
    removeInvalids(v);
    return v;
}

bool invalid(Position p)
{
    return p[0] < 'A' || p[0] > 'H' || p[1] < '0' || p[1] > '7';
}

void Piece::removeInvalids(vector<Position>& v)
{
    v.erase(std::remove_if(v.begin(), v.end(), invalid), v.end());
}

This avoids an absolute ton of dynamic memory allocations that were hiding inside all those strings & stringstreams. Now you have just one per getAvailPositions() call. Plus it's less code.