Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
share|improve this question
2  
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 –  the_lotus 7 hours ago

3 Answers 3

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.

share|improve this answer
3  
You suggestion kinda screams for a lambda :) –  Code Clown 13 hours ago
1  
@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. –  Corbin 5 hours ago
1  
Why did you use int instead of char? –  Paul 1 hour ago

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

std::string ret = "";

Would be equivalent to:

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 = "";.


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.

share|improve this answer

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.

share|improve this answer
    
The difference between const auto &el and auto const &el is nothing? –  true 16 hours ago
4  
@true: I don't believe there is a difference. –  Jamal 16 hours ago
1  
You sure? e.g. in const char* const every const has its meaning. –  Code Clown 13 hours ago
1  
@CodeClown: It works with my small tests, and this answer says the same thing. –  Jamal 13 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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