Based on the author's solution, there seems to be an implicit assumption that whatever the size of the outer array, is also the size of each of the inner arrays. If this is the case, it needs to be explictized (and will probably invalidate some answers). All the current tests hardcode a length of 3, and the random tests of the Python translation hardcode the inner array size as 3. The description should be cleaned up to sort this out, as it makes adapting this Kata awkward into languages that support tuples or compile-time sized arrays because each person who adapts this Kata seems to make a different set of assumptions.

Oh neat I assumed std::multiplies<void> only had one template parameter like the regular version does. Cool

Yup, the updates do not change any part of the functionality at all. The changes just update the assert equal syntax in the fixed tests. The part of the random tests with the customized assertion messages was not touched whatsoever since I was already using the "newer" syntax because the older syntax doesn't allow you to customize messages

• n should be std::size_t, not int. This change would remove the need for the odd cast in the if. Variables throughout your code that are supposed to represent an index or a size should be std::size_t
• Your reference solution is exposed in the global scope. Please move it to the private section of the Describe struct, and keep the existing code in a public section. Here is an example
• All the headers are missing. You need to #include: <vector>, <cstddef>, <random> (more on that below)
• Error feedback is missing. With each assertion, you should provide an ExtraMessage. Many examples on how you can do so in this example suite
• Do not use the C-style random generation utilities. Use the utilities from C++11's <random> instead. Link to the relevant docs
• For appropriate semantics, accept the vector by const std::vector<int>& instead of by value
• The default setup should compile, therefore add a single return {}; to the body of the solution setup
• Remove the unneeded default Codewars TODO comment on top of each snippet. It's bound to confuse some solvers
• Description mentions a reference but doesn't include it. Either that line should be hidden for cpp, or you should pick a method to showcase. The issue is that there is no canonical way to slice a vector in c++. You can kinda do it in like 9 different ways.

Since this Kata has been/will be adapted to languages where going out of bounds throws or invokes undefined behavior, the description should explicitly note that n can be larger than the array's size. It's 8kyu, so I think we should give the solvers some grace and have them pre-emptively handle edge cases instead of flying into exceptions or crashes

.

approved by someone

Fork published

Fork of eurydice5717's translation that resolves this issue caused by overflow. Each call to generate a random value now constructs its own int distribution, removing the arithmetic and guaranteeing that the numbers are in range.

Approved

LGTM! Approved. One minor quirk is that operations like (disb(gen)%(extra/2))+1 return a lower bound that is one-less than the JS version (ex: 1 here instead of 2). It infinitesimally increases the likelihood of integer divisions, but that seems harmless to me (since the finite division section in JS also generates integer divisions every now and then). If another reviewer finds this a deal breaker, it can be forked

Very faithful translation, but I would like to point out one issue and offer 3 suggestions:

1. Issue: referenceSolution being a free public function means it is visible to the solver, who can invoke your function to pass the tests (after a simple forward declaration). Consider moving it into the private section of the Describe struct
2. I think it'd be beneficial change the body of the test function to this so the solvers receive error feedback:

   Assert::That(fractionToPeriodic(n,d), Equals(s), ExtraMessage(fmt::format("Incorrect result for fractionToPeriodic({}, {}):", n, d)));
   

3. Some header inclusions are missing (<cmath>, <utility>)
4. The entire code with stringstream and substr-ing out the zeroes can be replaced with a clearer one-liner

   const auto stringified = fmt::format("{}", static_cast<double>(n) / d);
   

   The library handles the zeroes for you, and returns a result identical to what you've hand-rolled

Everything else LGTM
(#include <fmt/core.h> assumed for #2 and #4)

C# Translation

Gripes with the description that I feel should be addressed:

• Should the existing list(s) be mutated or should a new list be created and returned?
• Description could be made more language agnostic.