Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Please standardize the long conditions must be extract to a variable, before the if statement, and the minimum length of the variable names #714

Closed
sarkiroka opened this issue Dec 8, 2016 · 8 comments

Comments

@sarkiroka
Copy link

commented Dec 8, 2016

I would be sad if this style became the javascript standard code style
some line from sample file

if (equals(sha1(r.salt ? Buffer.concat([r.salt, r.k]) : r.k), key)) {

In my opinion this more readable in this format:

let k = r.salt ? Buffer.concat([r.salt, r.k]) : r.k)
let ok = equals(sha1(k, key)
if (ok) {

or

if (!pong.r || !pong.r.id || !Buffer.isBuffer(pong.r.id) || pong.r.id.length !== 20) {

let isNotPongIdDefinedOrNotBuffer = !pong.r || !pong.r.id || !Buffer.isBuffer(pong.r.id);
let isPongSizeCorrect = isNotPongIdDefinedOrNotBuffer && pong.r.id.length !== 20;
if (isNotPongIdDefinedOrNotBuffer || !isPongSizeCorrect) {

and who understood this snippet?

if (!r || !r.v) return true
var isMutable = r.k || r.sig

what mean the r, r.v, r.k or r.sig?
Please standardize the minimum length of a variable name - except in for cycle

@feross

This comment has been minimized.

Copy link
Member

commented Dec 9, 2016

You're right that the bittorrent-dht package probably isn't the best example to link from the readme anymore. Since the last refactor it's gotten quite terse and dense. We can find a better example.

As for the line if (equals(sha1(r.salt ? Buffer.concat([r.salt, r.k]) : r.k), key)) {, I agree that it would be better if split into more lines.

The readme does say: "Clever short-hands are discouraged, in favor of clear and readable expressions, whenever possible."

I don't think we should go so far as to ban one-letter variables, because that would warn on x, y, and z in graphics programs, or even i and j in for-loops. Line length is also impractical: sometimes lines need to be long.

@feross feross closed this in 2b9afb6 Dec 9, 2016

@feross

This comment has been minimized.

Copy link
Member

commented Dec 9, 2016

Fixed by linking to expressjs/body-parser instead.

@sarkiroka

This comment has been minimized.

Copy link
Author

commented Dec 9, 2016

@sarkiroka

This comment has been minimized.

Copy link
Author

commented Dec 9, 2016

@feross

This comment has been minimized.

Copy link
Member

commented Dec 17, 2016

@sarkiroka Point taken. Feel free to avoid single letter variables in your code. But I think a rule that bans x, y, i, etc. goes too far for standard.

@dcousens

This comment has been minimized.

Copy link
Member

commented Dec 17, 2016

So, x is a coordinate, or the missing peremeter from an equation? Or http x
prefixed header? Or force of x-ray?

That is obviously going to be dependent on your context.

E.g

let { x, y } = myPosition

// ...

Clearly provides context as to x and y.
If you reached a scope complex enough such that x might have a conflict, you might instead do:

let { x: myX, y: myY } = myPosition

// ...

But, no one is going to rename x in a vector datastructure, to something other than x without a very good reason, of which this isn't.

This is the same for things like myValues.map(x => x * 2) is perfectly acceptable, because the context is immediately clear.

You don't need to be overly verbose myValues.map(value => value * 2), because it doesn't add any clarity when the context is immediately apparent.
If there is misdirection somehow, then, maybe, to re-establish that context, you might name the variables more verbosely, but, again, its situation dependent.

This is not something easily resolved with a blanket rule I'm afraid.

@sarkiroka

This comment has been minimized.

Copy link
Author

commented Dec 19, 2016

I see I think. But I think one character length variables obfuscate the code (outside from a loop - as I wrote before)
You say variable x and y are perfect for coordinate. But my question is "Where are you use a coordinate? In a graphics program? In this program which coordinate is this?"
A program never stay in a hello-world level. A live program is a more complex thing. In a graphic program - I think - you have not only one coordinate, but have many coordinates. I think a var x not enough clear to define which coordinate is this.
And I think you misunderstood me. I would like to standardize only the variable names, but not the structure parts. For example myLocation.x is good, but I think var x is bad in my idea.

And in an arrow function the hello-world level myValues.map(x => x * 2) it looks good, but what about type of x, and the mean of x? Is x an age? Is x a name of a city? Or what is the x?? The code doesn't tell me nothing. :(
And same the second arrow function where value is totally meanless name. And the myValues too.
If your name of array is ages or cities then maybe looks good the x because x is near to the name of the array. But I saw an arrow function today which was 30+ lines. In this case (real example, not hello-world level) the usage of the variable x and the array name is too far to clear and understandable code. But when the array is ages and the arrow function parameter name is age then this is clear trough the code.

I accept this is not an easy rule. But I sad if you ignore this important thing. But I can accept this too.

@dcousens

This comment has been minimized.

Copy link
Member

commented Dec 19, 2016

And I think you misunderstood me. I would like to standardize only the variable names, but not the structure parts. For example myLocation.x is good, but I think var x is bad in my idea.

I suppose I don't disagree with this, as var x on its own implies a context, which, often is only resorting to defining singular coordinates as a performance optimisation.

And in an arrow function the hello-world level myValues.map(x => x * 2) it looks good, but what about type of x, and the mean of x? Is x an age? Is x a name of a city? Or what is the x?? The code doesn't tell me nothing. :(

It is a myValue... whatever that is, the problem you are outlining is with the poorly named myValues, not its elements.

Overall, the problem is that you are attempting to predict the total set of possible contexts and use cases... I don't think we can do anything here.

We might as well be throwing warnings on spelling errors.

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
3 participants
You can’t perform that action at this time.