Decimal to binary and binary to decimal convertor

Question

Please review this code. How can I optimise this code?

#include <iostream>
#include <vector>
#include <algorithm> // std::reverse
#include <cmath> // pow()

typedef unsigned long long int ulli;

bool is_binary(ulli num)
{
    bool status = true;
    while (true)
    {
        if (num == 0)
        {
            break;
        }
        else
        {
            int temp = num % 10;
            if (temp > 1)
            {
                status = false;
                break;
            }
            num = num / 10;
        }
    }
    return status;
}

std::vector<ulli> decimal_to_binary(int val)
{
    std::vector<ulli> result;
    int rem;
    if (val == 0)
    {
        result.push_back(0);
        return result;
    }
    while (val != 0)
    {
        rem = val % 2;
        result.push_back(rem);
        val = val / 2;
    }
    std::reverse(result.begin(), result.end());
    return result;
}

int binary_to_decimal(ulli val)
{
    int sum = 0;
    int rem, i = 0;
    while (val > 0)
    {
        rem = val % 10;
        sum += pow(2, i) * rem;
        val = val / 10;
        i++;
    }
    return sum;
}

void display(std::vector<ulli>& vec)
{
    for (int i = 0; i < vec.size(); i++)
    {
        std::cout << vec[i];
    }
    std::cout << '\n';
}

int main()
{
    int option;
    std::cout << "1. Decimal to Binary \n2. Binary to Decimal \n";
    std::cout << "Enter your option\n";
    std::cin >> option;

    switch(option)
    {
        case 1:
          {
             int num;
             std::cout <<"\nEnter Decimal number\n";
             std::cin >> num;
             std::vector<ulli> binary = decimal_to_binary(num);
             std::cout << "The binary equivalent of " << num <<" is :\n";
             display(binary);
             break;
          }
        case 2:
          {
             ulli binary_num;
             x:
             std::cout << "\nEnter Binary number\n";
             std::cin >> binary_num;
             bool flag = is_binary(binary_num);
             if (!flag)
             {
                std::cout << "The number is not binary\n";
                goto x;
             }
             int decimal = binary_to_decimal(binary_num);
             std::cout << "The decimal equivalent of " << binary_num << " is :\n";
             std::cout << decimal << '\n';
             break;
          }
        default: std::cout << "Enter option 1 or 2\n";
    }
}

Answer 1

Validate User Input:

Always check user input is what you expect. The user may enter a valid number, or they may enter a number too long to be stored in the given type, or they might enter "fifty-three".

The code should handle these cases, rather than crashing, entering an infinite loop (option 2) or producing incorrect output (option 1). These are bugs in your program that need to be fixed. Check the answers here for ideas.

One way to do this is to loop and re-request data until the user enters an expected value. This pattern can be applied to significantly improve the overall structure of your program, e.g.:

while (true)
{
    auto conversionType = ask_conversion_type();

    if (conversionType == ConversionType::BinaryToDecimal)
    {
        // ... ask user for binary input with another loop
    }
    else if (conversionType == ConversionType::DecimalToBinary)
    {
        // ... ask user for decimal input with another loop
    }
    else
    {
        // error...
    }

    // maybe we also need an exit option?
}

---

Testing

Write unit tests, so you can check that the various components of the code work. In the simplest case, this can be done by adding a little function like so:

void test(bool condition)
{
    if (condition)
        std::cout << "pass" << std::endl;
    else
        std::cout << "FAIL" << std::endl;
}

and then writing some tests at the start of main() to make sure you get the output you expect for each function:

test(decimal_to_binary(0) == std::vector<ulli>{ 0 });
test(decimal_to_binary(2) == std::vector<ulli>{ 1, 0 });
//test(decimal_to_binary(-5) == um??? );

This will also help with thinking about edge cases!

---

Types

If each number in the vector will only store 0 or 1, maybe it doesn't need to be a vector<ulli>...

---

Context

What is the purpose of this code? If it's an excercise doing a particular thing (i.e. you have a special definition of a "binary number" you must adhere to, or you have to write your own functions), then you need to share that with us, lest it all be "optimised" away.

For example, we could just use std::bitset:

#include <bitset>
...

case 1:
{
    std::cout << "\nEnter Decimal number\n";
    unsigned long long num;
    std::cin >> num;
    // check input... then:
    std::bitset<64> bits(num);
    std::cout << "The binary equivalent of " << num << " is :\n" << bits << "\n";
    break;
}
case 2:
{
    std::cout << "\nEnter Binary number\n";
    std::bitset<64> bits;
    std::cin >> bits;
    // check input... then:
    std::cout << "The decimal equivalent of " << bits << " is :\n" << bits.to_ullong() << "\n";
    break;
}

The code is certainly shorter! But maybe doesn't fulfill some other requirements?

Answer 2

Prior to any optimization, you must try to establish the soundness of your design. And I feel that your design doesn't distinguish enough between a number and its (based) representation. is_binary doesn't really make sense, since every integer has a binary representation -and is actually stored in a binary format. I know you know this, but this knowledge must find its way into your code.

It means that you should have two different kinds of functions: functions to express the representation of a number in different bases, and functions to extract a number from one of its possible representations. Once you have identified this need, you can decide on the best types to use.

You could choose std::string for representations, for instance. It allows you to represent numbers in bases greater than 10. An iterator-based interface might be yet more versatile (you can write directly to the output stream without allocating memory as you would have to do for a string). Here's a quick-and-dirty implementation of the representation part:

#include <iostream>
#include <iterator>
#include <cmath>

auto log_n(unsigned base, double d) { // not provided in cmath
    return std::log(d) / std::log(base);
}

auto digit_at(unsigned base, unsigned pos, unsigned number) {
    unsigned left_digits = number / std::pow(base, pos);
    return left_digits % base;
}

template <std::size_t Base>
struct number_iterator {

    using value_type = unsigned;
    using iterator_category = std::input_iterator_tag;

    number_iterator() = default;
                                             // position is the number of digits in the target base
    number_iterator(unsigned i) : number(i), position(static_cast<int>(log_n(Base, number))) {}
    number_iterator(const number_iterator&) = default;

    bool operator!=(const number_iterator& o) { return position != o.position; }

    value_type operator*() const { return digit_at(Base, position, number); }
    number_iterator& operator++()    { --position; return *this; }
    number_iterator  operator++(int) { auto tmp = *this; ++(*this); return tmp; }

    unsigned number = 0;
    int position = -1;
};

template <std::size_t Base>
struct representation {
    representation(unsigned i) : number(i) {}
    auto begin() const { return number_iterator<Base>(number); }
    auto end()   const { return number_iterator<Base>(); }
    unsigned number;
};

int main() {
    std::cout << "decimal: ";
    for (auto n : representation<10>(25435)) std::cout << n;
    std::cout << '\n';
    std::cout << "octal:   ";
    for (auto n : representation<8>(25435))  std::cout << n;
    std::cout << '\n';
    std::cout << "binary:  ";
    for (auto n : representation<2>(25435))  std::cout << n;
    std::cout << '\n';
}

Naturally, there is still much to be added, such as hexadecimal support, but the ground design is flexible enough to accommodate extensions easily.

The other operation (representation -> number), can also be iterator based, since its formula can be expressed as:

// ((((it[0]*base)+it[1]*base)+...*base)+it[n])

so the interface would be:

// needs #include <algorithm>
template <std::size_t Base, typename Iterator>
auto to_number(Iterator first, Iterator last) {
    return std::accumulate(first, last, 0, [](auto lhs, auto rhs) {
        return lhs*base+rhs; // or value(rhs) is a custom type is needed
    });
}