I saw that problem a few weeks ago on a youtube thumbnail, it's quite a nice problem and I think it gets even more interesting if you start to consider 5^3 or even 5^N horses. Cool that you actually coded a solution for it! I like the idea of hiding the actual values to prevent the developer from cheating :)
I haven't really checked whether you program does exactly what I'd expect it to do since I had a hard time to understand what's happening.
I think a few comments here and there would help to understand what exactly is going on and maybe some better names would hep, too (example: number_matrix
- what does it do? I see from the type that it is a 2d array of ints but what does it contain and how does it relate to horse_matrix
).
I still have some general recommendations for you how you could improve your code:
- You seem to use OS specific random numbers, there is a C++ api for that nowadays (
std::random_device
/std::mt19937
) - You call
rand() % 26
and check against 0, in that case you reroll - you could instead directly calculaterand() % 25 + 1
- As alternative to that manual
rand()
approach, you could look intostd::shuffle
- Take a look at sets (e.g.
std::unordered_set
), they have acontaints()
function that tells you whether a value is in the set or not. It's not a big issue here but using vectors withstd::count
when all you want to do is check whether an element is in a set will not scale well - Consider using an enum instead of the
char option
. I'd go with something likeenum class Option { kRow, kColumn };
. If you want to stay with a char, I'd recommend not using a preprocessor directive for this (#define
) but instead go with a regular constant (e.g.constexpr char kRow = 'r'
) - Consider using
std::unique_ptr<MatrixPosition>
instead of returning a raw pointer - that way you avoid memory leaks. ignore_count
andignore_vector
are only used in one function, so they don't have to be members of the class.InitializeMatrix
probably does not need to be a class in the first place.- Consider not using
using namespace std
. For small projects that's not an issue for bigger ones you'll find your namespace polluted.
Something that you might also not be aware of is that std::endl
automatically flushes buffers and makes sure that your line is printed immediately. If you don't need this you can just use \n
instead which is a little bit faster. But that's more of a "fun fact" than a general recommendation.
Hope that helps and doesn't sound too harsh, I know how code reviews can feel sometimes..