-
Notifications
You must be signed in to change notification settings - Fork 511
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
Added 'splice' function to the string object #2642
base: master
Are you sure you want to change the base?
Conversation
Your pull request looks really good! Also there is no doubt this function can be useful. The question is whether this should be in -core, or somewhere else. Let's hear what others have to say... |
### Examples: | ||
|
||
'Hello, big world!'.splice(6, 4, ''); // returns 'Hello, world!' | ||
'Add here'.splice(3, 0, ' substring'); // returns 'Add substring here' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem intuitive. One could argue this would have been the result: 'Add substring'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's suppose to mimic Array.splice
... just not exactly very readable.
If this is useful for anyone else, then we can go ahead and pull into Core. I'm arguing the readability, but it's a moot argument if others pick up the idea. |
|
}, | ||
|
||
splice: function(pos, rem, ins) { | ||
return (this.slice(0,pos) + ins + this.slice(pos + Math.abs(rem))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after 0, pos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it should work the same as Array#splice, you should be able to insert all rest arguments in between.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't ins
be optional
String#splice should obviously work the same as Array#splice. Too bad |
One difference, looking at Array#splice, is that it returns an array of the removed items. |
I see two ways to fix this: first is to try to fully mimic array.splice, second is to brake intuitive similarity and invite something new. |
expect('Add here'.splice(3, 0, ' substring')).toEqual('Add substring here'); | ||
expect('Replace this word'.splice(8, 4, 'that')).toEqual('Replace that word'); | ||
expect('You can count from an end'.splice(-6, 2, 'the')).toEqual('You can count from the end'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if remove
was < 0
?
What if remove
and insert
was missing?
What if position
, remove
, and insert
were missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if remove
was > this.length
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if position
was > this.length
? Pad with empty string?
You can also just delegate to splice? var t = this.split("");
t.splice(pos, rem, ins);
return t.join(""); |
Re: the stick to Array.splice mimicry vs. venture into splice-like I would start with the Specs, and define the questions and answers. I'd personally prefer to write something like: 'Some string'.splice('my other string', 5); // 'Some my other string'
'abc'.splice(' ', 1); // 'a c'
'abc'.splice(' ', 1, 2); // 'a c'
myString.splice(withAnother, optStartPosition, optLength);
'abc'.splice('def'); // 'abcdef' |
@ibolmo what if you just want to remove a part of the string, and don't want to insert something new, then rather than passing an empty string/value (null/undefined), the replacement string should be the last argument, like it is in Array.splice... Then it's really optional. |
If so, I would prefer to split this operation into paste(what, optPosition), insert(what, optPosition) and delete(length, optPosition).
[bad idea] |
... or, as suggests @megawac, just delegate it. May even better:
or return two arguments:
|
So here's a new implementation. Now it's really close to array.splice because it's based on array.splice.
|
Is something wrong to this PR? Why so silent here? |
@AmatanHead really nice PR! @ibolmo, @arian anything you would like to comment, discuss or add here? |
splice is a modificative function for arrays. having splice on strings would assume it either is. But strings cannot be modified in JavaScript, you still get a new string. If you really need this method, IMO, it should be slice, not splice. |
I share the opinion of @nfroidure. Consider the following code: String.implement({
splice: function() {
arguments[0] += (arguments[0] < 0 ? this.length + 1 : 0);
var return_both = (typeof arguments[arguments.length - 1] === 'boolean' ? arguments[--arguments.length] : false),
t = this.split(""),
change = t.splice.apply(t, arguments).join(""),
out = t.join("");
return (return_both ? [out, change] : out);
}
})
var str = "Madonna"
, arr = [ 1, 2, 3, 4 ]
str.splice(2, 1)
arr.splice(2, 1)
console.log(str)
console.log(arr)
The function should be renamed as https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/slice |
A splice function for strings could be useful. It takes string and inserts another string before the specified position.