2
\$\begingroup\$

To find neighbors of a vector and return array of Vectors which are neighbors of a Vector:

vector<XY> neighboursOf(XY coord){
    short x = coord.x;
    short y = coord.y;

    vector<XY> neighbours;
    neighbours.reserve(9);

    neighbours.push_back(XY(x-1, y));
    neighbours.push_back(XY(x-1, y-1));
    neighbours.push_back(XY(x-1, y+1));

    neighbours.push_back(XY(x+1, y));
    neighbours.push_back(XY(x+1, y-1));
    neighbours.push_back(XY(x+1, y+1));
    neighbours.push_back(XY(x+1, y-1));

    neighbours.push_back(XY(x, y-1));
    neighbours.push_back(XY(x, y+1));

    return neighbours;
}
}

XY is a class which has members x and y:

class XY{
    public:
    XY(short x=0, short y=0);
    short x;
    short y;
};

XY::XY(short x, short y) {
    this->x = x;
    this->y = y;
}

The param is Vector of the yellow square, the neighbor array is supposed to be the black squares

Example

\$\endgroup\$
3
  • 5
    \$\begingroup\$ A bit more context would be helpful to reviewers. First, it would be helpful to see the XY class or struct. Second, I don't believe that XY[] is a valid return type. It would look to the compiler like a structured binding. \$\endgroup\$
    – Edward
    Mar 31 at 11:45
  • 2
    \$\begingroup\$ There may be a recursive way, but here we review the code you have written, not the code you may write in the future. If you have code that works (to the best of your knowledge), then post it for review (as Edward says, we'll want more context), and perhaps someone will point out if it's inefficient (personally, I'd leave it as is, or perhaps write a short loop). \$\endgroup\$ Mar 31 at 12:07
  • 1
    \$\begingroup\$ Incorporating advice from an answer into the question violates the question-and-answer nature of this site. You could post improved code as a new question, as an answer, or as a link to an external site - as described in I improved my code based on the reviews. What next?. I have rolled back the edit, so the answers make sense again. \$\endgroup\$ Apr 1 at 12:51
6
\$\begingroup\$

The XY class looks like it would be better as a plain struct:

struct XY
{
    short x;
    short y;
};

Then we don't need to declare a constructor, because it can be aggregate-initialised.

I'll assume that your vector is an alias for std::vector here, since there's no definition visible.

Eight of the statements to populate the vector look reasonable. The duplicate of x+1, y-1 is probably unintended, though.

You might profitably use emplace_back rather than writing those constructors. We could create the vector directly, with all the element values as its initializer-list:

std::vector<XY> neighboursOf(XY coord)
{
    auto const x = coord.x;
    auto const y = coord.y;

    return { {x-1, y-1}, {x, y-1}, {x+1, y-1},
             {x-1, y  },           {x+1, y  },
             {x-1, y+1}, {x, y+1}, {x+1, y+1} };
}

See how the formatting here helps us see that each neighbour is included exactly once?

It is possible to write a clever loop, but that's going to be harder to read and no more efficient than the straightforward code.

Note that if we have a finite grid, then we might want special behaviour at the edges. And if we have an infinite grid, we'll suffer arithmetic overflow (Undefined Behaviour) when we reach the limits of short.

\$\endgroup\$
5
\$\begingroup\$

I suggest initializing the vector on creation since you know all the needed elements at compile time. Also, if you are not changing the size of that vector, it is probably better to use a std::array instead of std::vector. Keep in mind, that most x64 C++ compilers by default use QWORD-aligned (8-byte) pointers, so it is better to align your variables with the pointer size. There is a ptrdiff_t type in <cstddef> library created for this purpose. Check the following code snippet:

#include <array>
#include <cstddef>

class XY
{
public:
    ptrdiff_t x;
    ptrdiff_t y;
public:
    XY(const ptrdiff_t& x=0, const ptrdiff_t& y=0): x(x), y(y) {}
};

std::array<XY, 8> neighboursOf(const XY& coord)
{
    const auto x = coord.x;
    const auto y = coord.y;

    return {
            XY{x-1, y}, XY{x-1, y-1},
            XY{x-1, y+1}, XY{x+1, y},
            XY{x+1, y+1}, XY{x+1, y-1},
            XY{x, y-1}, XY{x, y+1}
        };
}

You can check the compiler output on godbolt and compare it with your code.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ We managed to write very similar answers within seconds of each other! \$\endgroup\$ Mar 31 at 17:23
  • \$\begingroup\$ Yeah, I just have been checking the assembler output before posting the answer. \$\endgroup\$ Mar 31 at 17:26
  • \$\begingroup\$ The duplicate XY(x+1, y-1) was unintended btw, so you can edit that out \$\endgroup\$
    – mTvare
    Apr 1 at 4:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.