Replace every letter in the string with the letter following it in the alphabet and capitalize vowels

Question

Any suggestions to make this code better(efficient, style, etc.)?

I also want to know if there is a simpler way to check if a character is a vowel or not because I had to copy and paste "element == (vowel)" like 4 times.

int isVowel(char element)
{
    if(element == 'a'||element == 'e'||element == 'i'||element == 'o'||element == 'u')
    {
        return 0;
    }else
    {
        return 1;
    }
}
std::string LetterChanges(const std::string& str)
{
    std::vector<char>cstr(str.c_str(),str.c_str()+str.size());
    for(auto &element : cstr)
    {
        if((element >= 'A' && element < 'Z')||(element >='a' && element <   'z'))
        {
            element = element+1;

            if(isVowel(element)==0)
            {
                element = std::toupper(element);
            }

        }
    }
    std::string ss(cstr.begin(),cstr.end());
    return ss;
}

Answer 1

I suppose I'm morally obliged to give the standard speech about optimization. Avoid it if you can, and measure first if you can't avoid it.

A great deal of optimization depends on what you want to optimize for. In this case, I'm going to take a stab in the dark and assume that you want to be able to transform a lot of text this way, and do it extremely quickly. Just for example, let's assume you wanted to transform many megabytes of data this way.

To that end, I'd get most of the conditional logic out of the inner loop. Instead of having tests and jumps in the inner loop, I'd build a table holding the transformation for each input, and the inner loop would just load the correct value from the table.

class changer {
    std::array<char, std::numeric_limits<unsigned char>::max()> table;
public:
    changer() { 
        char vowels[] = { 'e', 'i', 'o', 'u'};
        std::iota(table.begin(), table.end(), 0);
        std::iota(&table['a'], &table['z'], 'b');
        std::iota(&table['A'], &table['Z'], 'B');
        for (auto i : vowels)
            table[i-1] = std::toupper(i);
    }

    std::string &operator()(std::string &input) {
        std::transform(input.begin(), input.end(), input.begin(), [&](unsigned char in) { return table[in]; });
        return input;
    }
};

As an aside, I'd note that this produces results that aren't reversible--when you see a 'z' or 'Z' in the output, you have no way of knowing whether the input was a 'y'/'Y' or a 'z'/'Z'. I'd guess the real intent is that an input of 'z'/'Z' should be transformed to 'A', but that doesn't seem to be included in your description.

Without that, there's no real point in testing for the input containing an 'a' or 'A'--any of those in the input were already converted to 'b'/'B', and nothing else converts to 'a'/'A'. That (in case you care) is why my "vowels" doesn't currently contain an 'a'.

Going back to the optimization situation again: it obviously takes a little time to generate the entire transformation table that this uses. If you're transforming only a tiny number of characters (a lot less than 256) building the table may take more time than it saves in the transformation. So, if you're only transforming a few characters, this probably isn't a good choice. Going with that, if you do transform a lot of text, you want to create a changer once, then re-use it for all the text you're going to transform. Re-creating it often will likely lead to an unnecessary slow-down.

Answer 2

isVowel()

The first thing to say about isVowel() is that it should return bool, not int. And the logic should really be flipped so that it returns true or 1 if it's a vowel... not false or 0.

You are correct that there is a more elegant way to do a check like isVowel(). What you'd probably want to do is make an array of all the vowels, like this:

static constexpr auto vowels = std::array{ 'a', 'e', 'i', 'o', 'u' };

If you're not using C++17, you need to specify the type and size of the array:

static constexpr auto vowels = std::array<char, 5>{ 'a', 'e', 'i', 'o', 'u' };

Don't forget to #include <array>.

Then to find if a character is a vowel, you'd search for it in the array:

std::find(vowels.begin(), vowels.end(), element);

If the element is not in the array, std::find() will return the end iterator. So all you need to do is check for that. Putting it all together:

bool isVowel(char element) noexcept
{
    static constexpr auto vowels = std::array{ 'a', 'e', 'i', 'o', 'u' };

    return std::find(vowels.begin(), vowels.end(), element) != vowels.end();
}

Note that you can also make the function noexcept. And as of C++20, you can even make it constexpr, because std::find() is constexpr in C++20.

LetterChanges()

If your goal is efficiency, then it must be pointed out that you're doing a lot of unnecessary shuffling around of your data. You get a std::string argument, then you copy it all into a std::vector, then you copy it all again into another std::string.

There's no need for all those acrobatics. Rather than creating a vector, you can simply make cstr a std::string, and return that when you're done. Even better, rather than taking str as a std::string const&, just take it as a std::string and use that. You don't need to copy the data, not even once!

But let's go through the function line-by-line first.

std::vector<char>cstr(str.c_str(),str.c_str()+str.size());

You don't need to go through all that hassle to copy a string's characters into a vector. You can just do:

std::vector<char> cstr{str.begin(), str.end()};

In the loop:

if((element >= 'A' && element < 'Z')||(element >='a' && element <   'z'))

This is a bit of an advanced technical point, but... you are assuming that the letters A-Z and a-z have contiguous code points. (In other words, you are assuming that if you take 'A' and add 25, you'll get 'Z'.) That's true in the most common character sets these days (ASCII and Unicode), but it's not always true.

The correct thing to do would be to have a function that works similarly to isVowel(), with an array. Something like this:

char transformLetter(char c) noexcept
{
    static constexpr auto uppercase = std::array{ 'A', 'B', 'C', /* rest of the letters */ };
    static constexpr auto lowercase = std::array{ 'a', 'b', 'c', /* rest of the letters */ };

    auto const p_uppercase = std::find(uppercase.begin(), uppercase.end(), c);
    if (p_uppercase != uppercase.end())
    {
        // c is an uppercase letter... but is it Z?
        if (++p_uppercase == uppercase.end())
            return 'Z';
        return *p_uppercase;
    }

    // c wasn't uppercase, so check lowercase.
    auto const p_lowercase = std::find(lowercase.begin(), lowercase.end(), c);
    if (p_lowercase != lowercase.end())
    {
        // c is an lowercase letter... but is it z?
        if (++p_lowercase == lowercase.end())
            return 'z';

        // Not z, but is it a vowel?
        if (isVowel(*p_lowercase))
        {
            // Since we already have upper and lowercase arrays,
            // let's just use them to do toupper cheaply.
            return uppercase[std::distance(lowercase.begin(), p_lowercase)];   
        }

        return *p_lowercase;
    }

    // Not a letter at all, so just return it unchanged.
    return c;
}

This could be simplified quite a bit, of course.

Anyway, if you don't care about exotic character encodings, and will only be using Unicode or ASCII, what you have is fine.

So if the function is simplified to remove the unnecessary copies, it should look something like:

std::string LetterChanges(std::string str)
{
    for (auto& element : str)
    {
        if ((element >= 'A' && element < 'Z') || (element >='a' && element <   'z'))
        {
            element = element+1;

            if (isVowel(element))
            {
                element = std::toupper(element);
            }
        }
    }

    return str;
}

Since you're not doing any copying or allocation at all, you can even make this function noexcept.

Summary

Use proper and consistent spacing and indentation. It makes code much easier to read and understand, and helps highlight errors.

• Use bool for true/false, not int. And don't invert logic. This is not shell scripting; 0 is false, 1 is true.
• Don't copy unnecessarily. Whenever someone complains that C++ is slow, at least 70% of the time it's because they're doing a lot of unnecessary copying. If you have a value that you want to modify, why copy it when you can modify it in place?