Finding palindromes and reverse strings in C

Question

I've created a program that will find if a word is a palindrome or a reverse string (emordnilap).

How it works:

The palindrome part works by reversing the string, and comparing the two strings using strcmp to see if they match one another. If they do match, it will return true, if they do not match false.

How it checks if the word is a reverse string is by running through a wordlist line by line, stripping the string using a strip function and testing if the word is in the wordlist. Examples of usage:

e@ubuntu:~/bin/c$ ./epi.exe -h
usage: epi -h -t -s [string] [file]
-h Print this help and exit
-t Run the test strings.
-s Input a string to check if it is either a palindrome or
 a emordinlap, followed by the path to a file.
e@ubuntu:~/bin/c$ ./epi.exe -t
'a' is a palindrome, it is the same forward and backwards
Reverse of 'a' is a emordnilap, it spells a word backwards.
'ab' is not a palindrome, it is not the same forward and backwards
Reverse of 'ab' is not a emordnilap, it does not spell a word backwards
'abc' is not a palindrome, it is not the same forward and backwards
Reverse of 'abc' is not a emordnilap, it does not spell a word backwards
'another' is not a palindrome, it is not the same forward and backwards
Reverse of 'another' is not a emordnilap, it does not spell a word backwards
'cbc' is a palindrome, it is the same forward and backwards
Reverse of 'cbc' is not a emordnilap, it does not spell a word backwards
'|0|' is a palindrome, it is the same forward and backwards
Reverse of '|0|' is not a emordnilap, it does not spell a word backwards
'palindrome' is not a palindrome, it is not the same forward and backwards
Reverse of 'palindrome' is not a emordnilap, it does not spell a word backwards // technically emordnilap is not a word
e@ubuntu:~/bin/c$ ./epi.exe -s 
You must provide a string to test.
usage: epi -h -t -s [string] [file]
-h Print this help and exit
-t Run the test strings.
-s Input a string to check if it is either a palindrome or
 a emordnilap, followed by the path to a file.
e@ubuntu:~/bin/c$ ./epi.exe -s test
You must pass a valid file path to use as a wordlist.
usage: epi -h -t -s [string] [file]
-h Print this help and exit
-t Run the test strings.
-s Input a string to check if it is either a palindrome or
 a emordinlap, followed by the path to a file.
e@ubuntu:~/bin/c$ ./epi.exe -s test test.txt
'test' is not a palindrome, it is not the same forward and backwards
Reverse of 'test' is not a emordnilap, it does not spell a word backwards
e@ubuntu:~/bin/c$ ./epi.exe -s tset test.txt
'tset' is not a palindrome, it is not the same forward and backwards
Reverse of 'tset' is a emordnilap, it spells a word backwards.
e@ubuntu:~/bin/c$ 

Key points that I would like to focus on, of course critique the entire program:

• Is there a better way to reverse a string?
• Running through a file list, am I doing it properly?
• Could this program be simplified?

#include <stdio.h>
#include <string.h>
#include <stdbool.h>
static void *helpPage(void)
{
 puts("usage: epi -h -t -s [string] [file]\n");
 puts("-h Print this help and exit\n");
 puts("-t Run the test strings.\n");
 puts("-s Input a string to check if it is either a palindrome or");
 puts(" a emordnilap, followed by the path to a file.");;
}
static char *reverse(char *str)
{
 // reverse a given string
 char tmp, *src, *dst;
 size_t len;
 if (str != NULL)
 {
 len = strlen(str);
 if (len > 1)
 {
 src = str;
 dst = src + len - 1;
 while (src < dst)
 {
 tmp = *src;
 *src++ = *dst;
 *dst-- = tmp;
 }
 }
 }
 return str;
}
static char *strip(char *s)
{
 // strip a string of a new line
 return strtok(s, "\n");
}
static bool checkEpi(char *reversed, char *filePath)
{
 // check if the word matches in a given file
 // or if the word is an emordnilap
 FILE *wordList;
 char *line = NULL;
 size_t len = 0;
 ssize_t read;
 wordList = fopen(filePath, "r");
 if (wordList == NULL)
 {
 perror("Failed to open file :"); // file probably doesn't exit
 }
 while ((read = getline(&line, &len, wordList)) != -1) // read the file line by line
 {
 if (strcmp(strip(line), reversed) == 0)
 {
 return true; // return true if the word matches
 }
 }
 fclose(wordList);
}
static bool checkPali(char *origin, char *reversed)
{
 // check if a given word is a palindrome or not
 if (strcmp(origin, reversed) == 0)
 {
 return true;
 }
 else
 {
 return false;
 }
}
static void checkAll(char *origin, char* reverse, char *filePath)
{
 // basically a main function to check if it's a palindrome or a emordnilap
 bool paliRes = checkPali(origin, reverse);
 bool epiRes = checkEpi(reverse, filePath);
 if (paliRes == true)
 {
 printf("\n'%s' is a palindrome, it is the same forward and backwards\n", origin);
 }
 else
 {
 printf("\n'%s' is not a palindrome, it is not the same forward and backwards\n", origin);
 }
 if (epiRes == true)
 {
 printf("Reverse of '%s' is a emordnilap, it spells a word backwards.\n\n", origin);
 }
 else
 {
 printf("Reverse of '%s' is not a emordnilap, it does not spell a word backwards\n\n", origin);
 }
}
int main(int argc, char *argv[])
{
 if (argv[1] == NULL)
 {
 puts("\nYou failed to pass a valid flag...\n");
 helpPage();
 return 1;
 }
 else
 {
 char *testStrings[] = {"a", "ab", "abc", "another", "cbc", "|0|", "palindrome"};
 int i;
 char s[10000];
 char *defaultWordList = "/usr/share/dict/american-english";
 size_t optInt;
 for (optInt = 1; optInt < argc && argv[optInt][0] == '-'; optInt++)
 {
 switch(argv[optInt][1])
 {
 case 't':
 {
 for (i = 0; i < sizeof(testStrings) / sizeof(testStrings[0]); i++)
 {
 strcpy(s, testStrings[i]);
 char *origin = testStrings[i];
 char *revStr = reverse(s);
 checkAll(origin, revStr, defaultWordList);
 }
 return 0;
 }
 case 's':
 {
 if (argv[2] == NULL)
 {
 puts("\nYou must provide a string to test.\n");
 helpPage();
 return 1;
 }
 else if (argv[3] == NULL)
 {
 puts("\nYou must pass a valid file path to use as a wordlist.\n");
 helpPage();
 return 1;
 }
 else
 {
 strcpy(s, argv[2]);
 char *revStr = reverse(s);
 checkAll(argv[2], revStr, argv[3]);
 return 0;
 }
 }
 case 'h': helpPage(); return 0;
 }
 }
 return 0;
 }
}

• fer-rum: Took only a quick look. Congratulations for quite clean code style. Some of your function parameters can be made 'const'. Also it might be a good idea to validate each argument at the beginning of the function (Especially in C, espeacially when using pointers).
• jakehimton: @fer-rum Thanks for noticing, I try hard to keep my code clean, and thanks for the tip
• fer-rum: Why does helpPage() suggest to return a void pointer (and then return nothing at all)? Double semicolon spotted. Note that printf is discouraged for real-world production use, since it can be used in exploits.
• jakehimton: @fer-rum To be 100% honest with you, it's because I have absolutely no idea what the hell I'm doing when it comes to C lol. How can printf be used for exploitation?
• Przemek: @jakehimton: The most possibly irrelevant comment coming through: you have misspelled word "emordinlap" in the last line of your helpPage function.

Answer 1

First off: I really like reading your code. The code style looks clean and the intend of all variables seem clear to the reader - good naming. This was pretty quick and I may revisit this post later - don't have enough time right now.

Here are some things I noticed (in no particular order):

• You declared the function helpPage(void) to return void *, but never return anything. You probably just meant to write void. Also I would've replaced helpPage with printHelpPage.

• Declare variables when you first use them. This makes the intend more clear to the reader. Example in reverse(char *str):

  // Reverse a given null-terminated string
  static char *reverse(char *str)
  {
   if (str != NULL)
   {
   size_t len = strlen(str);
   if (len > 1)
   {
   char *src, *dst;
   src = str;
   dst = src + len - 1;
   while (src < dst)
   {
   char tmp = *src;
   *src++ = *dst;
   *dst-- = tmp;
   }
   }
   }
   return str;
  }
  

  I think this makes the use of each variable easier to understand, otherwise the reader could be confused (What do I need all these variables for?).

• I'm not sure if creating an extra function strip(char *s) for a single strtok call is really necessary.

• Pay attention to const correctness in checkEpi: Mark reversed and filePath const, since checkEpi doesn't modify them through these pointers:

  static bool checkEpi(const char *reversed, const char *filePath);
  

  Same with checkPali / checkAll.

• Handle failed fopen. You do print and error message using perror, but the function should return right after printing the error.
• Memory leak in checkEpi. getline will allocate memory for line and you are responsible for freeing it later on.
• I don't see a return statement in checkEpi beside from the return true. Explicitly return false!
• checkPali shouldn't expect the original and the reversed string. The function should check if a single string is a palindrome (the reversing happens inside checkPali) - same thing in checkAll.

• Take a look at your current checkPali implementation. Never write code like this:

  if (something) {
   return true;
  } else {
   return false;
  }
  

  but instead write return something; right away .

• checkAll: Your current code assigns the return values of checkPali and checkEpi to variables named and paliRes and epiRes and later on checks them with paliRes == true. Consider renaming these variables to isPalindrome and isEmordnilap and using them like if(isPalindrome) ..., etc.
• Minor: I probably would've defined testStrings and defaultWordList at the top of the file via the #define-directive.
• Minor: You accidentally put one semicolon too many into helpPage(void).

This is what I came up with:

(The code should just give you a rough example on how to do it. I'm currently in a rush, so please excuse any mistakes made)

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
static void printHelpPage(void) {
 puts("Usage: epi -h -t -s [string] [file]\n");
 puts("-h Print this help and exit\n");
 puts("-t Run the test strings.\n");
 puts("-s Input a string to check if it is either a palindrome or");
 puts(" a emordnilap, followed by the path to a file.");
}
// In-place reverse null-terminated string
static char *reverse(char *str) {
 size_t len;
 if (str != NULL && (len = strlen(str)) > 1) {
 char *src, *dest;
 src = str;
 dest = src + len - 1;
 while (src < dest) {
 char tmp = *src;
 *src++ = *dest;
 *dest-- = tmp;
 }
 }
 return str;
}
// Return values: -1 on error, 0 for no Emordnilap, 1 for Emordnilap
static int checkEmordnilap(const char *str, const char *filePath) {
 char *reversed = malloc(strlen(str) + 1);
 if (!reversed) {
 perror("Failed to allocate memory to store reversed string ");
 return -1;
 }
 strcpy(reversed, str);
 reverse(reversed);
 FILE *wordListFile = fopen(filePath, "r");
 if (!wordListFile) {
 free(reversed);
 perror("Failed to open wordList ");
 return -1;
 }
 char *line = NULL;
 size_t linecap = 0;
 ssize_t linelen;
 while((linelen = getline(&line, &linecap, wordListFile)) > 0) {
 if (strcmp(strtok(line, "\n"), reversed) == 0) {
 return 1;
 }
 }
 fclose(wordListFile);
 free(reversed);
 return 0;
}
// Return -1 on error, 0 on no palindrome, 1 on palindrome
static int checkPalidrome(const char *str) {
 char *reversed = malloc(strlen(str) + 1);
 if (!reversed) {
 perror("Failed to allocate memory to store reversed string.\n");
 return -1;
 }
 strcpy(reversed, str);
 reverse(reversed);
 int result = strcmp(str, reversed) == 0;
 free(reversed);
 return result;
}
static void printStatus(const char *origin, const char *filePath) {
 int isPalindrome, isEmordnilap;
 isPalindrome = checkPalidrome(origin);
 isEmordnilap = checkEmordnilap(origin, filePath);
 if (isPalindrome == -1 || isEmordnilap == -1) {
 // We already see the error message printed from checkPalindrome/checkEmordnilap.
 return;
 }
 if (isPalindrome) {
 printf("\n'%s' is a palindrome, it is the same forward and backwards\n", origin);
 } else {
 printf("\n'%s' is not a palindrome, it is not the same forward and backwards\n", origin);
 }
 if (isEmordnilap) {
 printf("Reverse of '%s' is a emordnilap, it spells a word backwards.\n\n", origin); 
 } else {
 printf("Reverse of '%s' is not a emordnilap, it does not spell a word backwards\n\n", origin);
 }
}
#define TEST_STRINGS { "a", "ab", "abc", "another", "|0|", "palindrome" }
#define DEFAULT_WORD_LIST "/usr/share/dict/propernames"
int main(int argc, char *argv[]) {
 if (argc < 2) {
 printHelpPage();
 return 1;
 }
 for (int argNum = 1; argNum < argc && argv[argNum][0] == '-'; ++argNum) {
 switch(argv[argNum][1]) {
 case 't': {
 char *testStrings[] = TEST_STRINGS;
 for (size_t wordIndex = 0; wordIndex < sizeof(testStrings)/sizeof(testStrings[0]); ++wordIndex) {
 printStatus(testStrings[wordIndex], DEFAULT_WORD_LIST);
 }
 break;
 }
 case 's':
 if (argc < 3) {
 puts("\nYou must provide a string to test.\n");
 printHelpPage();
 return 1;
 }
 char *wordList = argc < 4 ? DEFAULT_WORD_LIST : argv[4];
 printStatus(argv[2], wordList);
 break;
 case 'h':
 continue;
 default:
 printHelpPage();
 break;
 }
 }
 return 0;
}

Answer 2

The (str != NULL) is not needed as str, as a string, is never NULL. In C, a string is a sequence (e.g. array) of characters up to and including the terminating null character.

Still it is not a bad idea to check against NULL for as a pointer, char * sometimes will be assigned NULL.

The if (len > 1) can be eliminated by taking advantage of starting with right assigned to the address of the null character.

static char *reverse(char *str) {
 char *left = str;
 char *right = str + strlen(str);
 while (right > left) {
 right--;
 char tmp = *right;
 *right = *left;
 *left = tmp;
 left++;
 }
 return str;
}