Skip to content
This repository was archived by the owner on Apr 3, 2023. It is now read-only.

refector(rateFn): RegularExpression optimize #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

refector(rateFn): RegularExpression optimize #12

wants to merge 1 commit into from

Conversation

hikaMaeng
Copy link

The internal functions toParsedFloat and getConverted are heavy logic used in the for loop. Changes to scope references without generating regular expressions each time, and fixes and optimizes errors in regular expressions.

The internal functions toParsedFloat and getConverted are heavy logic used in the for loop. Changes to scope references without generating regular expressions each time, and fixes and optimizes errors in regular expressions.
@jongmoon
Copy link
Contributor

jongmoon commented Aug 18, 2017

Thank you very much for your PR!

As you mentioned above, there was regex insufficiency. And your suggestion looks better than previous. 👍
But as count of parentheses is different from previous one, match result of regex also changed. So functions depend on that regex should be changed also. If not changed, it will not work.

In following code m[3] is no more valid, it should be m[2].

function toParsedFloat(val) {
	const m = val.match(toParsedFloatRegNum);
	let ret;

	if (m && m.length >= 1) {
		ret = {"num": parseFloat(m[1]), "unit": m[3]}; // m[3] is no more valid, it should be m[2]
	}
	return ret;
}

And related with the performance, getParsedFloat and getConverted are heavy function as you pointed. But it is not called in loop.

BecauserateFn does not work in loop. rateFn returns function which is used in loop.

fx.rateFn = fx.rateFn || rateFn(fx.elem, fx.start, fx.end); // fx.rateFn is not rateFn

Same name may be confusing your code reading. Sorry for confusing name. TT

And as I know inline regex is not generating reg expression each time. (although RegEx is generating each time.) But Caching inline regex (your suggestion) is more better (https://jsperf.com/regexp-indexof-perf).

Finally It would be thankful if you check eslint and test as following. :)

npm run lint // for eslint
npm run test // for unit test

After review applied, I'll merge your precious PR!
Thank you again.

@hikaMaeng
Copy link
Author

I apologize for requests without a lint & test.
Next time, prepare more for the guide ^^;

@hikaMaeng
Copy link
Author

Please close it.

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

Successfully merging this pull request may close these issues.

3 participants