converting bits binary to bcd

Question

I wrote a code that requests user input and then converts binary to bcd then back to binary using bitwise operations. While I think my code is good I want to know if there is a better way to write the code. Is there also a way to keep the code mostly the same but getting rid of the arrays for printing out the bit positions?

#include <stdio.h>
#include <limits.h>
#include <stdlib.h>
#include <string.h>
#define UINT_MAXIMUM 4294967294
#define MAX_NUMBER 99999999
void printBits(unsigned long number);
unsigned long convertBinaryToBCD(unsigned long number);
unsigned long convertBCDToBinary(unsigned long number);
unsigned long printNumber(unsigned long input)
{
 printf(" Input: ");
 printf("%10lu", input);
 printBits(input);
 unsigned long output_bcd = convertBinaryToBCD(input);
 unsigned long output_binary = convertBCDToBinary(output_bcd);
 printf(" BCD: ");
 if(output_bcd != UINT_MAX)
 {
 printf("%10lu", output_bcd);
 printBits(output_bcd);
 }
 printf("Binary: ");
 if(output_binary != UINT_MAX)
 {
 printf("%10lu", output_binary);
 printBits(output_binary);
 }
 return 0;
}
void printBits(unsigned long number)
{
 int bit_position[32];
 printf(" - ");
 int counter = 0;
 for(counter = 32; counter > 0; counter--)
 {
 bit_position[counter] = number % 2;
 number = number / 2;
 }
 for(counter = 1; counter <= 32; counter++)
 {
 if(counter == 9 || counter == 17 || counter == 25)
 {
 printf(" | ");
 }
 if(counter == 5 || counter == 13 || counter == 21 || counter == 29)
 {
 printf(" ");
 }
 printf("%d", bit_position[counter]);
 }
 printf("\n");
}
int main(int argc, char *argv[])
{
 unsigned long number;
 number = strtoul(argv[1], NULL, 10);
 if(number > UINT_MAXIMUM)
 {
 printf("too big\n");
 return 0;
 }
 printNumber(number);
 return 0;
}
unsigned long convertBinaryToBCD(unsigned long number)
{
 if (number > MAX_NUMBER)
 {
 return UINT_MAX;
 }
 if(number < 100)
 {
 return (((number/10)+((number/100)*6))*16)+(number%10);
 }
 unsigned long sub_number_one = ((number / 12) << 5);
 unsigned long sub_number_two = (number % 10) ;
 unsigned long temporary;
 while(sub_number_two != 0)
 {
 temporary = sub_number_one & sub_number_two;
 sub_number_one = sub_number_one ^ sub_number_two;
 sub_number_two = temporary << 1;
 }
 return sub_number_one;
}
unsigned long convertBCDToBinary(unsigned long number)
{
 if (number == UINT_MAX)
 {
 return UINT_MAX;
 }
 if(number < 100)
 {
 return (((number >> 8) * 100) + ((number >> 4) * 10) + (number & 0xF));
 }
 unsigned long sub_number_one = (number >> 8) * 100;
 unsigned long sub_number_two = (number & 0x0F);
 unsigned long temporary;
 while(sub_number_two != 0)
 {
 temporary = sub_number_one & sub_number_two;
 sub_number_one = sub_number_one ^ sub_number_two;
 sub_number_two = temporary << 1;
 }
 return sub_number_one;
}

• 1201ProgramAlarm: You have an off-by-one error in printBits. C array indexing starts at 0, so valid subscripts to bit_position are 0..31.

Answer 1

It's been a long time since I've heard the terms BCD or Binary Coded Decimal. Keep in mind that BCD was primarily a hardware encoding of integers provided by one particular manufacturer of main frame computers. There were computer performance issues when using BCD.

The best way to implement a BCD number would be an array of 4 bit integer values.

One good habit the code demonstrates is that all if statements and loops wraps the code in braces ({ and }). Thank you for that!

Magic Numbers

There are Magic Numbers in the void printBits(unsigned long number) function (32, 9, 17, 25, 5, 13, 21 and 29), it might be better to create symbolic constants for them to make the code more readable and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.

Unused Header Files

The program includes limits.h and string.h but the contents of these files are not used by the code in the program. In the case of string.h it might be better to remove the #include. In the case of limits.h it might be better to use the limits provided. It is better to not include files that aren't being used since this will decrease the size of the code being compiled and might improve build times. The #include directive literally copies the header file into the intermediate file that is compiled after the C pre-processor is run.

Algorithm

It might be better to prompt the user for the number to be converted rather than use argv as the source of the input. Even if argv continues to be the source of the input, it would be better to check argc to see if there are enough arguments and either prompt the user for a number in screen input, or report an error and quit.

Readability

The some of the functions (printBits and convertBinaryToBCD) might be easier to read if there was vertical spacing in the functions.

Off by One Bug

As noted by @1201ProgramAlarm there is an off by one bug in void printBits(unsigned long number), the code is accessing one invalid memory location bit_position[32] and ignoring bit_position[0].

It is also not clear why bit_position is 32 bits rather than 64 bits, since the code is primarily using unsigned long rather than unsigned int.

• Toby Speight: Not just in mainframes - BCD is very useful in embedded systems (e.g. counting with 7-segment displays). I guess that's why Z80 has pretty good BCD support.
• pacmaninbw: @TobySpeight I was thinking of IBM main frames. Didn't know it was used in embedded, but it makes sense for LED displays.
• Roland Illig: Do you really think these numbers are magic? I find them obvious since they all differ by 8, and in a function called printBits it makes sense to add some grouping.
• pacmaninbw: @RolandIllig then it could be written in terms of modulo 4 and modulo 8 rather than using hard coded constants. The off by one bug is also very clear in the constants.
• Roland Illig: Do you mean BITS_PER_NIBBLE and BITS_PER_OCTET instead of 4 and 8? ;) Or are they not magic?

Answer 2

unsigned vs, unsigned long

Code mixes use of unsigned and unsigned long as if they are of the 32-bit size.

In 2020, 16-bit unsigned is commonly found in embedded processors and 64-bit unsigned long in 64-bit processors.

Do not assume unsigned, unsigned long size/range other than they are at least 16, 32 bits.

#define UINT_MAXIMUM 4294967294 // UINT_MAX may only be 65535
printf("%10lu", input); // Largest `unsigned long` could be 19+ digits.
unsigned long output_binary;
if(output_binary != UINT_MAX) // output_binary could well exceed `UINT_MAX
unsigned long convertBinaryToBCD(unsigned long number) {
 ...
 return UINT_MAX; // Why not ULONG_MAX?

In particular, the below is not portable as it assumes an unsigned long is 32 bits.

int bit_position[32];

Questionable code

((number >> 8) * 100) is always zero.

if(number < 100) {
 return (((number >> 8) * 100) + ((number >> 4) * 10) + (number & 0xF));
}

Use math

Rather than enumerate a list that assume 32-bit integer, use a rule

// if(counter == 5 || counter == 13 || counter == 21 || counter == 29)
if (counter%8 == 5)