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
hikaMaeng wants to merge 1 commit into
naver:masterfrom
hikaMaeng:master
Open

refector(rateFn): RegularExpression optimize#12
hikaMaeng wants to merge 1 commit into
naver:masterfrom
hikaMaeng:master

Conversation

@hikaMaeng

Copy link
Copy Markdown

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

jongmoon commented Aug 18, 2017

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Author

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

@hikaMaeng

Copy link
Copy Markdown
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