Open lpatiny opened 7 years ago
[ ] this does not make sense in static functions https://github.com/cheminfo-js/spectra-data/blob/f8100bd69498719fe0bd46267c1cb369c9e49611/src/range/Ranges.js#L98
[ ] could be renamed to toXY https://github.com/cheminfo-js/spectra-data/blob/f8100bd69498719fe0bd46267c1cb369c9e49611/src/range/peak2Vector.js
[ ] It is important to always name the function to have it in the stack trace (other it is anonymous function). For example in: https://github.com/cheminfo-js/spectra-data/blob/9fec551/src/range/peak2Vector.js
module.exports = function toXY(peaks, opt) {
[ ] if possible it would be nice not to have to support 2 format of peaks. Only x, y, width (or x,y, but not x,intensity) https://github.com/cheminfo-js/spectra-data/blob/f8100bd69498719fe0bd46267c1cb369c9e49611/src/range/peak2Vector.js
[ ] For power of 2 it is likely that multiply * multiply is faster than Math.pow(xx,2)
[ ] Is it really useful here ? https://github.com/cheminfo-js/spectra-data/blob/9fec551/src/range/utils.js
[ ] What does this method and why > 11 ? https://github.com/cheminfo-js/spectra-data/blob/e921673/src/range/utils.js#L5 Is is related to ACS formatting ? I guess then the number of decimals should be an option. For carbon we need only one decimal, for proton 2
[ ] remove oldACS ?
[ ] Don't use global variable for other things that constants https://github.com/cheminfo-js/spectra-data/blob/e92167314d912e1ddd57169fb2d3f9d9b5110ff5/src/range/acs/acs.js#L7-L9
[ ] You are making a copy of ranges through JSON.parse / JSON.stringify and therefore there can not be any function on this object https://github.com/cheminfo-js/spectra-data/blob/e92167314d912e1ddd57169fb2d3f9d9b5110ff5/src/range/acs/acs.js#L14-L20
[ ] Would be better to have a hidden internal class for acsString (AcsStringBuilder) so that we can have an instance of it and call 'this' instead of using global variables
[ ] this does not make sense in static functions https://github.com/cheminfo-js/spectra-data/blob/f8100bd69498719fe0bd46267c1cb369c9e49611/src/range/Ranges.js#L98
[ ] could be renamed to toXY https://github.com/cheminfo-js/spectra-data/blob/f8100bd69498719fe0bd46267c1cb369c9e49611/src/range/peak2Vector.js
[ ] It is important to always name the function to have it in the stack trace (other it is anonymous function). For example in: https://github.com/cheminfo-js/spectra-data/blob/9fec551/src/range/peak2Vector.js
module.exports = function toXY(peaks, opt) {
[ ] if possible it would be nice not to have to support 2 format of peaks. Only x, y, width (or x,y, but not x,intensity) https://github.com/cheminfo-js/spectra-data/blob/f8100bd69498719fe0bd46267c1cb369c9e49611/src/range/peak2Vector.js
[ ] For power of 2 it is likely that multiply * multiply is faster than Math.pow(xx,2)
[ ] Is it really useful here ? https://github.com/cheminfo-js/spectra-data/blob/9fec551/src/range/utils.js
[ ] What does this method and why > 11 ? https://github.com/cheminfo-js/spectra-data/blob/e921673/src/range/utils.js#L5 Is is related to ACS formatting ? I guess then the number of decimals should be an option. For carbon we need only one decimal, for proton 2
[ ] remove oldACS ?
[ ] Don't use global variable for other things that constants https://github.com/cheminfo-js/spectra-data/blob/e92167314d912e1ddd57169fb2d3f9d9b5110ff5/src/range/acs/acs.js#L7-L9
[ ] You are making a copy of ranges through JSON.parse / JSON.stringify and therefore there can not be any function on this object https://github.com/cheminfo-js/spectra-data/blob/e92167314d912e1ddd57169fb2d3f9d9b5110ff5/src/range/acs/acs.js#L14-L20
[ ] Would be better to have a hidden internal class for acsString (AcsStringBuilder) so that we can have an instance of it and call 'this' instead of using global variables