Some suggested improvements

[jor-alv]

Hi there @sogoagain!

I have been playing around with scrambling-text-js (it looks great) and would love to help you make some improvements to it, namely:

• Add some additional built-in character choices,
• Add some additional options for users to more easily customize and control the animation,
• Remove duplicate variable initializations and make other minor clean-ups.

I have created a new branch to keep changes separated from the main branch and have split the proposed changes into five different commits to make them easier for you to review. None of them are breaking changes and I have tried to make commit messages as self-explanatory as possible.

Also, I have run both the original tests and some additional tests I have added, and run the animation locally, and everything works OK.

Cheers!

[sogoagain]

Hi, thank you so much for your contribution! It's great to see these improvements, and the overall usability seems much better. The changes look great, but I’ve left a few comments on some areas that could be improved. Please feel free to check them out and share your thoughts!

[sogoagain]

How about adding validation and default value assignment for the new maxCounter and delay inputs? This could help ensure that if a user inputs a negative or very large value, a more reasonable default value is used.

[jor-alv]

Default values are already assigned in lines 22 and 23. But I like your idea about also adding input validation. Some thoughts:

• delay appears to work the same with negative values and break only with 0 and decimals other than multiples of 0.5, so we could add the following logic in line 38: if (options.delay) this.delay = Math.round(options.delay * 2) / 2;, which would only allow negative and positive multiples of 0.5 that are different from 0.

• maxCounter on the other hand, seems to break only with negative values and not with 0 or decimals, so the validation logic in line 39 could look like: if (options.maxCounter !== undefined) this.maxCounter = Math.abs(options.maxCounter);, which would only let through values greater than or equal to 0.

I don't think validation for large values is needed. Users may want to make the animation super slow with a large delay or keep it running for a very long time with a large maxCounter. Let me know what you think.

[jor-alv]

Just tested the input validation logic I suggested above for delay and it produces inconsistent results. A delay of 3.4 seems to create a slower scrambling effect than a delay of 4, for example. So it may be a good idea to simply stick to whole numbers other than 0, with input validation looking something like: if (options.delay !== undefined) this.delay = Math.round(options.delay) || 1.

[sogoagain]

Thank you for thoroughly reviewing the input values.

Since we’re introducing a new interface that users will interact with, I want to proceed carefully. From my experience, once an interface is set, it’s hard to change.

First, I think we could improve the current parameter names to make them more intuitive for users. For example:

• delay -> speed (where a higher number means faster speed)
• maxCounter -> shuffleCount (since it represents the number of times the characters are shuffled)

Additionally, it’s important to provide clear input guidelines. Without them, users might be confused.

Based on my tests, I believe setting speed as an integer between 1 and 10 would cover most use cases. As for shuffleCount, I agree that allowing any positive integer makes sense since some users may want unlimited shuffling.

What do you think?

[sogoagain]

Originally, the values 2, 3, 4 were used as delay values to give different speeds to each animation stage (encode, fill, decode). Now that these have been unified under the delay value, what do you think about using delay - 1, delay, and delay + 1 to maintain that timing effect?

[jor-alv]

I agree (inside parentheses of course, like (this.delay - 1) and (this.delay + 1), to ensure they work as intended and that the final decodedText is the same as the original text).

[sogoagain]

It would be great if there are any use cases for the added parameters(maxCounter, delay) in the test cases.

[jor-alv]

I agree. Added the new parameters after these changes, so they are not included here. Happy to make the necessary additions. Let me know how you'd prefer to proceed.

[sogoagain]

There is currently a test case for "with the option to set the characters to use when scrambled." We could extend it to something like this:

describe('with custom characters, speed, and shuffleCount', () => {
    it('scrambles text according to the provided options', () => {
      const scrambler = new Scrambler();

      texts.forEach((text) => {
        scrambler.scramble(text, handleScramble, {
          characters: ['a', 'b', 'c'],
          speed: 2,          
          shuffleCount: 20,
        });
        expect(handleScramble).toHaveBeenLastCalledWith(text);
      });
    });
  });

This way, we can demonstrate the use cases to the users through the test cases.

Also, it would be good to add test cases for input validation of each parameter. If you encounter any difficulties, please feel free to let me know.

[jor-alv]

Hi @sogoagain!

Thanks for the quick review. I've added some further thoughts and suggestions below each comment.

Cheers!

[jor-alv]

Default values are already assigned in lines 22 and 23. But I like your idea about also adding input validation. Some thoughts:

• delay appears to work the same with negative values and break only with 0 and decimals other than multiples of 0.5, so we could add the following logic in line 38: if (options.delay) this.delay = Math.round(options.delay * 2) / 2;, which would only allow negative and positive multiples of 0.5 that are different from 0.

• maxCounter on the other hand, seems to break only with negative values and not with 0 or decimals, so the validation logic in line 39 could look like: if (options.maxCounter !== undefined) this.maxCounter = Math.abs(options.maxCounter);, which would only let through values greater than or equal to 0.

I don't think validation for large values is needed. Users may want to make the animation super slow with a large delay or keep it running for a very long time with a large maxCounter. Let me know what you think.

[jor-alv]

I agree (inside parentheses of course, like (this.delay - 1) and (this.delay + 1), to ensure they work as intended and that the final decodedText is the same as the original text).

[jor-alv]

I agree. Added the new parameters after these changes, so they are not included here. Happy to make the necessary additions. Let me know how you'd prefer to proceed.

[sogoagain]

Thank you for carefully reviewing my comments. I’ve added some detailed feedback as well.

I noticed that there are some linting errors in the current code. Could you please check them? You can run the lint check using the npm run lint command.

Thank you again for your contribution and hard work.

[sogoagain]

Thank you for thoroughly reviewing the input values.

Since we’re introducing a new interface that users will interact with, I want to proceed carefully. From my experience, once an interface is set, it’s hard to change.

First, I think we could improve the current parameter names to make them more intuitive for users. For example:

• delay -> speed (where a higher number means faster speed)
• maxCounter -> shuffleCount (since it represents the number of times the characters are shuffled)

Additionally, it’s important to provide clear input guidelines. Without them, users might be confused.

Based on my tests, I believe setting speed as an integer between 1 and 10 would cover most use cases. As for shuffleCount, I agree that allowing any positive integer makes sense since some users may want unlimited shuffling.

What do you think?

[sogoagain]

There is currently a test case for "with the option to set the characters to use when scrambled." We could extend it to something like this:

describe('with custom characters, speed, and shuffleCount', () => {
    it('scrambles text according to the provided options', () => {
      const scrambler = new Scrambler();

      texts.forEach((text) => {
        scrambler.scramble(text, handleScramble, {
          characters: ['a', 'b', 'c'],
          speed: 2,          
          shuffleCount: 20,
        });
        expect(handleScramble).toHaveBeenLastCalledWith(text);
      });
    });
  });

This way, we can demonstrate the use cases to the users through the test cases.

Also, it would be good to add test cases for input validation of each parameter. If you encounter any difficulties, please feel free to let me know.