Decimal To Binary using only the basics

Question

Convert a decimal number to binary using only loops, strings, if/else statements.

This is my first C++ project and it took me a while to do it. Let me know what you think.

#include <iostream>
#include <string>

using namespace std;

string ConvertInt(int value); 
string ConvertDec(double value);
void printBinary1(string str1);
void printBinary2(string str2);

int main()
{   
    double number, decValue;
    int remainder, intValue;
    string binary1, binary2;

    cout << "Enter a deciaml value to be converted to binary. Then press ENTER: ";
    cin >> number;

    intValue = (int)number;         
    decValue = number - intValue;       

    binary1 = ConvertInt(intValue);         

    binary2 = ConvertDec(decValue);             

    printBinary1(binary1);                          

    cout << ".";

    printBinary2(binary2);                                  

    int pause;
    cin >> pause;

    return 0;
}   

string ConvertInt(int value)
{   
    int remainder;
    string binary;

    while(value != 0)
    {
        remainder = value % 2;              
        binary += remainder;                        
        value /= 2;
    }

    return binary;

}   

string ConvertDec(double value)
{   
    string binary;
    int integer;
    double decimal;

    for(int i = 1; i <= 5; i++)
    {
        integer = (int)value;
        binary += integer;
        decimal = value - integer;
        value = decimal * 2;
    }

    return binary;
}

void printBinary1(string str1)
{
    string binary1 = str1;

    for(int i = (binary1.length() - 1); i <= (binary1.length() - 1); i--)
    {
        cout <<  (int)binary1[i];
    }
}

void printBinary2(string str2)
{
    string binary2 = str2;

    for(int index = 1; index < binary2.length(); index++)
    {
        cout << (int)binary2[index];
    }
}

Answer 1

It is considered bad practice (by most people) to declare more than one variable on a line.

double number, decValue;
int remainder, intValue;
string binary1, binary2;

Though it is not a problem here there are some corner cases where the declaration gets confusing.

Second you declare all your variables at the top of the function.
This is a very C like style and in C++ it is usually better to declare the variable as close to the point of usage as possible (no need to invoke a constructor if you are never going to use the variable, also it makes reading the code easier as you can see the type at the fist point of usage).

I know all the books say to use:

using namespace std;

But in the long run its not a good idea. Get used to prefix things with their namespace std::string, or importing the specific bits you want using std::string (even then scope as tightly as possible (you would not believe how many time string has been reinvented)).

Variables should be descriptive (and more importantly unique and easy to find in the source).

for(int i = 1; i <= 5; i++)

Think about 10 years of maintenance on this code. The code now covers 5 long pages. Try and find and update all usages of i. This becomes a pane as i will appear in nearly every other identifier in all the comments in lots of places etc. So try and use variables that can be easily searched for.

In your ConvertXXX() functions. You are saving integer values into the string. This is fine. But you are then extracting those char values and converting to int for printing. Why not convert the values directly into the characters you want to print.

string ConvertDec(double value)
{   
    // STUFF
         binary += '0' + integer; // each digit will be the character 0 or 1
    // MORE STUFF
}

When printing:

cout <<  (int)binary1[i];

// Can change this too:

cout <<  binary1[i];

More importantly you can replace the loop:

for(int index = 1; index < binary2.length(); index++)
{
    cout << (int)binary2[index];
}

// Can be changed too:

cout << binary2

Looking at your loops:

for(int i = (binary1.length() - 1); i <= (binary1.length() - 1); i--)

Does this even work? It seems that i is getting smaller and smaller then goes negative.

It is easier to loop up and subtract in the range:

for(int loop = 0; loop < bianry1.length(); ++lop)
{
    std::cout << bianry1[binary1.length() - 1 - loop];
}

But the std containers (including string) provide iterators (and more importantly reverse iterators)

for(std::string::const_reverse_iterator loop = bianry1.rbegin(); loop != binary1.rend(); ++loop)
{
    std::cout << *loop;
}

Second loop:

for(int index = 1; index < binary2.length(); index++)

Problem here is that you are starting from 1. arrays/containers nearly everything is indexed from 0 and you can access the elements 0 -> (size() - 1). So here you are starting at the wrong point and going past the end of the string.

But as mentioned above you don't even need this as you can replace the loop.

Answer 2

There are some issues with the code:

• You are (mis)using strings as integer arrays.
• The functions in the code are codependent. The ConvertInt functions returns a string that is unusable unless you use printBinary1 to display it. The same for ConvertDec and printBinary2.
• The ConvertDec function produces a result with a unused value (at index 0), which then has to be avoided in the printBinary2 function.
• There is a bug in printBinary1 where the condition in the loop should be i >= 0 rather than i <= (binary1.length() - 1).

You should either use a different format for storing the intermediate data, or make the ConvertInt and ConvertDec functions return real strings that you can just display without needing separate functions for that.

If you keep the codependent functions, their naming should suggest that they are to be used in pairs.