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

Make Array#swap dangerous #3741

Closed
wants to merge 1 commit into from

Conversation

tylerrash
Copy link

By modifying self in place, Array#swap was "dangerous". I've moved that functionality to Array#swap! and created a new Array#swap that operates on a duplicate array.


# Returns a new array with elements at `index0` and `index1` swapped. If the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argument names should be enclosed in * characters.

@asterite
Copy link
Member

Not sure about this. We also have Array#insert, but it's not insert!. I'm not sure there's a good use case for swapping elements in an array for a copy...

@bcardiff
Copy link
Member

I am not sure either, is there a common scenario where you would use a "swap & copy"? Usually when swapping elements we want to do it in-place. And if we only have one operation then #swap is fine.

The bang (!) does not mean "destructive" nor lack of it mean non destructive either. The bang sign means "the bang version is more dangerous than its non bang counterpart; handle with care".
https://www.ruby-forum.com/topic/176830#773946

@tylerrash
Copy link
Author

Fair enough. I don't know that there's common scenario where swapping in a copy is needed, but I ran into one and was surprised by Array#swap. If the consensus is that not every destructive method needs a non-destructive sibling, I'll close this.

@asterite
Copy link
Member

Yes, not every destructive methods needs a non-destructive method. And in particular, swap is often used to efficiently swap elements, and duping the array kind of defeats that purpose :-)

Thank you in any case for brining up this discussion.

@asterite asterite closed this Dec 20, 2016
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.

None yet

4 participants