7
\$\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? Let's get critical here.

#include <iostream>
#include <string>
//
// return a new string
//
std::string camel_case_rev(const std::string &word)
{
        std::string ret = "";

        for(unsigned i = 0; i < word.size(); i++) {
                if(word[i] <= 'Z' && word[i] >= 'A') {
                        ret += tolower(word[i]);
                }
                else if(word[i] <= 'z' && word[i] >= 'a') {
                        ret += toupper(word[i]);
                }
                else {
                        ret += word[i];
                }
        }
        return ret;
}


int main(int argc, char **argv)
{
        if(argc < 2) {
                std::cerr << "USAGE: " << argv[0] << " " << "<string>\n";
                return 1;
        }

        std::string a = camel_case_rev(argv[1]);

        std::cout << a << "\n";

        return 0;
}
\$\endgroup\$
  • 3
    \$\begingroup\$ My C is rusted so I'm not going to post an answer. In the asciitable the difference between upper and lower characters is 1 bit. You could just flip the 6th bit with a xor. A = 1000001, a = 1100001 \$\endgroup\$ – the_lotus Oct 23 '14 at 12:20
  • \$\begingroup\$ @the_lotus could you show me how to do that? \$\endgroup\$ – Ryan M Oct 23 '14 at 22:58
  • 1
    \$\begingroup\$ Like I said, my C is rusty. Might be something like this: if( (word[i] <= 'Z' && word[i] >= 'A') || (word[i] <= 'z' && word[i] >= 'a')) ret += word[i] ^ 0x20; This assume that the string is encoded in ASCII \$\endgroup\$ – the_lotus Oct 24 '14 at 12:20
  • \$\begingroup\$ @the_lotus this has worked perfectly and it is my favorite improvement. If you choose to submit an answer, be sure that I will accept it. \$\endgroup\$ – Ryan M Oct 24 '14 at 17:34
19
\$\begingroup\$

This seems like a perfect opportunity to leverage the standard library. I would make a function to toggle a single character and then use std::transform on top of that. Not only is it more consise, it will be more performant (you should use reserve to preallocate the resulting string -- otherwise the string may allocate over and over again).

#include <cctype>
#include <string>
#include <algorithm>

int toggle_case(int chr) {
    return std::islower(chr) ? std::toupper(chr) : std::tolower(chr);
}

std::string toggle_case(std::string str) {
    std::transform(std::begin(str), std::end(str), std::begin(str), toggle_case);
    return str;
}

Note that standard islower and isupper were used instead of comparison against character literals. Not only is it more compact and clearer, C++ does not guarantee that 'A', 'B', ..., 'Z' are contiguous.

Also, since you're (presumably) including the C++ version of ctype.h, cctype, you should be using std::toupper and std::tolower. C++ versions of C headers are allowed to place symbols in the global namespace, but they're only required to place them in std. There's no existing compiler I know of that doesn't also place them in the global namespace, but it can never hurt to strive for portabiltiy, especially when it's easy.

return std::islower(chr) ? std::toupper(chr) : std::tolower(chr);

This might look a little weird, but std::tolower and std::toupper are guaranteed to have no effect if no opposite-cased character exists for the input. In other words, std::tolower(c) is guaranteed to have no effect unless std::toupper(c) is true. To me, this makes sense and this short-cutted logic seems fairly natural. I could see this being rather weird to read though, so it might be better as a more explicit conditional chain.

I should also mention that the code makes an implicit copy of the string via the parameter. If you wanted to make the copy more explicit, you could, but that would actually be potentially hampering performance since it would disable movement into the parameter. In short, with the implicit copy, there's a potential that a copy will not actually happen. With an explicit copy, a copy will always happen.

\$\endgroup\$
  • 4
    \$\begingroup\$ You suggestion kinda screams for a lambda :) \$\endgroup\$ – aggsol Oct 23 '14 at 6:36
  • 2
    \$\begingroup\$ @CodeClown Yeah, I should have at least mentioned the lambda option. It seems feasible though that you might want the per-character toggle function as an accessible thing. \$\endgroup\$ – Corbin Oct 23 '14 at 14:46
  • 1
    \$\begingroup\$ Why did you use int instead of char? \$\endgroup\$ – Paul Oct 23 '14 at 18:32
  • 2
    \$\begingroup\$ @Paul the arg to isupper, islower is an int, you could also use unsigned char without any compiler warnings. \$\endgroup\$ – Ryan M Oct 23 '14 at 22:57
8
\$\begingroup\$

You don't need to default initialize a std::string.

std::string ret = "";

Has the equivalent result too:

std::string ret;

The string type has a constructor that creates an empty string automatically. It is OK though if you think the code looks clearer with the = "";.

To be exact:

std::string ret = "";  // Calls another constructor:       std::string::string(char const* s);
                       // Does what you expect; creates an empty string by copying the C-String
std::string ret;       // Calls the default constructor:   std::string::string()
                       // Does what you expect; creates an empty string.

tolower and toupper are declared in the <cctype> header, so you should include it to make dependencies explicit. Currently, you are relying on the "residual" include from <string>.

Also, since they belong to the namespace std, you should instead use std::tolower and std::toupper.


std::size_t would be a more adequate type for the loop counter in the for that is compared to word.size(). std::string::size() returns a string::size_type object, but that is just a typedef to size_t, so it is OK to use the shorter one in my opinion.

\$\endgroup\$
6
\$\begingroup\$

With C++11, you should use a range-based for-loop, which uses iterators:

for (auto const& iter : word) {
    if (iter <= 'Z' && iter >= 'A') {
        ret += tolower(iter);
    }
    else if (iter <= 'z' && iter >= 'a') {
        ret += toupper(iter);
    }
    else {
        ret += iter;
    }
}

This helps avoid bounds-checking or possible mismatched loop counter types. Also, making the iterator const& will prevent word from being modified and avoid copies on this read-only data.

\$\endgroup\$
  • \$\begingroup\$ The difference between const auto &el and auto const &el is nothing? \$\endgroup\$ – Ryan M Oct 23 '14 at 3:46
  • 4
    \$\begingroup\$ @true: I don't believe there is a difference. \$\endgroup\$ – Jamal Oct 23 '14 at 3:50
  • 1
    \$\begingroup\$ You sure? e.g. in const char* const every const has its meaning. \$\endgroup\$ – aggsol Oct 23 '14 at 6:34
  • 1
    \$\begingroup\$ @CodeClown: It works with my small tests, and this answer says the same thing. \$\endgroup\$ – Jamal Oct 23 '14 at 6:37
1
\$\begingroup\$

If the string is ASCII endoded, you can flip the 6th bit since it represend the case for a letter.

Ex: A = 1000001, a = 1100001

Haven't used C in a while but here's what it could look like

if( (word[i] <= 'Z' && word[i] >= 'A') || (word[i] <= 'z' && word[i] >= 'a') )
    ret += word[i] ^ 0x20;
\$\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.