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

Ability to mix longest buffer #7

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

Conversation

Jokab
Copy link

@Jokab Jokab commented May 10, 2017

Hey, I implemented the feature I requested myself since I figured you were busy. I added tests to make sure it works too. I decided to use a boolean parameter to decide whether to mix or not, added docs too.

@dy
Copy link
Member

dy commented May 11, 2017

@Jokab thank you for the PR! I am not sure though the API needs that sort of complication. Why don't you just compare which buffer is longer and mix shorter one into longer one?

@Jokab
Copy link
Author

Jokab commented May 11, 2017

@dfcreative I believe what you are suggesting does not work when there is an offset involved.

If, for example, you have two buffers A and B, of length 4 and 3 respectively, and an offset of 2. What happens currently is that B[0] is mixed at A[2], B[1] is mixed at A[3], and then the mixing loop stops because i < bufferA.length is violated in the code.

As you can see, B is indeed shorter than A, but it still doesn't behave as I would expect it to. The expected behavior, according to me, would be that B[2] is put at A[5], which would indeed extend A by one element. This is the functionality that my PR extends.

@dy
Copy link
Member

dy commented May 11, 2017

@Jokab oh, I see, makes sense.
But that is just making padding before mixing, right? Isn't explicit code better than mysterious true argument?

//short & mysterious
a = u.mix(a, b, .5, offset, true)

//long & explicit
a = u.pad(a, Math.max(a.length, b.length) + offset)
u.mix(a, b, .5, offset)

Also in your PR mix now behaves mutable/immutable depending on the longest argument: in first case it modifies a, in second case it returns a new buffer, which may be confusing. With pad we make sure returned buffer is a new one.

Also, this argument is not treated properly (yet).

mix(a, b, true)
mix(a, b, .5, true)
mix(a, b, .5, offset, true)

As a possible solution, I would think on detecting longest by comparing available lengths rather than a separate argument, and forcing immutable result (which is not always desirable though).

@Jokab
Copy link
Author

Jokab commented May 15, 2017

@dfcreative Yeah, I suppose you are right that works too. (Though, you need to do u.pad(a, Math.max(a.length, b.length+offset))) It all depends on how you want to design the API, which of course you decide. My opinion is that my PR is more intuitive (with your suggestions applied), but in the end it's your decision of course.

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.

2 participants