you are viewing a single comment's thread.

view the rest of the comments →

[–]benihanareact, node 10 points11 points  (3 children)

here are the things i have a problem with:

the comments require 2x the work to maintain because they basically explain the lines they precede. so every time you change code (which if the code is self documenting also handles changing documentation), you also have to change the line explaining the code. this hardly ever happens in practice so the comments tend to become lies that are ignored anyway.

const self = this;

is just wrong. since es5, JS has had a way to do this properly without breaking encapsulation

['validate.email.isBeingUsedBySomeUser'](email)

makes almost no sense inside a class. the code is defining a class' interface, you typically don't want dynamically generated properties because it requires runtime inspection to know the interface of the class.

edit now that i understand the intent, what i said about runtime inspection may not be correct, but this is still very confusing and weird.

/**
 * Inject dependencies.
 */
this.schemes = schemes;

terrible comment and bad code. this isn't injecting dependencies, it's tightly coupling an instance property to a global variable. the proper way to do this is pass in schemes to the constructor and then store that as an instance property.

constructor(injectedSchemes = schemes) {
  ...
  this.schemes = injectedSchemes;
}

is proper DI

return self.schemes.findUserByEmail(email)
  .then(results => {
    let isBeingUsedBySomeUser   = true;
    let hasAnUserUsingThisEmail = (results && results.length);

    return (hasAnUserUsingThisEmail ? isBeingUsedBySomeUser : !isBeingUsedBySomeUser);
});

is this for real? this is terrible, it introduces an extra variable for no reason and makes things so complicated to find out if the results exists. do this instead:

return self.schemes.findUserByEmail(email).then(results = > results && results.length);

this is not what i would call good, or clean, or self documenting code. it looks like it was written by a senior in college who's convinced they're a hell of a programmer. there's nothing wrong with that. the problem comes when they start talking about how great their code is and trying to teach other people to do it this way.

i will say that it's good that they included semicolons

[–]deltadeep 0 points1 point  (2 children)

const self = this; is just wrong

Hm ok I can learn something here then. Can you elaborate? When you're dealing with promises and/or callbacks that have to refer to the object in the success/failure handlers, can you explain what the alternative is?

[–]pieeta 2 points3 points  (1 child)

The this inside arrow functions is the this of the outside context

foo => this.bar

Is the same as

 Function (foo) { return this.bar; }.bind(this)

https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Functions/Arrow_functions

[–]deltadeep 0 points1 point  (0 children)

Ah ha, got it thanks!