String case reverse function in C

Question

This reverses the case of a character in a string (a string being an array of characters). How does it look? Anything I can improve on? Besides documentation, in particular, any thoughts on the type casting found in str_case_rev?

#include <stdio.h>  // fprintf
#include <stdlib.h> // malloc
#include <string.h> // strlen
#include <ctype.h>  // toupper, tolower

/*
 * a string is an array of characters, in C, all arrays
 * are always passed never passed value, always a pointer to
 * the variable
 * This is why the caller does not need to call the function like:
 * camel_case_rev(src, &dest, len)
 *
 * Since here the array of characters ("a string") is already being
 * passed as a pointer value
 */
void str_case_rev(const char *src, char *dest, size_t len)
{
        size_t i = 0;

        for(; i < len; i++)
        {
                if(src[i] <= 'Z' && src[i] >= 'A')
                {
                        *(dest + i) = (char)tolower((int)src[i]);
                }
                else if(src[i] <= 'z' && src[i] >= 'a')
                {
                        *(dest + i) = (char)toupper((int)src[i]);
                }
                else
                {
                        *(dest + i) = src[i];
                }
        }

        i++;
        *(dest + i) = '\0';
}

int main(int argc, char **argv)
{
        if(argc < 2)
        {
                fprintf(stderr, "Usage: %s <string>\n", argv[0]);
                return EXIT_FAILURE;
        }

        char *dest = NULL;

        size_t len = strlen(argv[1]);

        dest = malloc(len + 1);

        if(NULL == dest)
        {
                fprintf(stderr, "Memory error\n");
                return EXIT_FAILURE;
        }

        str_case_rev(argv[1], dest, len);

        fprintf(stdout, "%s\n", dest);

        free(dest);

        return EXIT_SUCCESS;
}

Answer 1

The case determination code (src[i] <= 'Z' && src[i] >= 'A') doesn't look right (think about locales). isupper and islower are specifically designed to deal with that. Besides, you are already using toupper and tolower.

Answer 2

I would call a function to convert uppercase to lowercase and vice versa something like str_case_toggle().

I am puzzled by the len parameter in the function signature. Usually, C strings are null-terminated. Since this function only makes sense on text strings and not binary data, I think that it makes more sense to look for the null terminator in src than to state the length explicitly.

On the other hand, sometimes functions accept a bufsize parameter, which has subtly different semantics. A bufsize indicates the size of the destination buffer and helps to prevent buffer overflows. However, in that case, I would still expect the the loop to stop if it encounters a NUL byte in src first, so as not to try to read possibly invalid memory.

Answer 3

1. i++; after the loop is functionally wrong. Delete it.

   // i++;
   *(dest + i) = '\0';
   

2. size_t len is a problem. If it represents the result of strlen(src), then it is not needed as str_case_rev() can calculate that. If len represents the size of src, then writing the '\0' is occurring outside the size of src even with the above fix.

3. If code uses tolower(), using isupper() makes more sense than if(src[i] <= 'Z' && src[i] >= 'A').

4. Consider returning something useful rather than void str_case_rev(). Suggest returning dest or a flag indicating some toggling occurred.

   char *str_case_rev(const char *src, char *dest, size_t len) {
      ...
      return dest;
   }
   

5. Consider re-ordering to match str...() functions like char *strcpy(char * dest, const char *src);

   char *str_case_rev(char *dest, const char *src, size_t len) {
      ...
      return dest;
   }
   

Answer 4

Rather than have a big conversion statement I would just do:

 static const char convert[] = {
                          0, 1 ,2 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15
                          ....
                          /* The character code for 'A' 65 replace with 'a' 97 */

                          62, 63, 64, 'a', 'b', 'c', 'd',
                          ....
                          /* The character code for 'a' 97 replace with 65 'A' */
                          94, 95, 96, 'A', 'B', 'C', 'D', 

                          ....
                          248, 249, 250, 251, 252, 253, 254, 255};

 void str_case_rev(const unsigned char *src, char *dest, size_t len)
 {
     for(; i < len; i++)
     {
         dest[i] = convert[src[i]];
     }
     dest[len] = '\0';
 }

Then all you have to do is define the conversion in a single array.
Since a char is only 8 bits you only need to define an array of 255 characters (just make sure you use unsigned char).

Answer 5

The lookup table method would probably be faster but otherwise the logic of the code is just fine.

However, although I realize you're just practicing your skills, it should be noted that this routine would not be suitable for processing real text. Real text routines need to handle non-ascii text. Today that means either UTF-8 on non-Windows systems or UTF-16LE on Windows systems. Specifically, on non-Windows systems you would need to do mbrtowc and then towupper or towlower and then wcstombs within each iteration of your loop. On Windows systems the logic would be the same but the exact functions would be different. Fortunately this sort of thing (toggling the case of a string) is quite rare. Something like comparing two non-ascii strings in a caseless way is a more realistic example but fortunately still rare.