trying to make function that replace a with e, e with i, i with o, o with u and u with a in a string

Question

string rotate_vowels(string& s) {
s = "computer";
char vow[] = { 'a', 'e', 'i', 'o', 'u' };
for (int i = 0; i < (int)s.length(); i++)
{
    for (int j = 0; j <= 4;)
    {
        if (s[i] == vow[j])
        {
        
            s[i] = vow[(j + 1)];

        }
        else
        {
            j++;
        }
    }
}
return s;
}

int main()
{
     string p;
     cout << rotate_vowels(p);
}

I tried using an array with all the vowels and iterate through it to check if a character of the string is equal to it.

Answer 1

The biggest issue is that you are going out of bounds here s[i] = vow[(j + 1)];. This invokes undefined behavior and the output of your code could in principle be anything.

There are some other minor issues. I suppose this is just due to testing the function, but assigning to the parameter in the function makes it impossible to test with other input than just "computer" (it is desirable to test is with different input without changing anything on the function). You don't need to pass the parameter by non-const reference and return the result. Doing both can be confusing, choose one of it. Also it looks like you fixed a warning due to signed-unsigned comparison, but you got the fix the wrong way around, use size_t for indices. Or rather use a range based loop.

If you fix the out-of bounds you will still face the problem, that the inner loop will replace a with e, on the next iteration it will increment j, next iteration it will replace e with i, and so on. You need to break out of the loop after replacing a character once.

To solve the out-of-bounds you can use %. A more general approach is to use a std::unordered_map:

#include <string>
#include <iostream>
#include <unordered_map>

std::string rotate_vowels(std::string s) {
    std::unordered_map<char,char> repl{{'a','e'},{'e','i'},{'i','o'},{'o','u'}};

    for (auto& c : s){
        auto it = repl.find(c);
        if (it != repl.end()) c = it->second; // if the character was found replace it
    }
    return s;
}


int main() {
     std::cout << rotate_vowels("computer");
}

Answer 2

If build your program with address sanitizer and run it, we get such error report:

stack-buffer-overflow on address 
rotate_vowels(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) /2rotate_vowels.cpp:12

To fix it we can add a modulus to avoid the index larger than the array size. Add need to break early if we get matched, to avoid reset the matched value again.

#include <iostream>
#include <string>
using namespace std;

string rotate_vowels(string& s) {
  s = "computer";
  char vow[] = {'a', 'e', 'i', 'o', 'u'};
  size_t len = std::size(vow);
  for (int i = 0; i < (int)s.length(); i++) {
    for (int j = 0; j < len; ++j) {
      if (s[i] == vow[j]) {
        s[i] = vow[(j + 1) % len];
        break;
      }
    }
  }
  return s;
}

int main() {
  string p;
  cout << rotate_vowels(p);
}

It would be clear to using a mapping here, generally, we can use std:unordered_map, but in this case, use a local array is enough since we can use the char type as an index, the benefit we get:

• faster than the loop used in your code since we use a table that looks up directly

• faster than the std::unordered_map version, and with less memory footprint

#include <iostream>
#include <string>


std::string rotate_vowels(std::string& s) {
  char vow[128] = {0};
  vow['a'] = 'e';
  vow['e'] = 'i';
  vow['i'] = 'o';
  vow['o'] = 'u';
  vow['u'] = 'a';
  for (size_t i = 0; i < s.length(); i++) {
    if (vow[s[i]]) {
      s[i] = vow[s[i]];
    }
  }
  return s;
}

int main() {
  std::string p = "computer";
  std::cout << rotate_vowels(p);
  return 0;
}

Online demo

---

Add the benchmark result for these three versions:

#include <benchmark/benchmark.h>

#include <functional>
#include <iostream>
#include <map>
#include <numeric>
#include <random>
#include <string>
#include <thread>
#include <unordered_map>
#include <vector>

std::string random_string(std::string::size_type length) {
  static auto& chrs =
      "0123456789"
      "abcdefghijklmnopqrstuvwxyz"
      "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

  thread_local static std::mt19937 rg{std::random_device{}()};
  thread_local static std::uniform_int_distribution<std::string::size_type>
      pick(0, sizeof(chrs) - 2);

  std::string s;

  s.reserve(length);

  while (length--) s += chrs[pick(rg)];

  return s;
}

void rotate_vowels_loop(std::string& s) {
  char vow[] = {'a', 'e', 'i', 'o', 'u'};
  for (int i = 0; i < (int)s.length(); i++) {
    for (int j = 0; j < 5; ++j) {
      if (s[i] == vow[j]) {
        s[i] = vow[(j + 1) % 5];
        break;
      }
    }
  }
}

void rotate_vowels_array(std::string& s) {
  char vow[128] = {0};
  vow['a'] = 'e';
  vow['e'] = 'i';
  vow['i'] = 'o';
  vow['o'] = 'u';
  vow['u'] = 'a';
  for (size_t i = 0; i < s.length(); i++) {
    if (vow[s[i]]) {
      s[i] = vow[s[i]];
    }
  }
}

void rotate_vowels_unordered(std::string& s) {
  static std::unordered_map<char, char> repl{
      {'a', 'e'}, {'e', 'i'}, {'i', 'o'}, {'o', 'u'}};

  for (auto& c : s) {
    auto it = repl.find(c);
    if (it != repl.end()) {
      c = it->second;
    }
  }
}
static void BM_array_loop(benchmark::State& state) {
  std::string str = random_string(state.range(0));
  for (auto _ : state) {
    rotate_vowels_loop(str);
    benchmark::DoNotOptimize(&str);
  }
}

static void BM_array_mapping(benchmark::State& state) {
  std::string str = random_string(state.range(0));
  for (auto _ : state) {
    rotate_vowels_array(str);
    benchmark::DoNotOptimize(&str);
  }
}
static void BM_unordered_map(benchmark::State& state) {
  std::string str = random_string(state.range(0));
  for (auto _ : state) {
    rotate_vowels_unordered(str);
    benchmark::DoNotOptimize(&str);
  }
}
BENCHMARK(BM_array_loop)->Arg(8)->Arg(64)->Arg(512)->Arg(1 << 10)->Arg(8 << 10);
BENCHMARK(BM_array_mapping)->Arg(8)->Arg(64)->Arg(512)->Arg(1 << 10)->Arg(8 << 10);
BENCHMARK(BM_unordered_map)
    ->Arg(8)
    ->Arg(64)
    ->Arg(512)
    ->Arg(1 << 10)
    ->Arg(8 << 10);

Result:

We can see that the local array mapping version is faster than the version from OP and much faster than the std::unordered_map version

Answer 3

The problem with your code is that you are using s[i] = vow[(j+1)] to replace the vowel but if s[i] = 'u', then j+1 = 6 which is out of range of the vow array.
You can create a map which can contain the vowels as keys and "vowels to replace with" as values. For example:

map<char, char> vow = {{'a', 'e'}, {'e', 'i'}, {'i', 'o'}, {'o', 'u'}, {'u', 'a'}};

'a' is replaced by 'e', 'e' is replace by 'i' and so on.

Then traverse the given string and if the current character is a vowel, then replace it is replaced by its respective value in the map.

#include <iostream>
#include <string>
#include <map>
using namespace std;

// function to rotate vowels
string rotate_vowels(string s){
    // create a map
    map<char, char> vow = {{'a', 'e'}, {'e', 'i'}, {'i', 'o'}, {'o', 'u'}, {'u', 'a'}};
    
    string rotated_string = s;
    
    // traverse the string
    for(int i=0;i<s.length();i++){
        // if char is vowel, rotate it
        if(vow.find(rotated_string[i]) != vow.end()){
            rotated_string[i] = vow[rotated_string[i]];
        }
    }
    
    // return the rotated string
    return rotated_string;
}

int main() {
    string s = "computer";
    cout<<rotate_vowels(s);
    return 0;
}

Since you will be traversing only the string and searching in map takes O(1), time complexity of the above code will be O(N).

Answer 4

You're going out of bounds, because you're not dealing with the "loopback" where you're comparing with 'u', but the next item in the list isn't there.

There are plenty of clever ways to deal with this, but you could simply add 'a' again to your list, and your program will work, this will deal with the "loopback".

#include <array>
#include <iostream>
#include <string>

std::string rotate_vowels(std::string& s) {
  std::array vowels = {'a', 'e', 'i', 'o', 'u', 'a'};
  for (int i = 0; i < s.length(); i++) {
    /* Looping to vowels.size() - 1, because we already deal with 'a' */
    for (std::size_t j = 0; j < vowels.size() - 1; j++) {
      if (s[i] == vowels[j]) {
        s[i] = vowels[j + 1];
        break; /* A replacement has been done, move on to next letter immediately */
      }
    }
  }
  return s;
}

int main() {
  std::string p("computer");
  std::cout << rotate_vowels(p);
}

Answer 5

This seems like your homework, but I will do it for you and explain you the steps. My output is:

cumpatir

#include <iostream>
#include <string>

using namespace std;

string rotate_vowels(const string& s) 
{
    string copy_of_s = s;
    char vow[] = { 'a', 'e', 'i', 'o', 'u' };
    const size_t size_of_vowels = 5;

    for(size_t i = 0; i < copy_of_s.length(); i++)
    {
        for(size_t j = 0; j < size_of_vowels; j++)
        {
                if (s[i] == vow[j])
                {
                    copy_of_s[i] = vow[ (j + 1) %  size_of_vowels]; //make sure you dont access the element after u, which does not exist
                    break;
                }
            }
    }
    return copy_of_s;
}

int main()
{
     string p = "computer";
     cout << rotate_vowels(p) << endl;
}

Explanation:

#include <iostream>
#include <string>

This part tells the compiler which functions and classes to use. In this case the input / output functions (iostream) and the string functions.

using namespace std;

This makes sure you don't have to use std::string and you can just write string.

string rotate_vowels(const string& s) 

If you don't change a value make it const. It make your programm run faster and you can not change it be accident.

const size_t size_of_vowels = 5;

Use variables for the size of arrays. Even better use Vectors since you are in C++ or use this MAKRO if you like C Style coding. Common array length macro for C?

for(size_t i = 0; i < copy_of_s.length(); i++)

Use size_t instead of int. size_t is like an int but it can't get negative. And on almost on Plattforms (x86, arm, etc.) it is way bigger than int, so it is safer to use.

break;

Use break to end the vowel loop, when a replacement was found (like a becomes an e).

copy_of_s[i] = vow[ (j + 1) %  size_of_vowels]; //make sure you dont access the element after u, which does not exist

This line is tricky. You have 5 Vowels char vow[] = { 'a', 'e', 'i', 'o', 'u' };. vow[0] is 'a', vow[4] is 'u'. What happens you have vow[4+1]? You would access vow[5] which does not exist. This can lead to a crash or random data which confuses your programm. To solve this problem you can use

vow[(4+1)%5]

The is a division where you only keep the remainder. Sounds complicated but is quite easy. 3 % 5 = 0 remainder 3 (the 5 fits zero times into the 3). 5 % 5 = 1 Remainder 0 (the 5 fits one time into the 5, but nothing is left). 7 % 5 = 1 Remainder 2 (you get the idea).

You ignore the 1 in 7 % 5 = 1 Remainder 2, and just keep the two. So the result for 7 % 5 is 2. This way you can "wrap around" your array index and never access vow[5]. Because vow[5%5] is vow[0], so your 'u' becomes an 'a'.