Program to convert binary to hex

Question

I wrote this program to convert binary to hex, it ignores other characters between the binary digits. I would like to receive any recommendations on how to improve it.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

const char table[] = "0123456789abcdef";

int main(int argc, char* argv[])
{
  int flip = argc - 2;
  unsigned char* ubin = argv[1];
  unsigned int nibble = 0;
  int i = 0;
  int nothing = 1;

  if((argc != 2) && (argc != 3 || strcmp(argv[2], "-f") != 0)){
    fprintf(stderr, "Usage: %s 00001111 [-f]\n", argv[0]);
    return EXIT_FAILURE;
  }

  printf("%s", "0x");

  while(*ubin){
    if((*ubin != '0') && (*ubin != '1')){
      ++ubin;
      continue;
    }

    nibble <<= 1;
    nibble |= *ubin - '0';

    if(++i == 4){
      i = 0;
      if(flip)
        nibble = ~nibble & 0xf;
      putchar(table[nibble]);
      nibble = 0;
      nothing = 0;
    }

    ++ubin;    
  }

  if(nothing)
    putchar('0');

  putchar('\n');

  if(i)
    fprintf(stderr, "Error: %d bits were discarded, min unit is a nibble\n", i);

  return 0;
}

Answer 1

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

const char table[] = "0123456789abcdef";

This constant should be static const char table[]. Just as a general rule, so that it doesn’t conflict with other variables in other files that are also called table.

int main(int argc, char* argv[])
{
  int flip = argc - 2;
  unsigned char* ubin = argv[1];

Using argc and argv before range checking them is dangerous in general. In this case it works, but it means that ubin could be NULL here. This code looks suspicious.

  unsigned int nibble = 0;
  int i = 0;

Bad variable name i. It should rather be nbits, since it contains the number of bits in the current nibble.

  int nothing = 1;

  if((argc != 2) && (argc != 3 || strcmp(argv[2], "-f") != 0)){
    fprintf(stderr, "Usage: %s 00001111 [-f]\n", argv[0]);
    return EXIT_FAILURE;
  }

  printf("%s", "0x");

A simple printf("0x") would be clearer.

  while(*ubin){

Instead of this while loop, write a for loop: for (ubin = argv[1]; *ubin != '\0'; ubin++) {. Then you don’t need to write the ubin++ multiple times.

    if((*ubin != '0') && (*ubin != '1')){
      ++ubin;
      continue;
    }

    nibble <<= 1;
    nibble |= *ubin - '0';

    if(++i == 4){
      i = 0;
      if(flip)
        nibble = ~nibble & 0xf;

An alternative would be nibble ^= 0xf;.

      putchar(table[nibble]);
      nibble = 0;
      nothing = 0;
    }

    ++ubin;    
  }

  if(nothing)
    putchar('0');

  putchar('\n');

  if(i)
    fprintf(stderr, "Error: %d bits were discarded, min unit is a nibble\n", i);

In case of an error, the return value should be EXIT_FAILURE, not 0.

  return 0;
}

But apart from all these comments, the program looks fine. You did the argument checking correctly and strictly. When your program gets more options, you should consider using the getopt library for parsing the command line. But for now, it is fine.

It is unusual to have the options after the remaining arguments; -f 00001111 is more common than 00001111 -f.

The main algorithm is clean and simple. I especially like the error handling at the end. There are too many other programs that just don’t do this.