6
\$\begingroup\$

I'm still new to C programming, but I'm playing around with it to ensure I progress. I just made the following converter that takes 8 numbers (0 and 1) as binary code then converts the binary value to decimal and hex values. Below is the full code.

Aside from perhaps adding loops to make my code less redundant (especially the printf() statements), is there something I could do to make my code look more professional.

#include <stdio.h>
#include <math.h>

int main() {
int b1, b2, b3, b4, b5, b6, b7, b8, decimal, hex1, hex2;
char convert1, convert2;

printf("Please enter the 1st bit of your binary number (0 or 1): ");
scanf("%i", &b1);

printf("Please enter the 2nd bit of your binary number (0 or 1) ");
scanf("%i", &b2);

printf("Please enter the 3rd bit of your binary number (0 or 1) ");
scanf("%i", &b3);

printf("Please enter the 4th bit of your binary number (0 or 1) ");
scanf("%i", &b4);

printf("Please enter the 5th bit of your binary number (0 or 1) ");
scanf("%i", &b5);

printf("Please enter the 6th bit of your binary number (0 or 1) ");
scanf("%i", &b6);

printf("Please enter the 7th bit of your binary number (0 or 1) ");
scanf("%i", &b7);

printf("Please enter the 8th bit of your binary number (0 or 1) ");
scanf("%i", &b8);

decimal = b1 * pow(2,7) + b2 * pow(2,6) + b3 * pow(2,5) + b4 * pow(2,4) + b5 * pow(2,3) + b6 * pow(2,2) + b7 * pow(2,1) + b8 * pow(2,0);

hex1 = b1 * 8 + b2 * 4 + b3 * 2 + b4 * 1;
hex2 = b5 * 8 + b6 * 4 + b7 * 2 + b8 * 1;

switch(hex1) {
case 10:
    convert1 = 'A';
    break;
case 11:
    convert1 = 'B';
    break;
case 12:
    convert1 = 'C';
    break;
case 13:
    convert1 = 'D';
    break;
case 14:
    convert1 = 'E';
    break;
case 15:
    convert1 = 'F';
    break;
}

switch(hex2) {
case 10:
    convert2 = 'A';
    break;
case 11:
    convert2 = 'B';
    break;
case 12:
    convert2 = 'C';
    break;
case 13:
    convert2 = 'D';
    break;
case 14:
    convert2 = 'E';
    break;
case 15:
    convert2 = 'F';
    break;
}

printf("\nThe decimal value of your binary number is %i\n\n", decimal);
printf("The hexadecimal value of your binary number is: ");
if(hex1 >= 10 && hex2 < 10) {
    printf("%c%i\n", convert1, hex2);
}
else if (hex1 <10 && hex2 >= 10) {
    printf("%i%c\n", hex1, convert2);
}
else if (hex1 >= 10 && hex2 >=10) {
    printf("%c%c\n", convert1, convert2);
}
else {
    printf("%i%i\n", hex1, hex2);
}


return 0;
}
\$\endgroup\$
5
\$\begingroup\$

First things first, props to you for learning good old C in the day of dynamically typed high level languages! Now on to the review...

Error checking

Right now, there's nothing to stop me from entering 512351 as the seventh digit, for example. As a rule of thumb, if something comes from user input, double-check it. Your user is the greatest threat to program integrity. For example, check the input in a loop and only save it once it is a valid bit (0 or 1), if it's not valid, simply ask your user again. But this takes me to the next issue...

Input

Right now, you ask for each bit separately. I'm lazy. I wouldn't want to have 16 keystrokes to enter 8 digits. So, instead of asking for each bit on its own, consider just asking your user to type the digits as a string. Instead of eight integer variables, you could store them (temporarily, that is) in an array of characters. See the following snippet:

char input[8 + 1 + 1]; // 8 characters + newline + null terminator

printf("Enter a binary number: ");
fgets(input, sizeof input, stdin);
input[8] = '\0'; // We don't want the newline

This reads a line from standard input and stores the first 9 characters - 8 bits and a newline - in the array input, as well as a '\0' to terminate the string.

Now this is way shorter!

Conversion to decimal

But now that you have your digits as characters in a array, the conversion has to change. You can even be fancy and make your conversion more generic, in case you want to handle arbitrarily long numbers one day.

A typical idiom to get the numeric value from a character is int number = char - '0'; This subtracts the ASCII value for the character 0 from the character you have, leaving for example '5' as the integer 5. This will get strange when you use non-digit characters, of course. But you've checked that beforehand, I hope!

What's left is looping over your characters and adding their digit values to the total value.

int decimal_value = 0;

for (int i = 0; i != 8; ++i) {
    int digit = input[i] - '0';
    int factor = pow(2, 7 - i);

    decimal_value += digit * factor;
}

But since your binary digits are always either 0 or 1, you could tweak this a bit to prevent adding 0 a bunch of times.

int decimal_value = 0;

for (int i = 0; i != 8; ++i) {
    if (input[i] - '0') {
        decimal_value += pow(2, 7 - i);
    }
}

If the current character processed is '0', input[i] - '0' will evaluate to 0, which is falsy - the body of the if will not be executed. Otherwise, it is non-zero, which leads to the if-body being executed. Nice!

Conversion to hex

Right now, you do two things which should not be done together. You store the hex-digits as integer value, but once they're larger than 10 you store a character instead. This... works, but it's unusual and not that great. Instead, convert both two characters! The actual conversion is left as an exercise for you, but once you have both digits stored as first_hexdigit and last_hexdigit for example, you could to this:

char hex[2]; // Since we have two digits, let's store those as an array.
             // Note that, in contrast to before with input, we do not
             // have to make the array one larger than it needs to be,
             // as we do not use fgets which appends an extra character.

hex[0] = first_hexdigit >= 10 ? first_hexdigit - 10 + 'A' 
                              : first_hexdigit + '0';
hex[1] = last_hexdigit >= 10 ? last_hexdigit - 10 + 'A' 
                             : last_hexdigit + '0';

If you have never used it before, the ? and : are the ternary operator. Basically, a ? b : c means: if a is a truthy value, then return b, else c. Or, in other words:

int result;    
if (a) {
    result = b;
} else {
    result = c;
}

To convert from a numeric value to a character, we switch our previous idiom around a bit: instead of subtracting our lowes character, we add it as value. If last_hexdigit is 6, for example, this becomes 6 + '0' which is just '6'. But if it's 12, the computation is 12 - 10 + 'A', which is 2 + 'A' which is 'C'.

Output

Since you've stored your results a bit differently, your way of printing has to be changed as well!

Assume we have our decimal value stored, as integer, in decimal_value and our hexadecimal as array of two characters in hexadecimal_value.

int decimal_value; // Your decimal
char hexadecimal_value[2]; // Your hexadecimal

printf("Decimal: %d\n", decimal_value);
printf("Hexadecimal: %c%c\n", hexadecimal_value[0], hexadecimal_value[1]);

This could made better a bit if you do make the hexadecimal_value array one larger than it needs to be and set the last character to '\0'. Because then, hexadecimal_value would be a string (as strings need to be null-terminated), which you could easily print with printf:

printf("Hexadecimal: %s\n", hexadecimal_value);

Encapsulation

Now that the actual code is cleaned up a bit, what can we do to make it more beautiful on a higher level?

Exctract functionality into functions, of course!

Right now, all your code is in main. This is okay for small programs like this, but should still be avoided. So instead, let's make some smaller functions for simple tasks! Your control flow is much like the following:

int main() {
    get_input();
    process_input();
    print_output();
}

Since it makes sense to handle the input and output in main (for this scale at least), let's focus on making the process_input()! I suggest you use three functions, which carry your main logic:

int   is_valid_binary(const char *string);
char *binary_to_decimal(const char *string);
char *binary_to_hexadecimal(const char*string);

is_valid_binary takes in a string and returns 0 if it is not a valid binary number (e.g. contains characters other than '1' and '0'), otherwise, it returns 1. binary_to_decimal takes in a string of binary digits and returns a string of the very same number, but with decimal digits. binary_to_hexadecimal works similar; it takes a string and returns a string of hexadecimal digits. I leave actually writing these functions to you.

Once you have those three functions, your main would thus become:

int main() {
    char input[8 + 1 + 1];
    printf("Enter a binary number: ");

    do {
        fgets(input, sizeof input, stdin);
        input[8] = '\0';
    } while (!is_valid_binary(input));

    char *decimal = binary_to_decimal(input);
    char *hexadecimal = binary_to_hexadecimal(input);

    printf("Decimal: %s\nHexadecimal: %s\n", decimal, hexadecimal);

    // Since you allocated the memory for decimal end hexadecimal yourself,
    // you have to free it yourself as well! At least, you should allocate
    // it yourself.
    free(decimal);
    free(hexadecimal);
}

Exercise to you

To recap, consider the final version of main I pasted above. Your task now is to write the functions is_valid_binary, binary_to_decimal and binary_to_hexadecimal. You could also expand those functions to handle arbitrarily long binary numbers (not necessarily eight digits, but also 2 digits or 24 digits!). And, as an extra, take the input from the commandline. Use argv for that one.

Addendum

As Toby Speight mentioned in the comments, it is not really a good idea to use pow() when calculating 2^n. First of all, pow() uses doubles, which adds some conversion overhead (and, worst case scenario, wrong results due to the way double works. But it's rather unlikely.), and second: it's slow. Let me introduce you to bitmagic!

Consider the number 4, or 0b0100. 4 times 2 is 8, or 0b1000. There is a pattern going on here which is everywhere: if you multiply any number by 2, its binary representation gets shifted by 1 to the left. Since exponention is just repeated multiplication, 2^7 is just 1 * 2 * 2 * 2 * 2 * 2 * 2 * 2. That's seven left-shifts right there! In C, performing left-shifts is done by using the << operator.

So, instead of calculating 2^7 by using the slow pow(), you can use 1 << 7 instead!

Basically: pow(2,x) == 1 << x.

\$\endgroup\$
  • 1
    \$\begingroup\$ Good review - you might want to make it even better by mentioning something about the (in)advisability of pow() for raising integers to small integer powers (and demonstrate << as an alternative for the particular case of 2^n). \$\endgroup\$ – Toby Speight Sep 19 '17 at 17:33
  • \$\begingroup\$ Good point about using bitshifts instead of pow, didn't even think about that. \$\endgroup\$ – Phil Kiener Sep 19 '17 at 17:38
  • \$\begingroup\$ "fgets(input, 8, stdin); This reads 8 characters from standard input and stores them in the array input," is quite wrong. Suggest char input[8+1+1]; ... fgets(input, sizeof input, stdin); to read 8 digits, a '\n' and room for the appended null character. Or why be stingy? Use a 2x buffer char input[20]; \$\endgroup\$ – chux Sep 20 '17 at 18:30
  • \$\begingroup\$ You're right, that should've been fgets(input, 9, stdin) to store eight characters and the null terminator. After all, that's how large the buffer is - should've used sizeof right away. But we don't need char input[8+1+1]; - no need to store the newline. \$\endgroup\$ – Phil Kiener Sep 20 '17 at 19:01
  • \$\begingroup\$ @PhilKiener True no need to store the '\n', but if it is not read, it remains in stdin for the next input function - important should code expand. Better to read a line of user input here and lop off the '\n' from the buffer if needed. \$\endgroup\$ – chux Sep 20 '17 at 20:10

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.