ngx-drag-to-select icon indicating copy to clipboard operation
ngx-drag-to-select copied to clipboard

proposal: refactor conditionals

Open GregOnNet opened this issue 5 years ago • 3 comments

Hi,

an idea could be to refactor the conditionals into certain rule lists.

Disclaimer

🤗 This issue is just an idea according to https://twitter.com/elmd_/status/1181875171667451905.

Propsal

const addRules = [
  withinBoundingBox && !item.selected && this.selectMode,
  withinBoundingBox && !this.shortcuts.toggleSingleItem(event) && !this.selectMode && !this.selectWithShortcut,
  withinBoundingBox && this.shortcuts.toggleSingleItem(event) && !item.selected,
  !withinBoundingBox && item.selected && this.selectMode,
  !withinBoundingBox && this.shortcuts.toggleSingleItem(event) && item.selected
];

// Usage
const shouldAdd = addRules.some(isMatch => isMatch)

// Usage inline
if (addRules.some(isMatch => isMatch)) {
  // ...
} else if ...

Affected code

https://github.com/d3lm/ngx-drag-to-select/blob/c47dee4a49c5e0d300d4b54873e0347779736dda/projects/ngx-drag-to-select/src/lib/select-container.component.ts#L423-L441

Further thoughts.

The handling of withinBoundingBox can be improved. It is a repetitively used. Maybe addRules can be transformed to a list of functions that deal with withinBoundingBox

GregOnNet avatar Oct 09 '19 12:10 GregOnNet

Hey @GregOnNet, thanks so much for opening this issue and proposing a refactoring for the messy conditional. I really appreciate it. I like the rule-based approach and I also agree that the withinBoundingBox is quite repetitive. I also notice that some parts of the conditionals are sometimes just the inverse of parts of another rule.

Also, I need to give the conditionals better names, cause I find it hard after some time to understand what they do without spending a couple of minutes thinking about them 😂

d3lm avatar Oct 09 '19 12:10 d3lm

Talking about names for rules brings classes to my mind. The taken class names below are incorrect, I believe. I don't know enough about the internals to come up with a better proposal.

interface BoxRule {
  isMatch: (predicate: boolean) => boolean
}

class InsideBoxRule implements BoxRule {
  isMatch(isInBox:boolean, predicate: boolean): boolean {
    return isInBox && predicate;
  }
}

class OutsideBoxRule implements BoxRule {
  isMatch(isInBox:boolean, predicate: boolean): boolean {
    return !isInBox && predicate;
}

GregOnNet avatar Oct 10 '19 06:10 GregOnNet

Gotcha! Definitely an interesting approach. Thanks for your input, really appreciate it. I ll have to think about this for a while but I might refactor the library to use state machines. I think this is a great way to tame the complexity of this lib including all of its modes and features, as it's getting more and more feature rich and the conditionals are getting out of control.

d3lm avatar Oct 21 '19 00:10 d3lm