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;
}

• vnp: What is conversieBaza10Baza8?
• Mike: sorry, i edited now

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.

• greybeard: CHAR_BIT * sizeof (long long) may be too small, as index 0 is not used. Then again, with one bit used for sign...
• Toby Speight: @greybeard, I missed that we never write to index 0 - thanks for spotting. Yes, using only positive numbers dodges that particular bullet, but I thought it wise to re-word anyway.

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;
 }

• greybeard: My C is beyond rusty. The alternative presented doesn't show much about layout.

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;
}

• Toby Speight: "Definitely"? Don't be so sure: that's the kind of transformation that any decent compiler will perform. I'd expect performance to be identical (on a 2's complement system, anyway - it would be incorrect elsewhere).
• pacmaninbw: Welcome to the Code Review Community. While this might be a good answer on stackoverflow, it is not a good answer on Code Review. A good answer on Code Review contains at least one insightful observation about the code. Alternate code only solutions are considered poor answers and may be down voted or deleted by the community. Please read How do I write a good answer.
• greybeard: What is the logical not of nrDecimal & 7?
• mdfst13: Another problem with this kind of approach is that sometimes it hides an even better compiler optimization. Because compilers tend to assume that bit fiddling operations are already optimal, so they don't analyze them the same way that they do mathematical operations. I'm not saying to never attempt this kind of optimization, but one should only do it on code that definitely needs optimized and should follow up with metrics that test both approaches.
• AnotherDeveloper: @TobySpeight : I agree. Should have given more thought for all systems as well. It appears to be super confidant. Shall edit