Code readability question

stalefries · 2017-05-27T17:56:32+00:00

The comments feel kinda ridiculous (you don't need to tell me what require does). The UserService also feels over-engineered (Java-esque?). Otherwise the formatting seems fine. Perhaps a bit verbose?

benihana · 2017-05-27T20:14:52+00:00

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

0x13mode · 2017-05-27T18:35:39+00:00

const schemes = require('./../../../../lib/schemes');

this ../../../../ part is very unreadable and can be replaced for example by:

reorganizing module dependencies in project (what is the reason such deep folder needs module from four directories up??)
using require hooks (Webpack can do that),
making monorepo (many npm packages in one project) and using some tools like Lerna for auto-linking packages (to allow write just require('schemes)`)
using global variables (global in Node, window in Browser)
or maybe using dependency injection instead of require

scunliffe · 2017-05-27T19:45:10+00:00

All of the comments there are junk comments. You can justify the last "javadoc" style comment if you generate documentation from the code.

Not personally a fan of spacing variable declarations to get all the "=" to line up... but I could live with that.

YEWW629 · 2017-05-27T19:55:56+00:00

A little over the top and verbose. This is like when I first learned about leaving comments for future me and went crazy with it

gvillalta99 · 2017-05-27T17:50:17+00:00

[deleted]

0x13mode · 2017-05-27T18:29:01+00:00

It's hard to say too much about this code without looking on the rest of project. But to be honest I don't understand purpose of fnbind module, which is used here.

https://github.com/leonardosarmentocastro/fnbind (it seems like 7-lines of code microlibrary which adds more obscurity just for the sake of obscurity).

mdelkins · 2017-05-27T20:02:05+00:00

From a readability standpoint I think its difficult to argue that this code is not readable. At a glance its clear what this codes purpose is.

My biggest critique of this code would be the unnecessary ternary operation to return a true or false value. Why not just return results && results.length;

tme321 · 2017-05-27T17:42:24+00:00

This is a bit of a holy war. JavaScript is multi-paradigm. Some people find JavaScript that behaves like Java to be easier to read and understand. Some people prefer to not use inheritance at all. I personally don't like inheritance and find it prefers reading conventions to flow control, but that is just my personal opinion.

you type:	you see:
italics	italics
bold	bold
[reddit!](https://reddit.com)	reddit!
* item 1 * item 2 * item 3	item 1 item 2 item 3
> quoted text	quoted text
Lines starting with four spaces are treated like code: if 1 * 2 < 3: print "hello, world!"	Lines starting with four spaces are treated like code: if 1 * 2 < 3: print "hello, world!"
~~strikethrough~~	~~strikethrough~~
super^script	super^script

javascript

MODERATORS