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

Checking if characters in a string can be rearranged to make a palindrome

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

Problem

Can I please have some advice on optimizing, cleaning up my code, and places where I could save space/time?

#include 
#include 
#include 
#include 

bool pal_perm(char*);
int main()
{
    printf("The output is %sa palindrome.\n", pal_perm("abbas")? "": "not "); //Output: The output is a palindrome.
    printf("The output is %sa palindrome.\n", pal_perm("deeds")? "": "not "); //Output: The output is a palindrome.
    printf("The output is %sa palindrome.\n", pal_perm("dead")? "": "not "); //Output: The output is not a palindrome.
    return 0;
}

bool pal_perm(char* str)
{
    char alpha[256];
    int oddCount =0;
    int size = strlen(str);
    memset(alpha, 0, sizeof(alpha)); 

    //see how many occurances of each letter
    for(char ch = 'a'; ch <= 'z'; ch++)
    {
        for(int i=0; i < size; i++)
        {
            if(str[i] == ch)
                alpha[str[i]]++;
        }
    }

    //count the number of times a letter only appears once
    for(int j=0; j<256; j++)
    {
        if(alpha[j] == 1 || (alpha[j]%2==1))
            oddCount++;

    }
    //if there is more than one letter that only occurs, then it 
    //cannot be a palindrome.
    if(oddCount <= 1)
        return true;
    else
        return false;
}

Solution

Strange output

What is a user to think when seeing such output of a program?

The output is a palindrome.
The output is not a palindrome.


I wouldn't know what this program is trying to tell me.

Consider this alternative:

void print_result(char * s)
{
    printf("The characters of \"%s\" %s be rearranged into a palindrome.\n", s, pal_perm(s) ? "can" : "cannot");
}

int main()
{
    print_result("abbas");
    print_result("deeds");
    print_result("dead");
}


Output:

The characters of "abbas" can be rearranged into a palindrome.
The characters of "deeds" can be rearranged into a palindrome.
The characters of "dead" cannot be rearranged into a palindrome.


Though actually I would prefer something much simpler than that:

printf("\"%s\" -> %s\n", s, pal_perm(s) ? "true" : "false");


Producing output:

"abbas" -> true
"deeds" -> true
"dead" -> false


Usability

It would be more interesting if the program took the strings from the command line, instead of using hardcoded values, for example:

int main(int argc, char ** argv) {   
    for (int i = 1; i < argc; i++) {
        print_result(argv[i]);
    }
}


For the record, @Law29 suggested another alternative in a comment:


You can also read from standard input. This lets you either type in words as they come to mind, or use a whole file (there are files of dictionary words, for example). Example:

#define MAX_WORD_SIZE 50
int main(int argc, char ** argv) {   
    char buf[MAX_WORD_SIZE]
    while (fgets (buf, MAX_WORD_SIZE, stdin)) {
        print_result(buf);
    }
}


Testing

Getting the implementation right can be tricky.
You revised your post 3-4 times to fix bugs pointed out in comments.
It's good to automate your tests so that they can be repeated easily,
for example by adding methods like these:

void check(char * s, bool expected)
{
    if (pal_perm(s) != expected) {
        printf("expected \"%s\" -> %s but got %s\n", s, expected ? "true" : "false", expected ? "false" : "true");
        exit(1);
    }
}

void run_tests()
{
    check("a", true);
    check("aa", true);
    check("aba", true);
    check("abba", true);
    check("aabb", true);
    check("aabbs", true);
    check("deeds", true);

    check("ab", false);
    check("abc", false);
    check("dead", false);
}


Use boolean expressions directly

Instead of this:

if(oddCount <= 1)
    return true;
else
    return false;


You can simply return the boolean expression itself:

return oddCount <= 1;


Excessive looping

As @DarthGizka explained, instead of this:

for(char ch = 'a'; ch <= 'z'; ch++)
{
    for(int i=0; i < size; i++)
    {
        if(str[i] == ch)
            alpha[str[i]]++;
    }
}


This is identical, but without unnecessary looping:

for(int i=0; i < size; i++)
{
    alpha[str[i]]++;
}


Unnecessary conditions

The first condition is unnecessary:

if(alpha[j] == 1 || (alpha[j]%2==1))


This is exactly the same:

if(alpha[j]%2==1)


Too compact writing style

Instead of this:

if(alpha[j]%2==1)


I suggest to put spaces around operators, and before ( in if statements:

if (alpha[j] % 2 == 1)


Stop iterating when you already know the result

Once you find two characters with odd number of occurrences,
you can stop iterating and return false.
As such, you don't even need an int oddCount, but a bool seenOdd.
So instead of this:

int oddCount = 0;

//count the number of times a letter only appears once
for (int j = 0; j < 256; j++)
{
    if (alpha[j] % 2 == 1) oddCount++;
}
//if there is more than one letter that only occurs, then it 
//cannot be a palindrome.
return oddCount <= 1;


You could write:

bool seenOdd = false;

// scan for odd number of occurrences, stop after seeing two
for (int j = 0; j < 256; j++)
{
    if (alpha[j] % 2 == 1) {
        if (seenOdd) return false;
        seenOdd = true;
    }
}

// less then 2 letters with odd number of occurrences, must be true
return true;

Code Snippets

The output is a palindrome.
The output is not a palindrome.
void print_result(char * s)
{
    printf("The characters of \"%s\" %s be rearranged into a palindrome.\n", s, pal_perm(s) ? "can" : "cannot");
}

int main()
{
    print_result("abbas");
    print_result("deeds");
    print_result("dead");
}
The characters of "abbas" can be rearranged into a palindrome.
The characters of "deeds" can be rearranged into a palindrome.
The characters of "dead" cannot be rearranged into a palindrome.
printf("\"%s\" -> %s\n", s, pal_perm(s) ? "true" : "false");
"abbas" -> true
"deeds" -> true
"dead" -> false

Context

StackExchange Code Review Q#115308, answer score: 10

Revisions (0)

No revisions yet.