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

Added 'splice' function to the string object #2642

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

Added 'splice' function to the string object #2642

wants to merge 3 commits into from

Conversation

AmatanHead
Copy link

A splice function for strings could be useful. It takes string and inserts another string before the specified position.

@arian
Copy link
Member

arian commented Aug 18, 2014

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'
Copy link
Member

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'

Copy link
Member

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.

@ibolmo ibolmo added this to the 1.6.0 milestone Aug 18, 2014
@ibolmo
Copy link
Member

ibolmo commented Aug 18, 2014

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.

@ibolmo
Copy link
Member

ibolmo commented Aug 18, 2014

Also, this is missing specs. I would assume that the specs would need to conform to Array.splice's specs (or at least generally). totally skipped the last file. gj!

},

splice: function(pos, rem, ins) {
return (this.slice(0,pos) + ins + this.slice(pos + Math.abs(rem)));
Copy link
Member

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

Copy link
Member

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.

Copy link

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

@arian
Copy link
Member

arian commented Aug 18, 2014

String#splice should obviously work the same as Array#splice. Too bad Array.prototype.splice.call('abc', 1, 0) doesn't work on strings in JS.

@arian
Copy link
Member

arian commented Aug 18, 2014

One difference, looking at Array#splice, is that it returns an array of the removed items.
I think for here this should be very unintuitive, and not really possible.

@AmatanHead
Copy link
Author

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');
});
Copy link
Member

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?

Copy link
Member

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?

Copy link
Member

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?

@megawac
Copy link

megawac commented Aug 18, 2014

You can also just delegate to splice?

var t = this.split("");
t.splice(pos, rem, ins);
return t.join("");

@ibolmo
Copy link
Member

ibolmo commented Aug 18, 2014

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'

@arian
Copy link
Member

arian commented Aug 18, 2014

@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.

@AmatanHead
Copy link
Author

If so, I would prefer to split this operation into paste(what, optPosition), insert(what, optPosition) and delete(length, optPosition).

'abc'.paste('q') // 'qabc'
'abc'.paste('q', 1) // 'aqbc'
'abc'.insert('q') // qbc
'abc'.insert('q', 1) // aqc
'abc'.remove(1) // 'bc'
'abc'.remove(1, 1) // 'ac'

[bad idea]

@AmatanHead
Copy link
Author

... or, as suggests @megawac, just delegate it. May even better:

splice: function() {
    var t = this.split("");
    console.log(t.splice.apply(t, arguments));
    return t.join("");
}

or return two arguments:

splice: function() {
    var t = this.split(""),
        change = t.splice.apply(t, arguments).join(""),
        out = t.join("");
    return [out, change];
}

"12345".splice(1, 2, 'Q', 'W', 'ERTY') // ["1QWERTY45", "23"]

@AmatanHead
Copy link
Author

So here's a new implementation. Now it's really close to array.splice because it's based on array.splice.

  • The syntax is very similar to array.splice
  • Negative positions are explicitly controlled.
  • It's available to get removed string.

@AmatanHead
Copy link
Author

Is something wrong to this PR? Why so silent here?

@SergioCrisostomo
Copy link
Member

@AmatanHead really nice PR!

@ibolmo, @arian anything you would like to comment, discuss or add here?

@nfroidure
Copy link

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.

@olvlvl
Copy link
Contributor

olvlvl commented Sep 11, 2014

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)

str is still "Madonna" when it should be "Maonna", but arr is [ 1, 2, 4 ] as expected.

The function should be renamed as slice, although that function already exists:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/slice

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

Successfully merging this pull request may close these issues.

7 participants