this post was submitted on 04 Jul 2023
15 points (100.0% liked)
Programming
13385 readers
1 users here now
All things programming and coding related. Subcommunity of Technology.
This community's icon was made by Aaron Schneider, under the CC-BY-NC-SA 4.0 license.
founded 2 years ago
MODERATORS
you are viewing a single comment's thread
view the rest of the comments
view the rest of the comments
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 tohorse_matrix
). I still have some general recommendations for you how you could improve your code:std::random_device
/std::mt19937
)rand() % 26
and check against 0, in that case you reroll - you could instead directly calculaterand() % 25 + 1
rand()
approach, you could look intostd::shuffle
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 wellchar 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'
)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.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..
After a few hours of work, I was able to improve the code by following your recommendations. I was genuinely surprised at how removing boilerplate code that I created at the beginning helped me see where the problems are and allowed me to see better solutions to problems.
For example: instead of copying each value into a temporary
int
array, sorting that array and then changing the original, I was able to just use C++ STL sort() with a function parameter to automatically sort structs based on their values when it came to the rows.I touched on every point that you've made and made changes accordingly, some things like the
char option
I just removed and checked for the conditions in the function instead of waiting for the programmer to give the correct answer. The only thing left for me is to relax a bit since I was at it for a lot of time, and then properly comment the code.