CODEWITHSUNDEEP
c++stringalgorithmreverse
Sharmin Aktar
Sharmin Aktar

Reputation: 59

results in stopped working when running reverse string program

I wrote the function to reverse a string in c++ but it results in "stopped working". 

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

string reverse(string s1)
{
    string s2;
    for(int i=0;i<s1.length();i++)s2[i]=s1[s1.length()-i-1];
    return s2;
}

int main()
{
    string s1,s2;

    cin>>s1;
    s2=reverse(s1);
    cout<<s2;

}

What can be the problem?

Upvotes: 2

Views: 115

Answers (5)

Vlad from Moscow
Vlad from Moscow

Reputation: 311068

The function declaration has a drawback. First of all in fact it does not reverse a string. It makes a copy of a string in the reverse order.

Also it is more efficient to declare the parameter as constant reference.

For example

std::string reverse_copy( const std::string &s );

Within the function you are using the subscript operator applied to an ampty string

string s2; // the string is empty
for(int i=0;i<s1.length();i++)s2[i]=s1[s1.length()-i-1];
                              ^^^^^

that results in undefined behaviour.

Also instead of the index of type int it is better to use an index of type std::string::size_type.

The function can be written without any explicit loop. For example

std::string reverse_copy( const std::string &s );
{
    return std::string( s.rbegin(), s.rend() );
}

If you want to use a loop then the function can look like

std::string reverse_copy( const std::string &s );
{
    std::string t;
    t.reserve( s.size() );

    for ( auto i = s.size(); i != 0; --i ) t.push_back( s[i-1] );

    return t;
}

Instead of the statement

t.push_back( s[i-1] );

you can write also

t += s[i-1];

For example

std::string reverse_copy( const std::string &s );
{
    std::string t;
    t.reserve( s.size() );

    for ( auto i = s.size(); i != 0; --i ) t += s[i-1];

    return t;
}

Upvotes: 2

pcodex
pcodex

Reputation: 1940

you can't index into a string when it's not been formed as yet.

Correct your code as follows and append to s2 rather than index into it

string reverse(string s1)
{
    string s2;
    for (int i = 0; i<s1.length(); i++)
        s2 += s1[s1.length() - i - 1];

    return s2;
}

Upvotes: 1

Bathsheba
Bathsheba

Reputation: 234785

s2 does not have a well-defined length: you are assuming it has at least the same length as s1. Specifically, the behaviour of s2[i] is undefined.

Consider the C++ standard library-based solution

std::string s2(s1);
std::reverse(s2.begin(), s2.end());

Where conceptually, I'm considering the string as a container of chars.Using standard functions means that someone viewing your code knows exactly what you're doing.

Upvotes: 0

Richard Hodges
Richard Hodges

Reputation: 69902

If you're not using standard algorithms, the problem is almost always that you're not using standard algorithms.

#include <iostream>
#include <string>
#include <algorithm>
#include <iterator>

std::string reverse(const std::string& s)
{
    std::string result;
    result.reserve(s.size());
    std::copy(s.rbegin(), s.rend(), std::back_inserter(result));
    return result;
}
int main()
{
    auto s = std::string("Hello, World");
    auto s2 = reverse(s);

    std::cout << s << std::endl;
    std::cout << s2 << std::endl;

    return 0;
}

expected result:

Hello, World
dlroW ,olleH

Upvotes: 0

H. Guijt
H. Guijt

Reputation: 3375

The problem is that you accessing string s2 outside its boundaries. You are only allowed to access characters with [] that already exist; attempting to write outside the string causes undefined behaviour.

One possible solution is to pre-allocate s2:

string s2 = s1;

Another option is to count down over s1, and then simply add new characters to the end of s2.

Upvotes: 4

Related Questions