1
\$\begingroup\$

I've written the following function in C which shall take as input a character array (decayed to a char * of course), and mutate the string into a title-cased string in the form "this is a string" -> "This Is A String".

void title_case(char str[])
{
    assert(str);
    size_t i;
    str[0] = toupper((int) str[0]);
    if(strlen(str) == 1) return;
    for(i = 1; i < strlen(str); ++i)
    {
        str[i] = tolower((int)str[i]);
        if(str[i -1] == ' ')
        {
            str[i] = toupper((int)str[i]);
        }
    }
    return;
}

Please provide any feedback you see fit. Is there a more concise way to do this? Is array bracket notation the most clear way?

Driver:

#include <stdio.h>
#include <string.h>
#include <assert.h>
#include <ctype.h>

void title_case(char str[]);
int main(void)
{
    char sentence[] = "this is a SeNTeNce.";
    title_case(sentence);
    printf("%s\n", sentence);
    return 0;
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ What is the expected output for "this.is.a.string"? \$\endgroup\$ – vnp Mar 21 '20 at 22:19
2
\$\begingroup\$
  1. In the context of function parameter passing, char* and char[] are equivalent (because char[] decays to char*).

    Anyway, I would rather use title_case(char* s) because it more clearly expresses the truth: what you really get within the function is a pointer to the first character of the array.

  2. Accepting char* as string parameter implicitly suggests that the passed string will be '\0'-terminated. As you probably know, there are many known security issues with this. If that's not the case, that means if you cannot guarantee the '\0'-ending always, consider passing the string length also, for ex.: title_case(char* s, size_t len)

  3. The cast from char to int (int)s[0] should not be needed, because conversion from smaller to the bigger integral type is always done implicitly and should trigger no warning on any compiler I know.

4.The strlen() in this loop for(i = 1; i < strlen(str); ++i) is called on every iteration, so the overall complexity will be quadratic. The common practice to avoid this is to store length into a variable and then use it, like:

size_t len = strlen(str);
for (i = 1; i < len; ++i) ...
  1. The more concise way with linear complexity would be something like this

>

#include <stdio.h>
#include <string.h>
#include <ctype.h>

void title_case(char *s)
{
    if (s == NULL) return;

    for (char *p = s; *p != '\0'; ++p)
        *p = (p == s || *(p-1) == ' ') ? toupper(*p) : tolower(*p);
}

int main()
{
    char s[] = "this is some     title";
    title_case(s);
    printf("%s\n", s);
}

Note: In the boolean expression p == s || *(p-1) == ' ' short-circuit evaluation capability of C/C++ is used: if p==s is true, then *(p-1) is not evaluated. Otherwise we would dereference invalid pointer -> which would lead to undefined behaviour.

Edited: Emphasize quadratic complexity issue, as proposed in comments by @sudo

\$\endgroup\$
5
  • \$\begingroup\$ Looks like you forgot, that OP's code also lowercased all the non-first letters of words \$\endgroup\$ – Yuri Kovalenko Mar 21 '20 at 10:37
  • \$\begingroup\$ @Yuri, yes thanks. Have made an update. \$\endgroup\$ – StPiere Mar 21 '20 at 10:54
  • \$\begingroup\$ Note that OP's code calls strlen on each iteration which makes the complexity quadratic while the proposed version in this answer has linear complexity. (GCC optimizes the calls to strlen, Clang does not.) \$\endgroup\$ – sudo rm -rf slash Mar 21 '20 at 21:22
  • \$\begingroup\$ @sudo: yeah, thanks. Updated. \$\endgroup\$ – StPiere Mar 22 '20 at 6:02
  • 1
    \$\begingroup\$ @chux: it's correct. updated. The emphasize was on more concise solution, not the most efficient. But good point anyway. \$\endgroup\$ – StPiere Mar 23 '20 at 18:44
1
\$\begingroup\$

O(n*n)

With a changing str and strlen(str) in the for(i = 1; i < strlen(str); ++i) loop, code repeatedly calculates the string length. Once is enough.

Even easier, test for the null character.

is...(int ch) called with char

is...(int) and to...er(int) functions expect an unsigned char value or EOF. When called with a char less than zero (and not EOF), the result in undefined behavior. The C standard library here treats char as if it was unsigned char. Best to do like-wise.

OP's (int) cast as in toupper((int) str[0]) serves scant purpose.

Simplify start of word logic

Suggest using a flag that is set whenever the processed char was a white-space.

Array brackets?

This is a style issue. As such, best to follow your group's coding standard. The standard C library use the 2nd style.

void title_case(char str[])
// or
void title_case(char *str)   // I find this more commmon

Sample code

#include <assert.h>
#include <ctype.h>
#include <stdbool.h>

void title_case(char str[]) {
  assert(str);
  bool StartOfWord = true;
  while (*str) {
    if (StartOfWord) {
      *str = toupper((unsigned char) *str);
    } else {
      *str = tolower((unsigned char) *str);
    }
    StartOfWord = isspace((unsigned char) *str);
  }
}
\$\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.