1
\$\begingroup\$

I'm a fairly new programmer and have been learning c++ for about 8 months now. I've written a program that can take either binary, hexadecimal, and decimal inputs and convert them to the other two values. For example, if the user inputs a binary value, then the program will output both decimal and hexadecimal conversions of the number. Unfortunately, the colouring of the output is unsatisfactory due to the fact that chaning colours in cmd changes the colour off all texts in that terminal. Instead of using the cmd colour commands, I used a custom header file provided by my teacher that didn't change the colour of all the texts in the terminal. Instead of adding the header file, I replaced all the custom colour commands with cmd colour commands, for convinience. Besides all that, it would mean a lot if you could review my code and point out on where I could improve on. Please keep in mind that I'm still new to programming in general and wouldn't understand some "programmer/coding" vocabulary.

Thank you.

#include <iostream>
#include <cmath>
#include <string>
#include <conio.h>
using namespace std;

long long num_limit;//user input for decimal
long long hex_remainder;//hexadecimal remainder
long long hexint = 0;//integer value for user input
char user_inp;//identifier for user input
bool userinput = true;//validator for illegal input
string hex_assign;//assiging hex remainder string value
string bitassign = "";//identifier assigning binary values to hex

void load()//loading effect
{
int g, c;//loop counter
cout << " ";
system("color 07");
for (g = 1; g <= 80; g++)
{
    for (c = 1; c <= 2850000; c++) {}
    cout << "*";
}
//used in void bit(), void hex(), and void dec()
}//load
void integer_only()
{
while (!num_limit > 0)//checking for illegal input
{
    cin.clear();
    cin.ignore(1000, '\n');
    system("color 04");
    cout << " Invalid input...try again: ";
    system("color");
    cin >> num_limit;//user input
    cin.ignore(1000, '\n');
}
//used in void bit(), void hex(), and void dec()
}//integer_only
void hexassign()
{
switch (hex_remainder)//using <remainder> as a switch to assign hex characters
{
case 0: hex_assign = "0"; break;
case 1: hex_assign = "1"; break;
case 2: hex_assign = "2"; break;
case 3: hex_assign = "3"; break;
case 4: hex_assign = "4"; break;
case 5: hex_assign = "5"; break;
case 6: hex_assign = "6"; break;
case 7: hex_assign = "7"; break;
case 8: hex_assign = "8"; break;
case 9: hex_assign = "9"; break;
case 10: hex_assign = "A"; break;
case 11: hex_assign = "B"; break;
case 12: hex_assign = "C"; break;
case 13: hex_assign = "D"; break;
case 14: hex_assign = "E"; break;
case 15: hex_assign = "F"; break;
}
//used in void bit() and void dec()
}//hexassign
void hex_input_validator()
{
switch (user_inp)
{
case '0':userinput = true; break;
case '1':userinput = true; break;
case '2':userinput = true; break;
case '3':userinput = true; break;
case '4':userinput = true; break;
case '5':userinput = true; break;
case '6':userinput = true; break;
case '7':userinput = true; break;
case '8':userinput = true; break;
case '9':userinput = true; break;
case 'A':userinput = true; break;
case 'B':userinput = true; break;
case 'C':userinput = true; break;
case 'D':userinput = true; break;
case 'E':userinput = true; break;
case 'F':userinput = true; break;
default: userinput = false;
}
//used in void hex()
}//hexinput_validator
void hex_binary_assign()
{

switch (user_inp)
{
case '0':bitassign = "0000 "; break;
case '1':bitassign = "0001 "; break;
case '2':bitassign = "0010 "; break;
case '3':bitassign = "0011 "; break;
case '4':bitassign = "0100 "; break;
case '5':bitassign = "0101 "; break;
case '6':bitassign = "0110 "; break;
case '7':bitassign = "0111 "; break;
case '8':bitassign = "1000 "; break;
case '9':bitassign = "1001 "; break;
case 'A':bitassign = "1010 "; break;
case 'B':bitassign = "1011 "; break;
case 'C':bitassign = "1100 "; break;
case 'D':bitassign = "1101 "; break;
case 'E':bitassign = "1110 "; break;
case 'F':bitassign = "1111 "; break;
}
//used in void hex()
}//hex_binary_assign
void hex_int_assign()
{
switch (user_inp)
{
case '0':hexint = 0; break;
case '1':hexint = 1; break;
case '2':hexint = 2; break;
case '3':hexint = 3; break;
case '4':hexint = 4; break;
case '5':hexint = 5; break;
case '6':hexint = 6; break;
case '7':hexint = 7; break;
case '8':hexint = 8; break;
case '9':hexint = 9; break;
case 'A':hexint = 10; break;
case 'B':hexint = 11; break;
case 'C':hexint = 12; break;
case 'D':hexint = 13; break;
case 'E':hexint = 14; break;
case 'F':hexint = 15; break;
}
//used in void hex()
}//hex_int_assign
void bit()//binary input
{
char user_input = 'x';//user input (1/0)
int num;//identifier assigned integer value for user input
int i, v, y;//loop counter
long long total_dec;//decimal value 
long long dec_count = 0;//total decimal value
long long quotient = 0;//quotient for bimary
long long decimal;//identifier to be used to find decimal value
string hex_val = "";//total hex value in one string
bool hex = true;//validator for hex calculation

system("color 09");
cout << " Enter the total number of digits: ";
system("color");
cin >> num_limit;//user input
cin.ignore(1000, '\n');
integer_only();//checking for illegal input
cout << endl;//blank line
y = num_limit;//assigning max index value value
for (i = 0; i < num_limit; i++)
{
    system("color 09");
    cout << " Num #" << i + 1 << ": ";
    system("color");
    user_input = _getwche();//user input
    cout << endl;//blank line
    switch (user_input)
    {
    case '0':num = 0; break;
    case '1':num = 1; break;
    }
    while (user_input != '1' && user_input != '0')//checking for illegal input
    {
        system("color 07");
        cout << " Invalid input...try again: ";
        system("color");
        user_input = _getwche();//user input
        cout << endl;//blank line
        switch (user_input)
        {
        case '0':num = 0; break;
        case '1':num = 1; break;
        }
    }
    total_dec = num * pow(2, (y - 1));//y-1 so that calculation is in reverse order
    dec_count += total_dec;
    y--;
}
decimal = dec_count;
while (hex)//will run while <hex> is true
{
    quotient = decimal / 16;
    hex_remainder = decimal % 16;
    decimal = quotient;
    hexassign();//assiging hex value
    hex_val += hex_assign;
    if (quotient == 0) { hex = false; }//hex is false when quotient is 0
}
system("cls");//clearing screen
load();//calling loading effect
cout << endl;//blank line
system("color 06");
cout << " Decimal value =========> ";
system("color");
cout << dec_count << endl;
system("color 06");
cout << " Hexadecimal value =====> ";
system("color");
for (v = 1; v <= hex_val.length(); v++)//printing backwards 
{
    cout << hex_val[hex_val.length() - v];
}
cout << endl;//blank line
}//bit
void hex()//hexadecimal input
{

int k, o;//loop counter
long long decimalvalue = 0;//total decimal value
long long totaldec = 0;//decimal value
string total_bin = "";//all binary values in one string

system("color 09");
cout << " Enter the total number of digits: ";
system("color");
cin >> num_limit;//user input
cin.ignore(1000, '\n');
integer_only();//checking for illegal input
cout << endl;//blank line
o = num_limit;//assigning loop counter its value
for (k = 1; k <= num_limit; k++)
{
    system("color 09");
    cout << " Enter num #" << k << ": ";
    system("color");
    user_inp = toupper(_getwche());//user input
    cout << endl;//blank line

    hex_input_validator();//validating illegal input
    if (userinput)
    {
        hex_int_assign();//assigning a integer value
        hex_binary_assign();//assiging a binary value
    }
    else {
        while (!userinput)
        {
            system("color 04");
            cout << " Invalid input...try again: ";
            system("color");
            user_inp = toupper(_getwche());//user input
            cout << endl;//blank line

            hex_int_assign();//assigning ineger value
            hex_binary_assign();//assiging a binary value
            hex_input_validator();//validating illegal input
        }
    }
    total_bin += bitassign;
    totaldec = hexint * pow(16, (o - 1));//o-1 for answer in reverse order
    decimalvalue += totaldec;
    o--;
}
system("cls");//clearing screen
load();//calling loading effect
cout << endl;//blank line
system("color 06");
cout << " Decimal value ======> ";
system("color");
cout << decimalvalue << endl;
system("color 06");
cout << " Binary value =======> ";
system("color");
cout << total_bin;
cout << endl;//blank line
}//hex
void dec()//decimal input
{
long long bit_quotient;//binary quotient
long long bit_remainder;//binary remainder
long long bit_decimal;//identier to be used with binary calculation with the value of user input
int f, q;//loop counter
long long hex_quotient;//hexadecimal quotient
long long hex_decimal;//identifier to be used with hex calculation with the value of user input
string total_hex_string;//all hex string values in one string
string bit_string;//assigning binary remainder, binary values
string total_bit_string;//all binary values in one string
bool hex = true;//validator for hex calculation
bool bit = true;//validator for binary calculation

system("color 09");
cout << " Enter number: ";
system("color");
cin >> num_limit;//user input
cin.ignore(1000, '\n');
integer_only();//checking for illegal input
bit_decimal = num_limit;
hex_decimal = num_limit;
while (bit)
{
    bit_quotient = bit_decimal / 2;
    bit_remainder = bit_decimal % 2;
    bit_decimal = bit_quotient;
    switch (bit_remainder)
    {
    case 0:bit_string = "0"; break;
    case 1:bit_string = "1"; break;
    }
    total_bit_string += bit_string;
    if (bit_quotient == 0) { bit = false; }
}
while (hex)
{
    hex_quotient = hex_decimal / 16;
    hex_remainder = hex_decimal % 16;
    hex_decimal = hex_quotient;
    hexassign();//assigning a hex value
    total_hex_string += hex_assign;
    if (hex_quotient == 0) { hex = false; }
}
system("cls");//clearing screen
load();//calling loading effect
cout << endl;//blank line
system("color 06");
cout << " Binary value ===========> ";
system("color");
for (f = 1; f <= total_bit_string.length(); f++)//printing in backwards order
{
    cout << total_bit_string[total_bit_string.length() - f];
}
cout << endl;//blank line
system("color 06");
cout << " Hexadecimal value ======> ";
system("color");
for (q = 1; q <= total_hex_string.length(); q++)//printing in backwards order
{
    cout << total_hex_string[total_hex_string.length() - q];
}
cout << endl;//blank line
}//dec
int main()
{
char user_choice;//identifier for user choice
char run_choice;//identifier for run choice
bool run = true;//validator for running the program
while (run)//will run as long as <run> is true
{
    cout << endl;//blank line
    system("color 09");
    cout << " Do you want to input (B)inary, (H)exadecimal, or (D)ecimal value? ";
    system("color");
    user_choice = toupper(_getwche());//user input
    cout << endl;//blank line
    cout << endl;//blank line
    while (user_choice != 'B' && user_choice != 'H' && user_choice != 'D')//checking illegal emtry
    {
        system("color 04");
        cout << " Wrong input...enter again: ";
        system("color");
        user_choice = toupper(_getwche());//user input
        cout << endl;//blank line
    }
    system("cls");
    cout << endl;//blank line
    if (user_choice == 'B') { bit(); }//if user chose binary
    else if (user_choice == 'H') { hex(); }//if user chose hexadecimal
    else if (user_choice == 'D') { dec(); }//if user chose decimal
    cout << endl;//blank line
    system("pause");
    system("cls");//clearing screen
    cout << endl;//blank line
    system("color 09");
    cout << " Do you want to calculate any more numbers?(Y/N): ";
    run_choice = toupper(_getwche());//user input
    cout << endl;//blank line
    cout << endl;//blank line
    while (run_choice != 'Y' && run_choice != 'N')
    {
        system("color 04");
        cout << " Wrong input...enter again: ";
        run_choice = toupper(_getwche());//user input
        cout << endl;//blank line
    }
    if (run_choice == 'Y') { run = true; system("cls"); }
    else if (run_choice == 'N') { run = false; }
}
system("color 07");//colour reset
system("cls");//clearing screen
return 0;
}//end
\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$

using namespace std;:

using namespace std;

This is a bad habit to get into. With larger projects it can cause name collisions. It's better to type std::whatever where necessary.


declaring variables:

long long num_limit;     //user input for decimal
long long hex_remainder; //hexadecimal remainder
long long hexint = 0;    //integer value for user input
char user_inp;           //identifier for user input
bool userinput = true;   //validator for illegal input
string hex_assign;       //assiging hex remainder string value
string bitassign = "";   //identifier assigning binary values to hex

Try to avoid global variables. These should all be local variables, function arguments or return values.

We should:

  • Declare variables as close to the point of use as possible (minimize their scope). Declaring local variables at the start of functions is an outdated and undesirable habit.
  • Initialize variables directly to useful values, not placeholders.
  • Avoid reusing variables (unless a variable requires a hefty resource allocation).

Another example:

void load() //loading effect
{
    int g, c; //loop counter
    cout << " ";
    system("color 07");
    for (g = 1; g <= 80; g++)
    {
        for (c = 1; c <= 2850000; c++)

g and c are not needed outside the loop, and should be declared in the loop statements:

for (int g = 1; g <= 80; ++g)
    ...

And again:

int main()
{
    char user_choice; //identifier for user choice
    char run_choice;  //identifier for run choice
    bool run = true;  //validator for running the program
    while (run)       //will run as long as <run> is true

run is ok here. The other two variables are not in the right place (they aren't used outside the loop, and aren't initialized to useful values).

As a beginner, things you've just learned about may seem comment worthy. However, it's best to ask yourself if the comment actually adds something to the reader's understanding. In this case, they are just restating the code, and could be omitted.


put repetitive code in a separate function:

    cout << endl; //blank line
    system("color 09");
    cout << " Do you want to input (B)inary, (H)exadecimal, or (D)ecimal value? ";
    system("color");
    char user_choice = toupper(_getwche());                                    //user input
    cout << endl;                                                          //blank line
    cout << endl;                                                          //blank line
    while (user_choice != 'B' && user_choice != 'H' && user_choice != 'D') //checking illegal emtry
    {
        system("color 04");
        cout << " Wrong input...enter again: ";
        system("color");
        user_choice = toupper(_getwche()); //user input
        cout << endl;                      //blank line
    }
    system("cls");
    cout << endl; //blank line
    if (user_choice == 'B')
    {
        bit();
    } //if user chose binary
    else if (user_choice == 'H')
    {
        hex();
    } //if user chose hexadecimal
    else if (user_choice == 'D')
    {
        dec();
    }             //if user chose decimal
    cout << endl; //blank line
    system("pause");
    system("cls"); //clearing screen
    cout << endl;  //blank line
    system("color 09");
    cout << " Do you want to calculate any more numbers?(Y/N): ";
    char run_choice = toupper(_getwche()); //user input
    cout << endl;                     //blank line
    cout << endl;                     //blank line
    while (run_choice != 'Y' && run_choice != 'N')
    {
        system("color 04");
        cout << " Wrong input...enter again: ";
        run_choice = toupper(_getwche()); //user input
        cout << endl;                     //blank line
    }
    if (run_choice == 'Y')
    {
        run = true;
        system("cls");
    }
    else if (run_choice == 'N')
    {
        run = false;
    }

A little more vertical spacing would make this more readable.

Note the duplication of code when fetching user input. We could abstract that code into a function so that our main becomes more readable, e.g.:

    char user_choice = get_char_from_user(" Do you want to input (B)inary, (H)exadecimal, or (D)ecimal value? ", { 'B', 'H', 'D' });

    system("cls");
    cout << endl;

    if (user_choice == 'B')
    {
        bit();
    }
    else if (user_choice == 'H')
    {
        hex();
    }
    else if (user_choice == 'D')
    {
        dec();
    }

    cout << endl;
    system("pause");
    system("cls");

    char run_choice = get_char_from_user(" Do you want to calculate any more numbers?(Y/N): ", { 'Y', 'N' });

    if (run_choice == 'Y')
    {
        run = true;
        system("cls");
    }
    else if (run_choice == 'N')
    {
        run = false;
    }

get_char_from_user() might look something like:

bool vector_contains(std::vector<char> vec, char item)
{
    for (auto c : vec)
        if (c == item)
            return true;
    
    return false;
}

char get_char_from_user(std::string prompt, std::vector<char> valid_choices)
{
    cout << endl;
    system("color 09");
    cout << prompt;

    while (true)
    {
        system("color");
        char user_choice = toupper(_getwche());
        cout << endl;

        if (vector_contains(valid_choices, user_choice))
            return user_choice;

        system("color 04");
        cout << " Wrong input...enter again: ";
    }
}

bit():

char user_input = 'x';   //user input (1/0)
int num;                 //identifier assigned integer value for user input
int i, v, y;             //loop counter
long long total_dec;     //decimal value
long long dec_count = 0; //total decimal value
long long quotient = 0;  //quotient for bimary
long long decimal;       //identifier to be used to find decimal value
string hex_val = "";     //total hex value in one string
bool hex = true;         //validator for hex calculation

Note the advice about variable declaration above.

system("color 09");
cout << " Enter the total number of digits: ";
system("color");
cin >> num_limit; //user input
cin.ignore(1000, '\n');

integer_only(); //checking for illegal input
cout << endl;   //blank line

We should avoid using the global variable num_limit. Again, we move this code into a separate function so that we only have to do:

int y = get_integer_from_user(" Enter the total number of digits: ")

Removing global variables (using function arguments and return values) and moving the input / printing code into separate functions we would get something a lot more readable:

void bit() //binary input
{
    int y = get_integer_from_user(" Enter the total number of digits: ");

    long long dec_count = 0;
    for (int i = 0; i < num_limit; i++)
    {
        int num = get_binary_digit_from_user(" Num #" + std::to_string(i + 1) + ": ");

        long long total_dec = num * pow(2, (y - 1)); //y-1 so that calculation is in reverse order
        dec_count += total_dec;
        y--;
    }

    string hex_val;
    long long decimal = dec_count;

    while (decimal)
    {
        long long quotient = decimal / 16;
        long long hex_remainder = decimal % 16;
        hex_val += hexassign(hex_remainder);
        
        decimal = quotient;
    }

    print_dec_result(dec_count);
    print_hex_result(hex_val);
}

(Note: not compiled / tested).

We should generally ensure that functions have only a single responsibility, so this could be split up further:

void bit() // binary input
{
    std::string bin = get_binary_string_from_user(); // (read whole string before converting to decimal)
    long long dec = binary_string_to_decimal(binary);
    std::string hex = decimal_to_hex_string(decimal);

    print_decimal(dec);
    print_hex_string(hex);
}

The hex() and dec() functions have similar issues, so I won't look at them specifically.


hexassign() and switch statements:

As mentioned, hexassign and the other similar functions should use function arguments (parameters) and return values, not global variables.

The switch statements are also not ideal. We have lots of other choices here, e.g.:

  • Look up tables - finding an index or element in a std::vector or std::map is quite simple.

  • std::printf("%x", ...) or the modern equivalent stringstream << std::hex << ... for printing to a hex string.

  • We could also use manual character ranges (if we're careful) to do conversions: return c < 10 ? '0' + c : 'A' + (c - 10);.

I'd suggest experimenting with a few different ways to see what's shortest, easiest and safest. It's worth thinking about what happens (and what we want to happen) when we pass invalid inputs into each function. assert(condition) or if (!condition) throw std::runtime_error(); might be useful...

\$\endgroup\$
1
  • 1
    \$\begingroup\$ While 'A' + (c - 10) does achieve the desired results in the common character codings, C++ doesn't guarantee that the letters are consecutive. A lookup such as "0123456789ABCDEF"[c] is portable, even for pedants. \$\endgroup\$ Commented Nov 25, 2021 at 11:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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