current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 10 down vote favorite

I want some feedback on the program I developed in C for printing the permutations of a given string.

#include <stdio.h>
bool isUnique(char x[], int y)
{
   int i, j;
   for (i = 0; i < (y-1); i++)
     for (j = i+1; j < y; j++)
       if (x[i] == x[j])
          return false;
   return true;
}
void perm(char a[], char b[], int x, int y)
{
   if ((y == x) && isUnique(b,x))
   {
     b[y] = '\0';
     printf("%s\n", b);
   }
   else
     for (int i = 0; i < x; i++)
     {
        b[y] = a[i];
        if (y < x)
          perm(a,b,x,y+1);
     }
}
int main()
{
   char a[] = {'1','2','3','4'}, b[] = {'\0','\0','\0','\0','\0'};
   perm(a,b,4,0);
   return 0;
}

Also, please tell me where I'm making blunders as far as conventions and good practice are concerned.

share|improve this question

2 Answers 2

up vote 7 down vote accepted

  • Do not use single-character names for variables. This will make your code harder to understand. Give them names based on their significance to the program.

  • Put your two arrays on separate lines for clarity:

    char a[] = {'1','2','3','4'};
char b[] = {'\0','\0','\0','\0','\0'};

  • Arguments to a function that aren't modified within the function should be const.

    This applies to a[] in main():

    const char a[] = {'1','2','3','4'};

  • Parameters that aren't modified within a function could be const in order to prevent any accidental modifications.

    This applies to all parameters in this program except for b[].

  • Since you declare your for loop counters inside the loop statement, and you're using bools, then it appears you're using C99.

    Two things you should be doing here:

    1. Consistently declare your loop counters inside the statements. You're doing it the pre-C99 way in isUnique().

    2. Your code does not compile with the bools. To use them, you must include <stdbool.h>.

  • You're inconsistently declaring your loop counters inside and outside the for loop statements. Since you initialize them inside in one instance, you must be using C99. If so, then this should be consistent for each of these loops.

  • In perm(), you should have curly braces for the else, just as you have them for the if. Make sure to maintain such consistencies throughout your program.

    You should also do this for the for loops and ifs in isUnique(). It is especially important here as it helps with maintenance and will allow you to add additional lines if needed.

  • Functions in C that have no parameters should have a void parameter:

    int main(void) {}

  • The return 0 at the end of main() is unneeded; the compiler will do this automatically.

With these changes, I've come up with the following code (also tested on Ideone):

#include <stdbool.h>
#include <stdio.h>

bool isUnique(const char x[], const int y)
{
    for (int i = 0; i < (y-1); i++)
    {
        for (int j = i+1; j < y; j++)
        {
            if (x[i] == x[j])
            {
                return false;
            }
        }
    }

    return true;
}

void perm(const char a[], char b[], const int x, const int y)
{
    if ((y == x) && isUnique(b, x))
    {
        b[y] = '\0';
        printf("%s\n", b);
    }
    else
    {
        for (int i = 0; i < x; i++)
        {
            b[y] = a[i];

            if (y < x)
            {
                perm(a, b, x, y+1);
            }
        }
    }
}

int main(void)
{
    const char a[] = { '1', '2', '3', '4' };
    char b[] = { '\0', '\0', '\0', '\0', '\0' };

    perm(a, b, 4, 0);
}
share|improve this answer
    
Thanks a lot. It was helpful. :) –  Mahesh Mar 21 '14 at 7:28
up vote 6 down vote

  • There is no need to write like:

    char a[] = {'1','2','3','4'};

    You can change it to:

    char a[] = {"1234"};

  • Single character variables makes code harder to understand, so use proper names for variables

  • Use {} for if, else, and for statements, even they have a single statement inside

  • Actually you can do it without using second array like:

// This function is used for swapping two characters (From the array)
void swapCharacters(char *swappingCharacter, char *swappingCharacter)
{
    char temp;
    temp               = *swappingCharacter;
    *swappingCharacter = *swappingCharacter;
    *swappingCharacter = temp;
}


// Calculated the permutation
void calculatePermutation(char *string, int startIndex, int endIndex)
{
   int counter;
   if (startIndex == endIndex)
   {
     printf("%s\n", string);
   }
   else
   {
       for (counter = startIndex; counter <= endIndex; counter++)
       {
          swapCharacters((string + startIndex), (string + counter));
          permute(string, startIndex + 1, endIndex);
          swapCharacters((string + startIndex), (string + counter));
       }
   }
}

And call it as:

calculatePermutation(a, 0, 2);
share|improve this answer
    
Thanks for your feedback and also your suggestion. –  Mahesh Mar 21 '14 at 7:29
1  
Note that char a[] = {"1234"} (or just char a[] = "1234") is not quite the same as the original, as it adds a \0 terminator. –  200_success♦ Mar 29 '14 at 3:44

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.