HiveBrain v1.2.0
Get Started
← Back to all entries
patterncModerate

isPermutation method

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
ispermutationmethodstackoverflow

Problem

I wrote an isPermute method. Can I get some tips and advice on better coding style?

#include 
#include 
#include 
#include 

bool isPermute(char*, char*);
char* sort(char*);

int main(int argv, char **argc)
{
  printf("%s and %s are %spermutations of each other. \n", argc[1], argc[2], isPermute(argc[1], argc[2]) ? 
    "" : "not ");

  return 0;
}

bool isPermute(char* s1, char* s2)
{

  int size1 = strlen(s1);
  int size2 = strlen(s2);

  //default cases
  if((size1 != size2) || (size1 == 0) || (size2==0))
    return false;  

  if(strcmp(sort(s1), sort(s2)))
    return false;

  else
    return true;
}

char* sort(char* str1)
{
  int d=0, size = strlen(str1);
  char character;
  char *original = str1;

  char *result = (char *)malloc(size);

  for ( character = 'a' ; character <= 'z' ; character++ )
  {
     int i;
     for ( i = 0 ; i < size ; i++ )
     {
        if ( str1[i] == character )
        {
           result[d] = str1[i];
           d++;
        }
     }
     str1 = original;
  }
  return result;
}

Solution

First off:

bool isPermute(char*, char*);
char* sort(char*);


You could just move the function implementation over the main method, avoiding to have these declarations; however, that isn't really a big deal.

The convention for int main isn't:

int main(int argv, char **argc)


It normally is the opposite: argc is for the arg count and argv is for the arguments passed to the program.

So it really should be:

int main(int argc, char **argv)


Next:

printf("%s and %s are %spermutations of each other. \n", argc[1], argc[2], 
    isPermute(argc[1], argc[2]) ? "" : "not ");


You never checked if the program was actually passed two parameters, and so instead it should be (argc is the arg count from now on, and argv is the arguments)

if(argc == 3) 
{
    printf("%s and %s are %spermutations of each other. \n", argv[1], argv[2], 
        isPermute(argv[1], argv[2]) ? "" : "not ");
}
else 
{
    printf("Incorrect amount of parameters passed!\n");
    return -1;
}


The following:

if(strcmp(sort(s1), sort(s2)))
    return false;
else
    return true;


can be trimmed down to be just:

return !strcmp(sort(s1), sort(s2));


The following:

char *result = (char *)malloc(size);


You don't need to have the explicit cast (char ) as malloc returns a void, so it can just be

char *result = malloc(size);


Which doesn't detract from clarity in any real way.

Also, I've noticed that you are declaring variables for the for loops outside the loop, even though that isn't necessary (unless you are using C89, in which case, ignore this)

So this:

for ( character = 'a' ; character <= 'z' ; character++ )
{
     ...
}


would become

for (char character = 'a'; character <= 'z'; character++)
{
    ...
}


The same thing essentially applies for the other for loop.

for ( i = 0 ; i < size ; i++ )
{ 
    ...
}


would become

for (int i = 0; i < size; i++)
{
    ...
}

Code Snippets

bool isPermute(char*, char*);
char* sort(char*);
int main(int argv, char **argc)
int main(int argc, char **argv)
printf("%s and %s are %spermutations of each other. \n", argc[1], argc[2], 
    isPermute(argc[1], argc[2]) ? "" : "not ");
if(argc == 3) 
{
    printf("%s and %s are %spermutations of each other. \n", argv[1], argv[2], 
        isPermute(argv[1], argv[2]) ? "" : "not ");
}
else 
{
    printf("Incorrect amount of parameters passed!\n");
    return -1;
}

Context

StackExchange Code Review Q#115287, answer score: 11

Revisions (0)

No revisions yet.