small javascript code cleanup #138
Conversation
lib/sqlite.core.js
Outdated
| i = 0; | ||
| mycbmap = {}; | ||
| mycbmap = []; |
There was a problem hiding this comment.
I don't see why do we need to have an array here
| if (result.length == 0){ | ||
| return; | ||
| } | ||
| last = result.length - 1; | ||
| for (i = j = 0, ref = last; 0 <= ref ? j <= ref : j >= ref; i = 0 <= ref ? ++j : --j) { |
There was a problem hiding this comment.
Honestly, I don't understand the reason behind such complexity.
Though I'm not a js developer so I might be missing something.
for (i = j = 0, ref = last; 0 <= ref ? j <= ref : j >= ref; i = 0 <= ref ? ++j : --j) {
the ref could be == -1 only in case the map is empty.
I don't think we need to handle this case there.
In all other cases, the ref is >= 0.
So 0 <= ref is always true and we never will use --j and j >= ref.
There was a problem hiding this comment.
the reason for this is that it is translated from a coffee script. I keep it in sync with Cordova plugin.
|
Sergey - apologies but I won't merge this in as I want to stay as close as possible to the translated version of the coffee script. |
|
Sad. I always stumbled upon this crazy loop implementation and so I was very excited about this cleanup and that someone finally stepped thru this "special"-for-loop that i've never understood in detail, and improved it to a simple one. This PR also could have some (minor) performance-improvements since it takes a lot of complexity and ternary-operations with every iteration on the loop. Since this code shall base always on cordova plugin (why?), which is based on coffeescript and is transpiled to javascript, there would be never any chance to get improved, simple-to-understand and simple-to-change-javascript code in this repository like happened with this PR? I'm not sure if this is future-proof and if we can get more companies sending their improvements to this repo then |
|
@dryganets Sergey - I re-opened this PR - I will review again and reconsider based on performance improvement factor. @itinance this is staying close to cordova version of the JS glue because I periodically merge the fixes from that repo. Since the fixes are usually applied to CoffeeScript there it is sometimes very hard to figure out how it applies to converted JS, if that JS is radically changed... |
|
Merged. Thank you. |
just generic cleanup