Skip to main content
Code Review

Return to Revisions

6 of 6
replaced http://stackoverflow.com/ with https://stackoverflow.com/

system is bad. Either use getchar(), set your IDE to pause after execution, or use a terminal when running a terminal program.


The typedef for uint64 should just be uint64_t from stdint.h.


Your input reading should be checked. For example, std::cin >> choice; can fail. It should be

if (!(std::cin >> choice)) { /* handle this */ }

If you're thinking you're ok because choice is checked to be either 1, 2, or 3, you're not ok. You're just almost certainly ok. If the read fails, choice will still have an indeterminate value. That could mean 1, 2, or 3.


You can basically replace all of your ToDecimal functions with strtoul (C >= C89) or strtoull (C >= C99). An unsigned long is only gauranteed to be 4 bytes, so you'll want the strtoull version.


Similarly, sprintf() can convert to hex for you (format llX).


If you're not willing to do that, but you're willing to assume that '0'...'9' is always contiguous, you can use the class value = c - '0' shortcut.

As an example, you could simplify your beastly hexidecimalToDecimal():

uint64_t hexadecimalToDecimal(const std::string& binary)
{
 uint64_t decimal = 0;
 uint64_t power = 1;
 std::string::const_reverse_iterator iter;
 for (iter = binary.rbegin(); iter != binary.rend(); iter++)
 {
 const char ch = std::tolower(*iter);
 if (ch >= 'a') {
 decimal += (ch - 'a') * power;
 } else {
 decimal += (ch - '0') * power;
 }
 power *= 16;
 }
 return decimal;
}

When arguments are NOT modified, they should be passed as const references. This avoids the overhead of copying. (And it's part of a larger concept called const-correctness.)


I tend to put arguments names in declarations. In this situation, it's a marginal concern since all of the functions have very obvious parameters, but in some cases, it can be quite confusing to see a declaration and wonder what the different params are.


Unless performance is tight, I'd be tempted to implement everything in terms of decimal. For example, binaryToHex(bin): decimalToHex(binaryToDecimal(bin))


Speaking of performance, if you wanted to, you could inline the exponent calculations instead of recalculating it from scratch every time.

uint64_t binaryToDecimal(const std::string& binary)
{
 uint64_t decimal = 0;
 uint64_t p = 1;
 std::string::const_reverse_iterator iter;
 for (iter = binary.rbegin(); iter != binary.rend(); iter++)
 {
 if (*iter == '1')
 decimal += p;
 p *= 2;
 }
 return decimal;
}

(And if you're really paranoid about performance, you'll want that iter++ to be ++iter as it avoid a potential copy)


If for some odd reason you don't want to do that, calculatePower() can be optimized in many different ways:

  • Use the double version and just cast back to uint64_t.
  • Use powers of 2 and abuses of shifting (x16 = x << 4)
  • Use divide and conquer style

remainder = decimal % 2;
if (remainder == 0)
 binary += '0';
else if (remainder == 1)
 binary += '1';

As remaining can only be 0 <= remainder <= 1, the else if is unnecessary. I would just use:

if (decimal % 2 == 0) {
 binary += '0';
} else {
 binary += '1';
}

You can use std::reverse (from <algorithm>) rather than your manual reversal loop.

Example:

std::string decimalToBinary(uint64_t decimal)
{
 std::string binary;
 while (decimal > 0)
 {
 if (decimal % 2 == 0)
 binary += '0';
 else 
 binary += '1';
 decimal /= 2;
 }
 std::reverse(binary.begin(), binary.end());
 return binary;
}
 
Corbin
  • 10.6k
  • 2
  • 31
  • 51
default

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