9
\$\begingroup\$

I wrote a simple function that converts an array of words into proper case (using bitwise operations) and returns the array. I performed a simple test using std::chrono to check how fast it was able to perform the conversion. Using a vector of 1000 words the function was able to do it in an average of 27 microseconds.

My question is, if there is a faster way of doing this or if there is anything that I have missed that may be detrimental to the performance of the function. In addition, if there is any syntactical issues or changes that you would recommend that would be greatly appreciated.

#include <vector>
#include <string>

std::vector<std::string> ToProperCase(std::vector<std::string>& array)
{
    if (array.empty())
        return array;

    // Loop through each word
    for (std::string& value : array)
    {
        // Loop through each character in the word
        for (unsigned int i = 0; i < value.size(); ++i)
        {
            // Capitalise first character
            value[0] &= (~(1 << 5));

            // Convert character to lower case
            if (i > 0)
                value[i] |= (1 << 5);
        }
    }

    return array;
}

\$\endgroup\$
  • 1
    \$\begingroup\$ Fast, but it's not unicode-aware, so what's the point? \$\endgroup\$ – Alexander - Reinstate Monica Nov 23 '19 at 14:26
  • 4
    \$\begingroup\$ @Alexander We get users from various backgrounds; some beginners, some pros. Ridiculing users for not solving challenges you think are 'hard enough' isn't helpful. Please keep your elitism out of Code Review. \$\endgroup\$ – Peilonrayz Nov 23 '19 at 17:49
  • 1
    \$\begingroup\$ @Alexander I wrote this function fully aware that it will not be compatible for all characters and instead could have used the toupper() and tolower() functions. I am mainly interested in the performance of using bit shifting directly as well as my coding methodologies such as if I should be checking if an array is empty or simply the naming conventions and style. I do 100% agree that it would be easier to use the functions mentioned above. \$\endgroup\$ – ZOulhadj Nov 23 '19 at 18:24
  • 3
    \$\begingroup\$ @Peilonrayz Wasn't elitism, though I was quite blunt, which I would edit if I could. IMO, beginner materials gloss too much over unicode correctness (read: supporting anything that isn't english, which is most of the world) \$\endgroup\$ – Alexander - Reinstate Monica Nov 23 '19 at 18:28
  • 1
    \$\begingroup\$ I wouldn't call what you're doing "bit shifting". There a shift operator << used with a constant, so it doesn't count. I guess the right name is "bitwise" operations. \$\endgroup\$ – maaartinus Nov 24 '19 at 16:43
12
\$\begingroup\$
std::vector<std::string> ToProperCase(std::vector<std::string>& array)
{
    // ...
    return array;
}

There are many styles when it comes to capitalization. ToProperCase should have a better name to indicate what the function does, which is to capitalize the first letter of each word.

array is a non-local reference being returned by value. Is this intended?


    if (array.empty())
        return array;

    // Loop through each word
    for (std::string& value : array)

While compilers will optimize it, the early return is unnecessary. The range-based for will check to see if array is empty. If it is, the function will return array.

Don't say in comments what can be clearly stated in code. Rather than using variable names like array and value, omit the comment and use words and word respectively. Reserve comments to concisely state intent and keep them crisp.

        // Loop through each character in the word
        for (unsigned int i = 0; i < value.size(); ++i)
        {
            // Capitalise first character
            value[0] &= (~(1 << 5));

            // Convert character to lower case
            if (i > 0)
                value[i] |= (1 << 5);
        }
    }

What is the proper case of a digit? Punctuation? Control characters? Should you be mutating non-alpha characters?

You do more work than necessary here. For every character in the word, you bitwise-and the first character then bitwise-or the current character. You can unswitch the body of the innermost loop by handling the first character before every subsequent character.

    for (auto& word : words)
    {
        if (word.empty()) continue;

        word[0] = std::toupper(static_cast<unsigned char>(word[0]));

        for (auto i = 1u; i < word.size(); ++i) 
        {
            word[i] = std::tolower(static_cast<unsigned char>(word[i]));
        }
    }
\$\endgroup\$
  • \$\begingroup\$ Thanks for the review. One question I wanted to ask is although there are many different capitalisations, doesn't 'proper case' indicated by default that the first letter in each word will be capitalised? \$\endgroup\$ – ZOulhadj Nov 23 '19 at 19:18
  • 3
    \$\begingroup\$ @ZOulhad I don't think what you're doing here is any kind of actual, real-world capitalization, it's certainly not one I would call "proper". I assume what you actually want to implement would be title case, which capitalizes more words than your normal sentence casing but by no means all. Might make a good next training program. \$\endgroup\$ – Voo Nov 23 '19 at 19:26
  • \$\begingroup\$ I went ahead and striked the naming of the function. Apparently "proper case" is an appropriate variation of title casing. TIL. \$\endgroup\$ – Snowhawk Nov 23 '19 at 19:50
  • \$\begingroup\$ @Snowhawk Wouldn't that make it worse since the implemented function is not any form of title case I'm aware of? (There are variations of title case with everybody and their dog using slightly different versions, but "capitalize every word" is not one I can imagine anyone's using)\ \$\endgroup\$ – Voo Nov 23 '19 at 19:53
  • 2
    \$\begingroup\$ @Voo I asked a friend about it right before I struck it and she mentioned that Oracle refers to the action as proper casing. \$\endgroup\$ – Snowhawk Nov 23 '19 at 20:00
7
\$\begingroup\$

You code is pretty nice, but still, there are improvements.

Each time in the inner loop, your code unnecessarily runs value[0] &= (~(1 << 5)); for value.size() times. That would take unnecessary time. Running it once should be enough.

This would be the code after the improvements.

vector<string> ToProperCase(vector<string>& array)
{
    if (array.empty())
        return array;

    for (string& value : array)
    {
        if(value == "")
            continue;

        value[0] &= ~(1 << 5);

        for (unsigned int i = 1; i < value.size(); ++i)
            value[i] |= (1 << 5);
    }

    return array;
}

Also, you can use toupper() and tolower() instead, though I'm not sure if they are faster.

If I find anymore improvements, I'll be sure to edit them in!

EDIT:

If you want your code to be really fast, you can use map to store all the alphabets, but that would reduce the neatness of the code. It's your wish!

\$\endgroup\$
  • 4
    \$\begingroup\$ You migth use range based for in the second loop as well. toupper/tolower are probably not faster, but that's because they are actually handling all cases properly. \$\endgroup\$ – slepic Nov 23 '19 at 8:02
  • \$\begingroup\$ Thank you for review. In regards to the toupper() and tolower(), I didn't use them intentionally as I wanted to implement the conversion without the use of a function and at the same time get a better idea of how they bit shifting works. In addition, as @slepic mentioned, the functions take into account all types of variables such as characters form different languages and I would not be surprised if they were slower. \$\endgroup\$ – ZOulhadj Nov 23 '19 at 10:46
  • 1
    \$\begingroup\$ Just out of curiosity, but if possible, can you please tell me what the average time taken now? Thanks :) \$\endgroup\$ – Srivaths Nov 23 '19 at 10:55
  • \$\begingroup\$ @Srivaths The same exact code replaced with the functions is an average of 80 microseconds which around 60 more than by simply using bit shifting on my system. A slight increase but still fast considering that it supports international characters. \$\endgroup\$ – ZOulhadj Nov 23 '19 at 18:33
  • \$\begingroup\$ Oh, but what if toupper and tolower is not used, but the code is replaced with mine. How much timw does it take then? (Sorry to nag, but my PC is slow, so the time might not be accurate) \$\endgroup\$ – Srivaths Nov 23 '19 at 18:38

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.