General
#General II like the presentation. It's easy to read, with good use of whitespace and useful comments.
Width
#Width
It'sIt's inconvenient to have to recompile to use a larger width HugeInt
, and impossible to mix sizes. Consider making numDigits
a template parameter (and use an unsigned type for it - perhaps std::size_t
).
Conversions
#Conversions
IfIf this were my code, I think I'd make the char*
constructor explicit
. The one taking long int
seems reasonable to accept as implicit.
Comparisons
#Comparisons
ImplementingImplementing the relational operators in terms of subtraction misses an opportunity: if we find a difference in the high-order digits, there's no need to examine the rest of the number. I'd consider writing a simple <=>
function, and using that to implement the public comparisons. (In C++20, you'll be able to implement operator<=>()
and the compiler will then produce all the rest for you).
Streaming
#Streaming
We'reWe're missing an operator >>
to accept input from a standard stream.
Missing std::
qualifier
#Missing std::
qualifier
AA lot of the C Standard Library identifiers are missing their namespace prefix (e.g. std::abs
, std::strlen
, etc). These should be specified, as these names are not guaranteed to also be in the global namespace.
#Overflow bug
Overflow bug
Internationalisation
#Internationalisation ThisThis loop embodies a specific locale convention:
Input handling
#Input handling
II know it's only meant to be illustrative, but here we need to check the status of std::cin
before repeating the loop:
#General I like the presentation. It's easy to read, with good use of whitespace and useful comments.
#Width
It's inconvenient to have to recompile to use a larger width HugeInt
, and impossible to mix sizes. Consider making numDigits
a template parameter (and use an unsigned type for it - perhaps std::size_t
).
#Conversions
If this were my code, I think I'd make the char*
constructor explicit
. The one taking long int
seems reasonable to accept as implicit.
#Comparisons
Implementing the relational operators in terms of subtraction misses an opportunity: if we find a difference in the high-order digits, there's no need to examine the rest of the number. I'd consider writing a simple <=>
function, and using that to implement the public comparisons. (In C++20, you'll be able to implement operator<=>()
and the compiler will then produce all the rest for you).
#Streaming
We're missing an operator >>
to accept input from a standard stream.
#Missing std::
qualifier
A lot of the C Standard Library identifiers are missing their namespace prefix (e.g. std::abs
, std::strlen
, etc). These should be specified, as these names are not guaranteed to also be in the global namespace.
#Overflow bug
#Internationalisation This loop embodies a specific locale convention:
#Input handling
I know it's only meant to be illustrative, but here we need to check the status of std::cin
before repeating the loop:
General
I like the presentation. It's easy to read, with good use of whitespace and useful comments.
Width
It's inconvenient to have to recompile to use a larger width HugeInt
, and impossible to mix sizes. Consider making numDigits
a template parameter (and use an unsigned type for it - perhaps std::size_t
).
Conversions
If this were my code, I think I'd make the char*
constructor explicit
. The one taking long int
seems reasonable to accept as implicit.
Comparisons
Implementing the relational operators in terms of subtraction misses an opportunity: if we find a difference in the high-order digits, there's no need to examine the rest of the number. I'd consider writing a simple <=>
function, and using that to implement the public comparisons. (In C++20, you'll be able to implement operator<=>()
and the compiler will then produce all the rest for you).
Streaming
We're missing an operator >>
to accept input from a standard stream.
Missing std::
qualifier
A lot of the C Standard Library identifiers are missing their namespace prefix (e.g. std::abs
, std::strlen
, etc). These should be specified, as these names are not guaranteed to also be in the global namespace.
Overflow bug
Internationalisation
This loop embodies a specific locale convention:
Input handling
I know it's only meant to be illustrative, but here we need to check the status of std::cin
before repeating the loop:
When streaming out, we might be able to produce two digits at a time if we carefully manage leading zeros - that will reduce the number of divisions by around 50%.
When streaming out, we might be able to produce two digits at a time if we carefully manage leading zeros - that will reduce the number of divisions by around 50%.
#General I like the presentation. It's easy to read, with good use of whitespace and useful comments.
#Width
It's inconvenient to have to recompile to use a larger width HugeInt
, and impossible to mix sizes. Consider making numDigits
a template parameter (and use an unsigned type for it - perhaps std::size_t
).
If we template the width, then we'll have a bit of work to do to support promotions between different width values, but you'll find that good exercise.
#Conversions
If this were my code, I think I'd make the char*
constructor explicit
. The one taking long int
seems reasonable to accept as implicit.
Consider adding an explicit operator bool()
to allow idiomatic tests such as if (!num)
.
#Comparisons
Implementing the relational operators in terms of subtraction misses an opportunity: if we find a difference in the high-order digits, there's no need to examine the rest of the number. I'd consider writing a simple <=>
function, and using that to implement the public comparisons. (In C++20, you'll be able to implement operator<=>()
and the compiler will then produce all the rest for you).
#Streaming
We're missing an operator >>
to accept input from a standard stream.
#Missing std::
qualifier
A lot of the C Standard Library identifiers are missing their namespace prefix (e.g. std::abs
, std::strlen
, etc). These should be specified, as these names are not guaranteed to also be in the global namespace.
#Overflow bug
long int xp{std::abs(x)};
On twos-complement systems, LONG_MIN
is greater in magnitude than LONG_MAX
, so we fail to convert LONG_MIN
correctly.
#Internationalisation This loop embodies a specific locale convention:
for (int j = i - 1; j >= 0; --j) { if (j < i - 1) { if ((j + 1) % 3 == 0) // show thousands separator { oss << ','; // thousands separator } }
That's fine for European English, but isn't a good match for Indian English, for example. I believe we can get information from the locale's std::numpunct
facet, but I don't know the details.
I worry that writing separators by default (and with no option to disable) may be a poor choice unless we update our string-to-number conversion to be able to ignore separators - I'm much more comfortable when a round-trip will work.
#Input handling
I know it's only meant to be illustrative, but here we need to check the status of std::cin
before repeating the loop:
do {
std::cout << "Enter a non-negative integer (0-200): ";
std::cin >> inum;
} while (inum < 0 || inum > 200);
If I give -1
as input (and nothing else), the program enters an infinite loop, because the closed stream never changes inum
to an acceptable value. For a simple program like this, perhaps it's simplest to arrange the stream to throw on EOF, and possibly on other errors.