Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

substr and replace methods for sf::String #355

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@abodelot
Copy link
Contributor

abodelot commented Feb 16, 2013

This is a fix for issue #21 and provides substr and replace (2 versions) methods for sf::String.

The substr and the first replace methods are pretty much equivalent to their std::string counterparts.

// Generate a substring
String substr(std::size_t pos = 0, std::size_t len = InvalidPos) const;
// Replace a portion of the string
void replace(std::size_t pos, std::size_t len, const String& str);

The second replace method allows user to replace a substring by a replacement value.

// Replace all occurrences of a substring with a replacement string
String replace(const String& search, const String& replacement) const;

Please find bellow a test case for these new methods:

#include <SFML/System/String.hpp>
#include <cassert>

int main()
{
    // Test sf::String::substr
    sf::String string("Hello, World!");
    assert(string.substr() == string);
    assert(string.substr(0) == string);
    assert(string.substr(0, 5) == "Hello");
    assert(string.substr(7, 5) == "World");
    assert(string.substr(7) == "World!");
    assert(string.substr(7, 9000) == "World!");

    // Test sf::String::replace (1)
    string.replace(0, 5, "Hi");
    assert(string == "Hi, World!");
    string.replace(4, sf::String::InvalidPos, "SFML!");
    assert(string == "Hi, SFML!");

    // Test sf::String::replace (2)
    string = "foobar foobaz barbaz";
    assert(string.replace("foo", "f") == "fbar fbaz barbaz");
    assert(string.replace("baz", "foobar") == "foobar foofoobar barfoobar");
    return 0;
}

Regards,

-- Alexandre

@abodelot

This comment has been minimized.

Copy link
Contributor Author

abodelot commented Feb 23, 2013

Edited last commit (removed useless sf::).

Any chance to see this merged or is this not suitable?

@LaurentGomila

This comment has been minimized.

Copy link
Member

LaurentGomila commented Feb 23, 2013

It looks ok, but this is not one of my priorities. I keep it for later :)

@abodelot

This comment has been minimized.

Copy link
Contributor Author

abodelot commented Feb 24, 2013

Ok, I guess you're already busy enough with finalizing SFML 2.0 :)
Thanks for your feedback.

@ghost ghost assigned LaurentGomila Jul 30, 2013

Bromeon added a commit that referenced this pull request Feb 6, 2014

Added String::replace() methods
Based on pull request #355 from abodelot
@Bromeon

This comment has been minimized.

Copy link
Member

Bromeon commented Feb 6, 2014

I added two replace() overloads. I changed the signature from

String replace(const String& searchFor, const String& replaceWith) const

to

void replace(const String& searchFor, const String& replaceWith)

since this is more consistent and allows in-place modifications.

Concerning substring, the functionality is now available through String::fromUtf32() taking iterators. So a substring function would only exist for convenience. I'm also thinking about the name; while substr is standard, it doesn't really match the naming of other functions like isEmpty or getSize. Alternatives are substring, subString, getSubstring, ...?

@MarioLiebisch

This comment has been minimized.

Copy link
Member

MarioLiebisch commented Feb 6, 2014

I think it should follow the behavior of std::string::replace() and return a reference as well - not just void.

@Bromeon

This comment has been minimized.

Copy link
Member

Bromeon commented Feb 6, 2014

The return type of replace() is consistent with erase() and insert(). Those methods don't return a reference either, although the std::string equivalents do.

@abodelot

This comment has been minimized.

Copy link
Contributor Author

abodelot commented Feb 6, 2014

Thanks for giving it a look, it's been a while.

I changed the signature [...] since this is more consistent and allows in-place modifications.

I'm ok with that.

Regarding substr I think many users will miss the fact that fromUtf32 can achieve the same feature, as the name leads to think it's only about converting encoding. Regarding naming conventions, getSubstring is the one that matches other SFML methods the best in my opinion ; they all use full names.

@Bromeon

This comment has been minimized.

Copy link
Member

Bromeon commented Feb 6, 2014

I've looked at other popular libraries with their own string types (C++ GUI frameworks and graphics engines, Java Class Library, .NET framework). substring and its uppercase variant seem to be a very widespread convention. Some call the method mid, but I've not seen getSubstring anywhere. It's not a typical getter anyway.

So I'll go with a simple substring.

Bromeon added a commit that referenced this pull request Feb 9, 2014

Added String::substring() method
Based on pull request #355 from abodelot
@Bromeon

This comment has been minimized.

Copy link
Member

Bromeon commented Feb 9, 2014

String::substring() is now implemented, too. I omitted the first default parameter; I don't think people who need a copy of the whole string are going to call substring().

@Bromeon Bromeon closed this Feb 9, 2014

@Bromeon Bromeon assigned Bromeon and unassigned LaurentGomila Feb 9, 2014

@Rapptz

This comment has been minimized.

Copy link

Rapptz commented Feb 12, 2014

Personally, I would have stuck with substr due to the fact that it aids generic code (i.e. it works with both std::string and sf::String). Though I guess maybe in this case it probably doesn't matter as much -- it still feels like a small loss.

MarioLiebisch added a commit to MarioLiebisch/SFML that referenced this pull request Feb 12, 2014

Added String::replace() methods
Based on pull request SFML#355 from abodelot

MarioLiebisch added a commit to MarioLiebisch/SFML that referenced this pull request Feb 12, 2014

Added String::substring() method
Based on pull request SFML#355 from abodelot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.