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;