search

cheminfo-js / spectra-data

Library to deal with spectra (IR, UV, ...), 1D NMR as well as 2D NMR
https://cheminfo-js.github.io/spectra-data
MIT License
4 stars 0 forks source link

Would be better to make push / join(CRLF) #35

Closed lpatiny closed 7 years ago

lpatiny commented 7 years ago

https://github.com/cheminfo-js/spectra-data/blob/c3578a2/src/jcampEncoder/JcampCreator.js#L17

andcastillo commented 7 years ago

I don't understand @lpatiny

lpatiny commented 7 years ago

You could use a structure like:

var result=[]; result.push('my first string'); result.push('my second string'); return result.join(CRLF);

andcastillo commented 7 years ago

When I did that function I checked and the advise was to use direct concatenation instead of the array push. It was faster at that time.

lpatiny commented 7 years ago

OK well possible this. We suggested this because the code looks prettier (at our point of view) but I will make a jsperf to check this. For sure the push used to be faster but you are probably right that this changed with time.

andcastillo commented 7 years ago

I definitely could not find anything concluding about this. A benchmark would help. @targos know something about it?

lpatiny commented 7 years ago

Yes you are right the concat is faster than the join currently with chrome: https://jsperf.com/test-push-or-concat So you may leave it like that.

lpatiny commented 7 years ago

Same result on Safari: image