johnny / johnny/jquery-sortable

Possible improvements to the codebase.

Open
#92 1 comment 0 reactions 0 assignees View on GitHub
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.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.