5
\$\begingroup\$

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 <iostream>
#include <string>

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 << "\n\n* Decimal -> Binary & Hex (1)\n";
    std::cout << "* Binary -> Decimal & Hex (2)\n";
    std::cout << "* Hex -> Binary & Decimal (3)\n\n";
    std::cin >> choice;

    if (choice == 1)
    {
        std::cout << "\n> Decimal Input: ";
        std::cin >> decimal;
        std::string binaryOutput = decimalToBinary(decimal);
        std::string hexOutput = binaryToHexadecimal(binaryOutput);
        std::cout << "\n  Decimal Output: " << binaryOutput;
        std::cout << "\n  Hex Output    : " << hexOutput << "\n\n\n";
    }

    else if (choice == 2)
    {
        std::cout << "\n> Binary Input: ";
        std::cin.ignore();
        std::getline(std::cin, binary);
        uInt64 decimalOutput = binaryToDecimal(binary);
        std::string hexOutput = binaryToHexadecimal(binary);
        std::cout << "\n  Hex Output    : " << hexOutput;
        std::cout << "\n  Decimal Output: " << decimalOutput << "\n\n\n";

    }

    else if (choice == 3)
    {
        std::cout << "\n> Hex Input: ";
        std::cin.ignore();
        std::getline(std::cin, hex);
        uInt64 decimalOutput = hexadecimalToDecimal(hex);
        std::string binaryOutput = decimalToBinary(decimalOutput);
        std::cout << "\n  Binary Output : " << binaryOutput;
        std::cout << "\n  Decimal Output: " << decimalOutput << "\n\n\n";
    }

    system("PAUSE");
}

uInt64 calculatePower(uInt64 base, uInt64 exponent)
{
    uInt64 total = 1;

    for (uInt64 iter = 0; iter < exponent; iter++)
        total *= base;

    return total;
}

uInt64 binaryToDecimal(std::string binary)
{
    uInt64 decimal = 0;
    uInt64 exponent = 0;
    std::string::reverse_iterator iter;

    for (iter = binary.rbegin(); iter != binary.rend(); iter++)
    {
        if (*iter == '1')
            decimal += calculatePower(2, exponent);

        exponent++;
    }

    return decimal;
}

uInt64 hexadecimalToDecimal(std::string binary)
{
    uInt64 decimal = 0;
    uInt64 exponent = 0;
    std::string::reverse_iterator iter;

    for (iter = binary.rbegin(); iter != binary.rend(); iter++)
    {
        switch (*iter)
        {
        case '1':
            decimal += calculatePower(16, exponent);
            break;
        case '2':
            decimal += (2 * calculatePower(16, exponent));
            break;
        case '3':
            decimal += (3 * calculatePower(16, exponent));
            break;
        case '4':
            decimal += (4 * calculatePower(16, exponent));
            break;
        case '5':
            decimal += (5 * calculatePower(16, exponent));
            break;
        case '6':
            decimal += (6 * calculatePower(16, exponent));
            break;
        case '7':
            decimal += (7 * calculatePower(16, exponent));
            break;
        case '8':
            decimal += (8 * calculatePower(16, exponent));
            break;
        case '9':
            decimal += (9 * calculatePower(16, exponent));
            break;
        case 'A':
            decimal += (10 * calculatePower(16, exponent));
            break;
        case 'B':
            decimal += (11 * calculatePower(16, exponent));
            break;
        case 'C':
            decimal += (12 * calculatePower(16, exponent));
            break;
        case 'D':
            decimal += (13 * calculatePower(16, exponent));
            break;
        case 'E':
            decimal += (14 * calculatePower(16, exponent));
            break;
        case 'F':
            decimal += (15 * calculatePower(16, exponent));
            break;
        }

        exponent++;
    }

    return decimal;
}

std::string decimalToBinary(uInt64 decimal)
{
    std::string binary, newBinary = "";
    std::string::reverse_iterator iter;
    uInt64 remainder;

    while (decimal > 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++)
    {
        newBinary += *iter;
    }

    return newBinary;
}

std::string binaryToHexadecimal(std::string binary)
{
    std::string hex, newHex = "";
    std::string::reverse_iterator iter;

    uInt64 incr = 1;

    uInt64 exponent = 0;

    uInt64 total = 0;

    for (iter = binary.rbegin(); iter != binary.rend(); iter++)
    {
        if (*iter == '1')
            total += calculatePower(2, exponent);

        if (incr == 4)
        {
            switch (total)
            {
            case 1:
                hex += '1';
                break;
            case 2:
                hex += '2';
                break;
            case 3:
                hex += '3';
                break;
            case 4:
                hex += '4';
                break;
            case 5:
                hex += '5';
                break;
            case 6:
                hex += '6';
                break;
            case 7:
                hex += '7';
                break;
            case 8:
                hex += '8';
                break;
            case 9:
                hex += '9';
                break;
            case 10:
                hex += 'A';
                break;
            case 11:
                hex += 'B';
                break;
            case 12:
                hex += 'C';
                break;
            case 13:
                hex += 'D';
                break;
            case 14:
                hex += 'E';
                break;
            case 15:
                hex += 'F';
                break;
            }

            incr = 0;
            exponent = -1;
            total = 0;
        }

        incr++;
        exponent++;
    }

    newHex += "0x";

    for (iter = hex.rbegin(); iter != hex.rend(); iter++)
    {
        newHex += *iter;
    }

    return newHex;
}
\$\endgroup\$
  • \$\begingroup\$ Are you willing to the code tied to ASCII? If so, the easiest simplification I see is to use c - '0' to get offsets. Like decimal += (*iter - '0') * calculatePower(16, exponent). (Technically not tied to ASCII; just requires that '0'...'9' be contiguous.) \$\endgroup\$ – Corbin Apr 23 '13 at 17:30
  • \$\begingroup\$ I am, if it'll go in place of the switch statements. Even what I have now is clear, but very bloated. Maybe I can work something out with a char array... \$\endgroup\$ – Jamal Apr 23 '13 at 17:36
5
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ I've added what I have now. I still need to simplify binaryToHex, or I can just keep it this way. I'll look at the input validation later. \$\endgroup\$ – Jamal Apr 23 '13 at 22:32
  • \$\begingroup\$ Should be <cstdint> instead of <stdint.h> and if you want to be really pedantic, std::uint64_t. Otherwise, +1. \$\endgroup\$ – Yuushi Apr 24 '13 at 3:44
  • \$\begingroup\$ @Yuushi Yeah, I definitely should have mentioned the C++11 version (no sign that he's using C++11, but he everyone should probably be headed that direction). With stdint.h though, std::uint64_t is actually wrong. Non c* version is guaranteed to put the types in the global namespace, but not in std. \$\endgroup\$ – Corbin Apr 24 '13 at 4:39
  • \$\begingroup\$ Yeah, that needs a logical connection in there - <cstdint> => std::uint64_t since it's the other way around (guaranteed to be in std but not in global). Have to love the interop rules sometimes... \$\endgroup\$ – Yuushi Apr 24 '13 at 4:45
  • 1
    \$\begingroup\$ In C++11 If a read fails. The value is guaranteed to be zero. From n3376: See [istream.formatted.arithmetic] => [facet.num.get.virtuals] Stage 3: The numeric value to be stored can be one of: 1: zero, if the conversion function fails to convert the entire field. ios_base::failbit is assigned to err. 2: .... \$\endgroup\$ – Martin York Apr 24 '13 at 15:35
2
\$\begingroup\$

You repeated calls to calculatePower are rather inefficient. There is no need to call this every time round a loop. In binaryToDecimal, you could do this:

    std::string::const_reverse_iterator i = b.rbegin();
    for (int exp = 0; i != b.rend(); ++i, ++exp) {
        if (*i == '1') {
            decimal += (1 << exp);
        }
    }

and in hexadecimalToDecimal:

    std::string::const_reverse_iterator i = h.rbegin();
    for (int exp = 0; i != h.rend(); ++i, exp += 4) {
    {
        int n = 0;
        if ((*i >= '0') && (*i <= '9')) {
            n = *i - '0';
        }
        else if ((*i >= 'a') && (*i <= 'f')) {
            n = *i - 'a';
        }
        else if ((*i >= 'A') (*i <= 'F')) {
            n = *i - 'A';
        }
        decimal += (n << exp);
    }

For binaryToHexadecimal it would be easier to convert binary to machine (what you call decimal) and then machine to hex.

\$\endgroup\$
2
\$\begingroup\$

Perhaps I'm missing something, but is there a reason you aren't converting to unsigned long long first with any of a set of functions, then formatting the output in a separate set of functions? e.g.:

unsigned long long fromHex(std::string& number);
unsigned long long fromBin(std::string& number);
unsigned long long fromDec(std::string& number);
std::string toHex(unsigned long long number);
std::string toBin(unsigned long long number);
std::string toDec(unsigned long long number);

It seems a bit inconsistent to use the term "decimal" for an int in system representation. It would just be confusing, if not for the problematic differences in function signatures between each of the XtoY functions. If it was more uniform, you could:

class NumberFormat {
public:
    static virtual unsigned long long from(std::string& number);
    static virtual std::string to(unsigned long long number);
    std::string description;
}

Then, you can make your list of number formats more declarative; construct an array, output the descriptions, and have the user pick what will effectively become an index into the array for the source format. Then read the destination format and numeric string, in either order.

As it stands, the input method is needlessly coupled to a finite set of formats and their names, as is the source/destination pairings.

I once wrote a hex-by-default calculator/expression evaluator in assembly, so I'm the last guy that's going to call a wheel-reinvention foul on you; in fact, I like the general theme of your code because I've never seen a number formatter that covers absolutely everything, like your binary output's adding leading zeroes to pad 4 bit groups. Bravo on making a wheel that spins exactly like you want it to.

With regard to the conversions themselves, I see more twos complement math than is necessary. In particular, I see several %s while converting to binary. If I'm interested in a number's binary representation, I will probably find the equivalent ones complement operations in your code just as readable, if not more so. The sequence:

if (usersDecimal & 1) {
    newBinary += '1';
} else {
    newBinary += '0';
}
usersDecimal >>= 1;

just screams "I am extracting a single bit from the number and printing it!" to me, whereas with % and /=, it seems like you might be calculating something at first glance. And, if you really like brevity:

newBinary += ((usersDecimal & 1) + '0');
usersDecimal >>= 1;

will get the job done. Not particularly clean, though.

Other than that, the particulars of the conversions are good. Stuff like constants for DIGIT_OFFSET strike a good balance between efficiency and readability.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.