Printing permutations of a given string

Question

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.

Answer 1

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

Answer 2

• 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);

• 200_success: Note that char a[] = {"1234"} (or just char a[] = "1234") is not quite the same as the original, as it adds a 0円 terminator.