Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

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:

added 167 characters in body
Source Link
Toby Speight
  • 87.8k
  • 14
  • 104
  • 325

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%.

Source Link
Toby Speight
  • 87.8k
  • 14
  • 104
  • 325

#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.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /