remove_if last character from a string

Question

I would like to remove first and last brackets in passed by reference string. Unfortunately, I have difficulties with removing first and last elements conditionally. I cannot understand why remove_if doesn't work as I expect with iterators.

Demo

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

void print_wo_brackets(string& str){
    auto detect_bracket = [](char x){ return(')' == x || '(' == x);};
    if (!str.empty()) 
    {
        str.erase(std::remove_if(str.begin(), str.begin() + 1, detect_bracket));
    }
    if (!str.empty()) 
    {
        str.erase(std::remove_if(str.end()-1, str.end(), detect_bracket));
    }
}

int main()
{
    string str = "abc)";
    cout << str << endl;
    print_wo_brackets(str);
    cout << str << endl;


    string str2 = "(abc";
    cout << str2 << endl;
    print_wo_brackets(str2);
    cout << str2 << endl;

    return 0;
}

Output

abc)
ac    <- HERE I expect abc 
(abc 
abc

This answer may be useful (also other answers may suit) http://stackoverflow.com/a/25385766/3807729 — Galik, Apr 20 '16 at 09:49
basically, remove_if does not change the size of the container. Instead it returns the new end. You have to update the string manually to reflect the change. Using it on the first character will probably make a mess of the string. — kuroi neko, Apr 20 '16 at 09:58

marcinj · Accepted Answer · 2016-04-20T10:03:57.797

6

If remove_if returns end iterator then you will try to erase nonexistent element. You should use erase version for range in both places:

void print_wo_brackets(string& str){
    auto detect_bracket = [](char x){ return(')' == x || '(' == x);};
    if (!str.empty())
    {
        str.erase(std::remove_if(str.begin(), str.begin() + 1, detect_bracket), str.begin() + 1);
    }
    if (!str.empty())
    {
        str.erase(std::remove_if(str.end()-1, str.end(), detect_bracket), str.end());
    }
}

edited Apr 20 '16 at 10:03

answered Apr 20 '16 at 09:56

marcinj

48,511
9
79
100

This is actually a valid answer, but in the example `end` points to letter `b` (as it's a typical pointer, most probably) – Tomasz Lewowski Apr 20 '16 at 10:00
It remove UB but - you are right, this wont make your code run correct - other bugs still exists – marcinj Apr 20 '16 at 10:02
@CronAcronis - first `str.erase` used wrong end iterator range, now it should work – marcinj Apr 20 '16 at 10:05
You must also check for cases when no brackets are present, string is of length 1,... – marcinj Apr 20 '16 at 10:09

Tomasz Lewowski · Answer 2 · 2016-04-20T10:08:42.223

The problem is here:

if (!str.empty()) 
{
    str.erase(std::remove_if(str.begin(), str.begin() + 1, detect_bracket));
}

you erase unconditionally. std::remove_if returns iterator to the beginning of range "for removal". If there are no elements for removal, it returns end of range (str.begin() + 1 in this case). So you remove begin+1 element, which is b.

To protect from this problem you shouldn't probably do something more like:

if (!str.empty()) 
{
    auto it = std::remove_if(str.begin(), str.begin() + 1, detect_bracket);
    if(it != str.begin() + 1)
        str.erase(it);
}

I assume you simply want to check behavior of standard library and iterators, as otherwise check:

if(str[0] == '(' || str[0] == ')')
    str.erase(0);

is much simpler.

But in case of `str.erase(std::remove_if(str.begin(), str.end, detect_bracket));` it will remove all brackets. — Cron Merdek, Apr 20 '16 at 10:08

score 3 · Answer 3 · answered Apr 20 '16 at 10:00

Alternative:

#include <iostream>
#include <string>

std::string without_brackets(std::string str, char beg = '(', char end = ')') {
    auto last = str.find_last_of(end);
    auto first = str.find_first_of(beg);

    if(last != std::string::npos) {
        str.erase(str.begin()+last);
    }
    if(first != std::string::npos) {
        str.erase(str.begin()+first);
    }

    return str;
}


using namespace std;

int main() {
    cout << without_brackets("abc)") << endl
         << without_brackets("(abc") << endl
         << without_brackets("(abc)") << endl
         << without_brackets("abc") << endl;
    return 0;
}

see: http://ideone.com/T2bZDe

result:

abc
abc
abc
abc

Nice solution, but not with `remove_if` – Cron Merdek Apr 20 '16 at 10:07 — Cron Merdek, Apr 20 '16 at 10:07
@CronAcronis - exactly. `remove_if` is the wrong tool here. – Pete Becker Apr 20 '16 at 12:45 — Pete Becker, Apr 20 '16 at 12:45

score 2 · Answer 4 · answered Apr 20 '16 at 12:57

As stated in the comments by @PeteBecker, remove_if is not the right algorithm here. Since you only want to remove the first and last characters if they match, a much simpler approach is to test back() and front() against the two parentheses ( and ) (brackets would be [ and ])

void remove_surrounding(string& str, char left = '(', char right = ')')
{
    if (!str.empty() && str.front() == left)
        str.erase(str.begin());
    if (!str.empty() && str.back() == right)
        str.erase(str.end() - 1);
}

Live Example

101010 · Answer 5 · 2016-04-20T10:13:32.570

1

All you need is this:

void print_wo_brackets(string& str){
  str.erase(std::remove_if(str.begin(), str.end(),
    [&](char &c) { return (c == ')' || c == '(') && (&c == str.data() || &c == (str.data() + str.size() - 1));}), str.end());
}

Live Demo

By stating:

str.erase(std::remove_if(str.end()-1, str.end(), detect_bracket));

You're evoking undefined behaviour.

edited Apr 20 '16 at 10:13

answered Apr 20 '16 at 10:01

101010

41,839
11
94
168

remove_if last character from a string

Output

5 Answers5