4
\$\begingroup\$

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;
}
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Agree with your bufsize ideas. Note: In C, strings always, by definition, have a null character termination, else that char array is not a string. \$\endgroup\$ – chux - Reinstate Monica Oct 30 '14 at 22:11
2
\$\begingroup\$
  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;
    }
    
\$\endgroup\$
2
\$\begingroup\$

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).

\$\endgroup\$
4
  • \$\begingroup\$ Can you elaborate on the array of 255 characters. Admittedly, I don't have a lot of experience with bit shifting. \$\endgroup\$ – Ryan McCullagh Oct 22 '14 at 22:40
  • \$\begingroup\$ convert[src[len]]; will not work as desired should char be signed. Suggest convert[(unsigned char) src[len]]; \$\endgroup\$ – chux - Reinstate Monica Oct 31 '14 at 1:14
  • \$\begingroup\$ @chux: Fixed first. Second does not apply as src is already unsigned char. \$\endgroup\$ – Martin York Oct 31 '14 at 5:18
  • \$\begingroup\$ @Loki Astari Oops - did not notice you changed the function signature - hmmmm: not so keen on that. IAC, we agree the index to convert[] should be unsigned. \$\endgroup\$ – chux - Reinstate Monica Oct 31 '14 at 12:49
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.