Unused header
We use nothing from <cmath>
, so it need not be included.
This should be a function, not a class
Putting all code into a class suggests you have a background in Java or similar.
In C++, we can (and should) use ordinary functions for operations that are mathematically functions. In this case, we have a pure function: it has no state, and should always give the same result for any given input.
If it's part of your program requirements that it must provide this (unhelpfully-named) class, then comment that. I'd recommend that you still write a plain function, and simply provide an adapter to conform to the requirements:
int reverse_decimal_digits(int i);
class Solution
{
public:
int reverse(int i) { return reverse_decimal_digits(i); }
}
Choice of data type
If we're processing a 32-bit integer, then we should be using std::int32_t
. Plain int
isn't necessarily large enough (it can be any size from 16 bits upwards).
Incorrect test
Given int i
, then i > INT_MAX || i < INT_MIN
is false by definition. The requirement you quote is (my emphasis):
When the reversed integer overflows, return 0.
Not all integers have a negative
Beware of overflow here:
if(i < 0) {
sign = -1;
i = i*sign;
}
On 2s-complement systems, -1 * INT_MIN
is undefined.
It turns out that we don't need this step, as in modern C++, the %
operator can be used predictably with negative numbers to our advantage (see my modified code, below).
Don't do I/O from a pure function
I guess this is some leftover debugging that should have been removed:
std::cout << reversed << '\n';
Additional tests
It's good that you've included some unit tests - I wish more people would do that!
Do think about which values to test. Your choice agrees with mine somewhat, but diverges later:
0
, 1
and -1
for the three simplest cases.
- positive and negative two-digit numbers (e.g.
12
and -23
).
- smallest and largest allowable input (
INT32_MIN
and INT32_MAX
).
- smallest and largest allowable result, and the first overflow in each direction in first and last digits (±
1463847412
, ±1463847413
, ±1563847412
).
Don't be tempted to over-test. Tests need to be maintained, too, so try to limit the tests to those that exercise the limits within the implementation.
Minor improvements
The scope of pop
can be reduced to within the loop. And perhaps a better name would be digit
?
noexcept
and constexpr
Can we annotate the function with noexcept
and constexpr
?
Future
Should the number base be hard-coded to 10? Perhaps there's a use for a reverser that works in arbitrary bases. Certainly, base-16 is convenient for testing.
Modified code
I've used GoogleTest rather than plain C assert()
, so as to get better messages when a test fails, but any testing method is fine.
#include <cstdint>
constexpr std::int32_t
reverse_digits(std::int32_t i, int base = 10) noexcept
{
std::int32_t reversed = 0;
const bool negative = i < 0;
while (negative ? i <= -base : i >= base) {
auto const digit = i % base; // negative if i < 0
reversed = reversed * base + digit;
i /= base;
}
// final digit may cause overflow
const bool overflow =
negative
? (reversed < (INT32_MIN - i) / base)
: (reversed > (INT32_MAX - i) / base);
if (overflow) {
return 0;
}
return reversed * base + i;
}
#include <gtest/gtest.h>
TEST(Reverse, decimal)
{
EXPECT_EQ(0, reverse_digits(0));
EXPECT_EQ(1, reverse_digits(1));
EXPECT_EQ(-1, reverse_digits(-1));
EXPECT_EQ(21, reverse_digits(12));
EXPECT_EQ(-32, reverse_digits(-23));
EXPECT_EQ(0, reverse_digits(INT32_MIN));
EXPECT_EQ(0, reverse_digits(INT32_MAX));
EXPECT_EQ(2147483641, reverse_digits(1463847412));
EXPECT_EQ(0, reverse_digits(1463847413));
EXPECT_EQ(0, reverse_digits(1563847412));
EXPECT_EQ(-2147483641, reverse_digits(-1463847412));
EXPECT_EQ(0, reverse_digits(-1463847413));
EXPECT_EQ(0, reverse_digits(-1563847412));
}
TEST(Reverse, hexadecimal)
{
EXPECT_EQ(0x7ffffff7, reverse_digits(0x7ffffff7, 16));
EXPECT_EQ(0, reverse_digits(0x10000008, 16));
EXPECT_EQ(-0x7ffffff7, reverse_digits(-0x7ffffff7, 16));
EXPECT_EQ(0, reverse_digits(-0x10000008, 16));
}
120
is21
, how can you know whether 'the' reverse of21
should be120
or12
? What makes 'the' reverse of1
number1
and not10000
? How can you tell your code solves the problem if the correct solution is not defined? \$\endgroup\$ – CiaPan Mar 26 '19 at 23:41