johnny / johnny/jquery-sortable
Possible improvements to the codebase.
- Dominant language
- HTML
- Stars
- 1.5k
- Forks
- 429
- PR merge metrics
- No merged PRs in 30d
Description
As I've been struggling in the codebase, I thought I could make a few comments.
I probably should have written them in a more constructive manner, sorry. In any case, my hope is that the source of some of my hesitations can lead to improvements.
- no tests
- coding style far from what I’m used to:
- not even close to passing jslint
- sloppy whitespace policy, I’ve seen "function () {“, "function() {", "function (){", "function () {", "function(){“
- no indent for multiple variable declarations threw me off a bit too
- not prefixing jQuery objects with $ (mostly. no idea why `$getChildren` has a `$` when so many do not)
- Naming far from obvious. `el`, `containerGroup` vs `childGroup`, `hasContainerGroup` returning non boolean, ...
- options are copied everywhere, so container options are in groups, and in sub containers, and some are modified (e.g. group), some are duplicated (`this.options.rootGroup` vs `this.rootGroup`) it’s difficult to keep track
- I understand there was an effort for lazily calculating stuff, but it’s very difficult to know what attributes to rely on. Which are set only for the root group? Which are valid only during a drag? When is `Container#items` valid? When are `Container` / `ContainerGroup` instances destroyed or updated?
- dubious separations of concern, e.g. caring about `rootGroup.dragInitDone` should be `rootGroup.dragInit`’s job exclusively.
- traverse should use `call(this)` instead of passing the `this` object. It would be even simpler if it accepted the name of a method to call instead of a function.
- An abstraction level for ‘node’, with methods that could abstracted, like `traverse`, `clear dimensions`, etc… might be helpful
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.