HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Decimal/binary/hex converter

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
hexdecimalbinaryconverter

Problem

I've finally finished my converter and have determined that it works, but I'm trying to make it cleaner and/or more practical (primarily with the switch statements). I could probably put more things into functions, but it'll be needless unless I can replace the switch statements with something simpler.

Are there other methods of performing these conversions that would use less code, or are these methods good enough? I am also using unsigned long long so that I can work with large numbers, but I can always change that if it may not be the best thing to do.

```
#include
#include

typedef unsigned long long uInt64;

uInt64 calculatePower(uInt64, uInt64);
uInt64 binaryToDecimal(std::string);
uInt64 hexadecimalToDecimal(std::string);
std::string decimalToBinary(uInt64);
std::string binaryToHexadecimal(std::string);

int main()
{
std::string binary, hex;
uInt64 decimal;
uInt64 choice;

std::cout Binary & Hex (1)\n";
std::cout Decimal & Hex (2)\n";
std::cout Binary & Decimal (3)\n\n";
std::cin >> choice;

if (choice == 1)
{
std::cout Decimal Input: ";
std::cin >> decimal;
std::string binaryOutput = decimalToBinary(decimal);
std::string hexOutput = binaryToHexadecimal(binaryOutput);
std::cout Binary Input: ";
std::cin.ignore();
std::getline(std::cin, binary);
uInt64 decimalOutput = binaryToDecimal(binary);
std::string hexOutput = binaryToHexadecimal(binary);
std::cout Hex Input: ";
std::cin.ignore();
std::getline(std::cin, hex);
uInt64 decimalOutput = hexadecimalToDecimal(hex);
std::string binaryOutput = decimalToBinary(decimalOutput);
std::cout 0)
{
remainder = decimal % 2;

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

decimal /= 2;
}

for (iter = binary.rbegin(); iter != binary.rend(); iter++)
{

Solution

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



  • 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
`) 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;
}

Code Snippets

if (!(std::cin >> choice)) { /* handle this */ }
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;
}
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;
}
remainder = decimal % 2;

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

Context

StackExchange Code Review Q#25381, answer score: 5

Revisions (0)

No revisions yet.