4
\$\begingroup\$

I've made a very simple program that takes integer number arguments either with plus/minus sign or without them, and print the number in binary format.

binary.c

#include <stdio.h>

void printBinary(int);
int toInt(int*, char[]);
int handleProgram(char[]);

int main(int argc, char *argv[])
{   
    return handleProgram(argv[1]);
}

int handleProgram(char param[])
{
    int number, state;

    if( toInt( &number, param ) != -1 )
    {
        printBinary(number);

        state = 0;
    }
    else
    {
        printf("\tmain()::Exiting program...\n");

        state = -1;
    }

    return state;
}


void printBinary(int number)
{
    for (int i = 31; i >= 0; i--)
    {
        if (number >> i & 1 == 1)
            putchar('1');
        else
            putchar('0');

        if (i != 0)
        {
            if (i % 4 == 0)
                putchar(' ');

            if (i % 16 == 0)
                printf(": ");
        }
    }

    printf("\n");
}

int toInt(int *num, char param[])
{
    *num = 0;
    int sign = 1;
    int i = 0;

    if(param == NULL)
    {
        printf("\ttoInt()::No parameter, please enter a valid number!\n");

        return -1;
    }

    if(param[i] == '-')
    {
        sign = -1;
        i++;
    }
    else if( param[i] == '+' )
    {
        i++;
    }

    // check that a number exist
    if(param[i] == '\0')
    {
        printf("\ttoInt()::No number detected, please enter a valid number\n");

        return -1;
    }

    for(; param[i] != '\0'; i++)
    {
        if(param[i] >= '0' && param[i] <= '9')
            *num = 10* *num + param[i] - '0';
        else
        {
            printf("\ttoInt()::Invalid parameter, please enter a valid number!\n");

            return -1;
        }
    }

    *num *= sign;

    return 0;
}

Sample output: positive and negative cases

D:\C\chapter2>gcc binary.c -o binary.exe

D:\C\chapter2>binary 3
0000 0000 0000 0000 : 0000 0000 0000 0011

D:\C\chapter2>binary +3
0000 0000 0000 0000 : 0000 0000 0000 0011

D:\C\chapter2>binary -3
1111 1111 1111 1111 : 1111 1111 1111 1101

D:\C\chapter2>binary +3s
        toInt()::Invalid parameter, please enter a valid number!
        main()::Exiting program...

D:\C\chapter2>binary -3s
        toInt()::Invalid parameter, please enter a valid number!
        main()::Exiting program...

D:\C\chapter2>binary s
        toInt()::Invalid parameter, please enter a valid number!
        main()::Exiting program...

D:\C\chapter2>binary +
        toInt()::No number detected, please enter a valid number
        main()::Exiting program...

D:\C\chapter2>binary -
        toInt()::No number detected, please enter a valid number
        main()::Exiting program...

D:\C\chapter2>binary
        toInt()::No parameter, please enter a valid number!
        main()::Exiting program...

Any ideas for a better approach (where to fix, what to do about negative cases, any more functions need)?

\$\endgroup\$
  • 2
    \$\begingroup\$ If your aim is writing elegant code making use of existing standard libraries, then the function strtol (in stdlib) is the way to go (the man page includes a comprehensive example on how to use it), and can reduce your code by half. Even though your code doesn't require IO performance, it's good practice using a buffer for constructing your output and print only once rather than on multiple little chunks. \$\endgroup\$ – Ziggy Crueltyfree Zeitgeister Apr 22 '16 at 5:39
  • \$\begingroup\$ I want to build up everything like reinventing the wheel. That's why I didn't used stdlib functions. I've used to code in C in the field however after 3 years, I want to go deep to be a better programmer thus started from the page no:1 of the Kernighan's book. \$\endgroup\$ – Levent Divilioglu Apr 22 '16 at 15:15
2
\$\begingroup\$

Functionality

  1. Why 31? Why not 15 or 63 or 42? If code is to handle 32-bit integers, then use long as that type is at least 32-bit, rather than int, which can be as small as 16. Alternatives int32_t, int_least32_t. Even better: re-code for the largest integer type intmax_t.

    // Why 31?
    for (int i = 31; i >= 0; i--)
    
  2. Right shifting a negative number leads to implementation defined behavior. Consider unsigned instead.

    // ... (int number)
    ... (unsigned number)
    ... 
    if (number >> i & 1 == 1)
    
  3. Good that code allows a leading '+'.

  4. Pedantic point. int overflow is undefined behavior and that is the result of the following with argv[1] == "-2147483648", a string that should "work".

    *num = 10* *num + param[i] - '0';
    
  5. Avoid using printf(some_string). Should some_string contain a %, maybe due to a code update, that will invoke UB as printf() expects the first arg to be a format string.

    // printf(": ");
    fputs(": ", stdout);
    // or (2nd choice)
    printf("%s", ": ");
    
  6. Do not use argv[1] unless it is valid

    int main(int argc, char *argv[]) {   
      if (argc > 1) return handleProgram(argv[1]);
      else ...  
    
  7. Prefer terminating error messages to go to stderr

    // printf("\ttoInt()::No number detected, ...
    fprintf(stderr, "\ttoInt()::No number detected, ...
    
  8. Overflow not detected in toInt().

Style

  1. Avoid naked magic numbers. Consider

    // for (int i = 31; i >= 0; i--)
    #define INT_WIDTH  32
    for (int i = INT_WIDTH - 1; i >= 0; i--)
    
  2. int toInt() returns only 2 different int. Consider bool.

Minor

  1. See little value in excessive in vertical spacing throughout code. Example: The need for a blank line between buys little clarity for the price of a line.

    printBinary(number);
    
    state = 0;
    
  2. Consider parens ()

    // if (number >> i & 1 == 1)
    if ((number >> i) & 1 == 1)
    // or simply
    if ((number >> i) & 1)
    
\$\endgroup\$
0
\$\begingroup\$

Two things about your code.

First, try putting all of your constants in macros. This will make your code more readable and less change-error prone.

Second, you assume that all of your input integers are signed. But if the user doesn't want a signed int and wants to convert an unsigned int to binary he would get one major difference: The int range is two times smaller.

I suggest you ask for input like s* if * is a signed number or u* if * is unsigned. Also change / add a function to handle this case.

\$\endgroup\$
  • 1
    \$\begingroup\$ I would also specify signed long ints \$\endgroup\$ – THE AMAZING Apr 22 '16 at 14:15
  • \$\begingroup\$ I disagree. macros are hard to debug at best, and it clutters up the code. I personally only use macros for values I want to be editable. \$\endgroup\$ – MarcusJ May 24 '16 at 20:34

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.