patterncModerate
Checking if characters in a string can be rearranged to make a palindrome
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?
I wouldn't know what this program is trying to tell me.
Consider this alternative:
Output:
Though actually I would prefer something much simpler than that:
Producing output:
Usability
It would be more interesting if the program took the strings from the command line, instead of using hardcoded values, for example:
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:
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:
Use boolean expressions directly
Instead of this:
You can simply return the boolean expression itself:
Excessive looping
As @DarthGizka explained, instead of this:
This is identical, but without unnecessary looping:
Unnecessary conditions
The first condition is unnecessary:
This is exactly the same:
Too compact writing style
Instead of this:
I suggest to put spaces around operators, and before
Stop iterating when you already know the result
Once you find two characters with odd number of occurrences,
you can stop iterating and return
As such, you don't even need an
So instead of this:
You could write:
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" -> falseUsability
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" -> falseContext
StackExchange Code Review Q#115308, answer score: 10
Revisions (0)
No revisions yet.