Convert from decimal to binary and octal

Question

I wrote a program that implements an algorithm that converts from decimal to binary and octal

Please criticize my implementation. What else can I do to improve?

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

int n, option, flagOctal;
int a[100];

void  baseConversion10Base2(long long x){
    int D = 0;
    while(x){
        a[++D] = abs(x % 2);
        x /= 2;
    }
    for(int i = D; i > 0; i--)
        printf("%d", a[i]);
}


void baseConversion10Base8(long long nrDecimal)
{
    int D = 0;
    while(nrDecimal){
        a[++D] = abs(nrDecimal % 8);
        nrDecimal /= 8;
    }
      for(int i = D; i > 0; i--)
      printf("%d", a[i]);
}

int main(int argc, char **argv)
{

    if(argc == 1)
    {
        printf("No arguments were entered");
    return 0;
    }
    else
    if(argc == optind)
    {
    printf("Only optional parameters have been entered");
    return 0;
    }
     else
    {
        opterr = 0;
        while((option = getopt(argc, argv, "o")) != -1){
        switch(option){
        case 'o':
            flagOctal = 1;
            break;
        default:
            ;
        }
        }
    if(optind == argc)
    {
    printf("Only optional parameters have been entered");
    return 0;
    }

    printf("Positive numbers converted to ");
    if(flagOctal == 1) printf("octal: "); else printf("binar: ");
    for(int i = optind; i < argc; i++)
  {
    if(strcmp(argv[i], "0") == 0)
    {
    printf("0");
    continue;
    }
    char *a = argv[i];
    long long nr = atol(a);
    if(nr == 0)
    printf("X ");
    else
    {
    if(flagOctal == 1)
    {
        baseConversion10Base8(nr);
        printf(" ");
    }
    else
    {
        baseConversion10Base2(nr);
        printf(" ");
    }
    }
  }

    printf("\nNegative numbers converted to ");
    if(flagOctal == 1) printf("octal: "); else printf("binar: ");
    for(int i = 1; i < optind; i++)
    {
    char *a = argv[i];
    long long nr = atol(a);
    if(nr < 0)
    {
    printf("-");
    if(flagOctal == 1)
        baseConversion10Base8(abs(nr));
    else
        baseConversion10Base2(abs(nr));
    printf(" ");
    }
    }   


 }
    printf("\n");
    return 0;
}

Answer 1

The global variables make it harder to reuse your code. They seem to be unnecessary anyway: the integer variables are used only in main(), and a could be scoped into the baseConversion functions.

Depending on target platform, the size of a might not be large enough to represent the binary value of a long long, which can have CHAR_BIT * sizeof (long long) bits.

The conversion functions mix computation and output, again making re-use more difficult (and baseConversion10Base8 could be replaced with a simple printf("%llo", …)).

We're using getopt(), so should be including <unistd.h> rather than <getopt.h> (which declares getopt_long() and getopt_long_only()).

Since optind is initialised to 1, the test if (argc == optind) immediately following if (argc == 1) can never be true.

We don't need else when the preceding block leaves the function.

The switch should include a case for getopt() returning '?' when the user provides an unrecognised option. The default case that does nothing can be removed.

atol() is a poor choice for converting string input to integer, as it doesn't distinguish between failure and an actual zero value. Prefer strtoll instead.

There's a value truncation hidden here:

   baseConversion10Base8(abs(nr));

abs(nr) will truncate its argument to int. We should be using llabs() to correctly convert a long long. Note that there's a failure case on 2's-complement systems when the input is LLONG_MIN, as -LLONG_MIN exceeds LLONG_MAX.

Instead of flag_octal, we could just store a pointer to the function to use:

    const char *base_name = "binary";
    void (*conversion_function)(long long int) = baseConversion10Base2;

        case 'o':
            base_name = "octal";
            conversion_function = baseConversion10Base8;

    printf("Positive numbers converted to %s: ", base_name);

    printf("-");
    conversion_function(llabs(nr));
    printf(" ");

printf() is somewhat heavyweight for printing single characters or fixed strings - prefer putchar() and fputs() respectively.

"Binary" is misspelt as "binar" in two of the printed messages.

Answer 2

• The code presented could be more readable.
  • Starting with comments telling what "everything" is there for.
  • Naming: integers/numbers are not decimal.
    Representations are, unless specified otherwise.
  • Code layout:
    The indentation is inconsistent
    After a conditional return, there is no need for an else.
    Conventionally, else if is kept together, and the if-(else-)statement is not indented another level.
• Don't Repeat Yourself.

    /** terminate program execution with the specified exit code
     *  and a message to out */
    void terminate(int exit_code, char const *message, FILE *out)
    {
        if (NULL != message && NULL != out) {
            fputs(message, out);
            fputc('\n', out);
        }
        exit(exit_code);
    }
    /** leave the program with a message to stdout */
    void leave(char const *message) {
        terminate(0, message, stdout);
    }
        
    /** print x as an (unsigned) number base 2, 
     *  most significant digit first. */
    void print_base2(long long x) {
        print_base(x, 2);
    }
    /** print x as an (unsigned) number base 8, 
     *  most significant digit first. */
    void print_base8(long long x) {
        print_base(x, 8);
    }
    /** print x as an (unsigned) number base base, 
     *  most significant digit first. */
    void print_base(unsigned long long x, int base) {
        if (base < 2 || 10 < base)  // CHAR_MAX? 10 + 'Z' - 'A' + 1?
            leave("base out of range");
        char a[CHAR_BIT * sizeof (long long)];  // ample for 2 < base
        int n = convert2base(x, base, a, sizeof a);
        if (n < 0)
            leave("conversion failed unexpectedly");
        while (0 <= --n)
            putchar('0' + a[n]);
    }

    /** store digits base base for n in digits, least significant digit first.
     *  return number of digits if length is sufficient, else -1. */
    int convert2base(unsigned long long n, int base, char digits[], int length)
    {
        if (NULL == digits || length <= 0)
            return -1;
        int d = 0;  // digit
        digits[0] = 0;
        while (n && d < length) {
            digits[d++] = n % base;
            n /= base;
        }
        if (0 != n)
            return -1;
        return d;
    }

Answer 3

This may give you computational boost. Although as pointed out by others in comment this may be "The" code produced by modern day compilers.

while(x){
    a[++D] = abs(x % 2);
    x /= 2;
}

can be written as

while(x){
    a[++D] = x&1;
    x = x>>1;
}

And writing

while(nrDecimal){
    a[++D] = abs(nrDecimal % 8);
    nrDecimal /= 8;
}

as

while(nrDecimal){
    a[++D] =  !(nrDecimal & 7)
    nrDecimal=nrDecimal>>3;
}