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

Add reverse method #124

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

davelnewton
Copy link

Used following as reference to choose the simplest method which is generally fast.

Feel free to override that decision.

@davelnewton
Copy link
Author

I reverted the minified file based on your comment on #88

@jprichardson
Copy link
Owner

Thank you! Would you be willing to update the README as well with the new method? Thanks again!

@davelnewton
Copy link
Author

Of course; sorry!

I accidentally stripped some trailing whitespace--hope that's okay.

I also dorked and this change is on my master branch, but that should be ok.

@jprichardson
Copy link
Owner

Thank you.

I accidentally stripped some trailing whitespace--hope that's okay.

No problem.

Ok, I'm not sure how I feel about the null and undefined case. It seems to me that any method that is expected to return a string should always return a string, but I need to do my due diligence to verify that that principle exists in all of the other methods... I suppose the result could be ''. What are your thoughts on these cases? What about in the case of {} of []? cc: @az7arul for his thoughts as well.

@az7arul
Copy link
Collaborator

az7arul commented Dec 21, 2014

I agree @jprichardson for null and undefined it should either be ' ' or throw an error.

As for [] I think it makes sense to use ' ' also because [].toString() is ' '
And for {} it should be ']tcejbO tcejbo[' since {}.toString() is '[object Object]'

…ring, arrays and objects return what you'd expect
@davelnewton
Copy link
Author

The [] and {} cases are already handled by (magic, I assume). I added test cases to show this; I didn't alter my reverse code for those.

I updated null and undefined handling to return empty strings, although I"m not convinced it's the best approach.

@jprichardson
Copy link
Owner

I updated null and undefined handling to return empty strings, although I"m not convinced it's the best approach.

Fair enough. What do you think should be the case for null and undefined?

@davelnewton
Copy link
Author

Not sure. My only concern is that letting a null through unannounced could lead to a bug. I'd probably prefer llun and denifednu or something.

At the same time, I'm pretty ambivalent.

@ghost
Copy link

ghost commented Feb 27, 2016

Sorry for the necro post. Personally, I think that returning null for both null and undefined would be best. Returning null implies that no data is available to be returned and returning undefined means a variable was not set. Returning back an empty string, llun, or denifednu just doesn't make much sense if the internal object is null or undefined as you would expect a member of an object that is null or undefined to return undefined if accessed. Accessing members of an empty object or an empty string will not fail and may actually cause bugs to go undiscovered.

@davelnewton
Copy link
Author

Oh I'd forgotten all about this.

Still, if the functionality hasn't been added yet, can someone make an executive decision on handling this and merge it in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants