3

I'm a novice programmer currently learning how to use C++. I'm trying to complete a challenge on CodeWars. The program is supposed to accept a string input, and remove all of the vowels contained in the string.

First, I created a character array containing the vowels in lowercase and uppercase. Then I used the std::find function to search the input. What I wanted to happen was: If it was able to find the current character in the array, it would erase the character, and start the loop over again. It was able to isolate the vowels, but when I try to return the modified string, I'm met with an out_of_range of memory location error.

I still don't quite understand how memory works, so I'd appreciate some help.

#include <string>
#include <iostream>
#include <conio.h>
#include <algorithm>

using namespace std;

string disemvowel(string str)
{

    char vowels[] = { 'a', 'e', 'i', 'o', 'u', 'A', 'E', 'I', 'O', 'U' };
    char *finder;

    for (int i = 0; i < str.length(); i++)
    {
        char active = str[i];
        finder = find(vowels, vowels + 10, active);
        if (finder != vowels + 10)
        {
            str.erase(str[i], 0);
        }
    }

    return str;
}

int main() {

    string str;
    cout << "say something \n";
    cin >> str;
    cout << disemvowel(str);

    _getch();
    return 0;
}

Thanks for the help

  • You want to use the erase-remove idiom. Look at the reference page for `remove_if` to see an example https://en.cppreference.com/w/cpp/algorithm/remove – JohnFilleau Jul 04 '20 at 23:22
  • Your use of `string::erase` is currently converting the character value of `str[i]` to an integer, and trying to erase 0 characters starting at the `str[i]`th character of `str`. The way you're using it you want to do `str.erase(i, 1)` to erase 1 character starting at index `i`. – JohnFilleau Jul 04 '20 at 23:29
  • 1
    Also I suggest using `std::begin(vowels)` instead of `vowels` and `std::end(vowels)` instead of `vowels + 10` as your range. Your code works as is, and it's a style thing, but I find the wrappers make it explicit that they're iterators. This also makes your code more robust to any changes to the size of `vowels`, which isn't likely to happen (unless of course your requirements change and now you have to include `y` and `Y`) – JohnFilleau Jul 04 '20 at 23:32
  • 1
    Eliminate the hand-coded loops for this. As stated in the previous comment, use the algorithm functions when there is an opportunity to do so. The function you want is `std::remove_if`. The title of your question is exactly the purpose of `remove_if` -- remove items from a sequence of values based on a certain criteria. – PaulMcKenzie Jul 04 '20 at 23:47
  • 2
    *I'm trying to complete a challenge on CodeWars.* -- If this a timed challenge, your solution may fail due to time-out issues. Note that you are erasing constantly from the string each time you find out there is a vowel. What if the string has a million characters, and half of them have vowels? You would be erasing a half-million times. Using `std::remove_if`, you erase only once. The `remove_if` just does swaps to move the "bad" characters to the end of the sequence. Then a single `erase` call is done at the end. – PaulMcKenzie Jul 04 '20 at 23:53

1 Answers1

6

Problem with current code:

    str.erase(str[i], 0);

This is incorrect. Let's look at string::erase function's signature in this case:

basic_string& erase( size_type index = 0, size_type count = npos );

First argument is size_type, which is basically an unsigned long integer. It is the index of the char you want to remove. Second one is also of the same type and it is the number of chars you want to remove from the index position, which is 1. What you are passing to the function is str[i] which is of char type which is incorrect.

Fixed version:

str.erase(i, 1);

Additionally:

finder = find(vowels, vowels + 10, active);

std::find returns an iterator, don't assign it char* even if it compiles. Fixed:

auto finder = find(vowels, vowels + 10, active);

You can also use the ready made algorithms from the standard template library(STL) to solve this problem in one line:

Just use remove_if along with string::erase:

  std::string str = "hello, world";
  
  str.erase (std::remove_if (str.begin (), str.end (), [](char c)
                 {
                 return c == 'a' || c == 'e' || c == 'i'
                 || c == 'o' || c == 'u' || c == 'A' || c == 'E' || c == 'I'
                 || c == 'O' || c == 'U';
                 }),
                 str.end ());
  std::cout << str; 

As mentioned by @PaulMckenzie, using the erase-remove idiom here is faster(twice as fast) than the loop variant + erase. Benchmark on quick-bench So, why is it faster?

Suppose we have the string: "Hello cpp"

Using erase(), every time we 'erase' a character, all the characters after it need to be moved back one position. In this case, we need to remove 'e' and 'o', position 1 and position 4.

  • Remove char from position 1 and move char 2 to char 8, one position back. That means we moved 7 chars.
  • Remove char from position 4 and move chars 5 to char 8 one position back. That means we moved 4 chars.
  • Total: 11 chars moved

remove works differently. It simply just moves back the elements that are not to be removed in the beginning, possibly overwriting the ones that are going to be removed. This results in much less moving around of elements making it faster.

See this SO Post for a more detailed and better explanation.

Note: You need #include <algorithm> for this.

Waqar
  • 8,558
  • 4
  • 35
  • 43
  • 1
    You could also explain why the second method using `remove_if` more than likely runs faster than the OP's method (not measured, but observable). – PaulMcKenzie Jul 05 '20 at 00:06