sindresorhus / eslint-plugin-unicorn

More than 100 powerful ESLint rules
MIT License
4.24k stars 367 forks source link

Rule proposal: Prevent abbreviations #169

Closed sindresorhus closed 5 years ago

sindresorhus commented 6 years ago

Issuehunt badges

Using complete words result in more readable code. Not everyone knows all your abbreviations. You only write code once, but it's read many times.

For a start, we could go with these:

Also i => index, but I have a feeling, that it's going to be too controversial.

Suggestions welcome for additional ones. Also looking for people's thoughts on this rule.

futpib earned $160.00 by resolving this issue!

lukechilds commented 6 years ago

Honestly, I think they're all quite controversial.

I agree with you that that in general abbreviations are not good variable names, but I personally prefer the shorter version of most of these and I think they're pretty widely understood. I'd say err is probably even more common than error. It's in all the official Node.js docs.

Also, how reliable will this be? What if cb is an abbreviation for circuit breaker. Obviously that's a bad variable name but it shouldn't be automatically changed to callback.

sindresorhus commented 6 years ago

But is it really worth sacrificing readability over saving a few characters? Swift has taught me a lot about writing readable code. Some would say it's too verbose, but I also think JS is generally too concise.

I know we have conventions and all that, but should we maybe stop and question those? I feel like everyone just does it because everyone else does it, without actually considering whether it's worth the downsides.

I'd say err is probably even more common than error. It's in all the official Node.js docs.

Neither the Node.js API nor docs are known for their approachability or user-friendliness, though.

Also, how reliable will this be? What if cb is an abbreviation for circuit breaker. Obviously that's a bad variable name but it shouldn't be automatically changed to callback.

I didn't intend for it to have auto-fixing, but if it did, it could only fix the non-ambiguous cases.

jamesmosier commented 6 years ago

arr => array

I've also seen elem => element but I see you have el above (which is more popular).

FezVrasta commented 6 years ago

event is also seen as evt quite often

frontend-3 commented 6 years ago

btn -> button msg -> message

TGC79 commented 6 years ago

len -> length

savetheclocktower commented 6 years ago

Abbreviations like obj and str, though arguably slightly less readable than object and string, are at least unambiguous. If this rule were to exist, it seems like it would be more useful if it targeted ambiguous abbreviations. For example: I've seen e used for both event and error, and it's made me more likely to write out event and error as a result just for clarity.

Hence, I have no problem with cb, evt, or req — I don't think I've ever come across one of those and not known what it was meant to stand for. I think val is a poor thing to name a variable, but value is just as bad — the problem there is the vagueness in the name, not the abbreviation.

sindresorhus commented 6 years ago

Abbreviations like obj and str, though arguably slightly less readable than object and string, are at least unambiguous.

Are they though? Only if you know that str always means string, which it usually does, but that's just a convention. It could just as easily be short for strength.

I think val is a poor thing to name a variable, but value is just as bad — the problem there is the vagueness in the name, not the abbreviation.

I agree, value is not a good name, but what would you call a variable that could be any value? You could call it any, but I don't think that's clearer.

sindresorhus commented 6 years ago

If you, for example, disagree with err=>error, I'm curious about why? It's only two extra characters and it improves readability. "Because archaic conventions" is not a great argument.

savetheclocktower commented 6 years ago

Are they though? Only if you know that str always means string, which it usually does, but that's just a convention. It could just as easily be short for strength.

It could, if you're building an RPG. In the general case I think that those abbreviations are common enough that (e.g.) str presumptively means string, and if you need strength then you should just type strength.

I agree, value is not a good name, but what would you call a variable that could be any value? You could call it any, but I don't think that's clearer.

This is the one caveat I should've added to my original comment, but I was growing tired of typing. :)

mantoni commented 6 years ago

I personally also base naming decisions on scope size. While having a catch(e) with a single line might be easy to read, a long function with short names becomes unreadable very quickly.

So as a rule of thumb, I’d say the larger the scope the more descriptive the variable names should be.

mischah commented 6 years ago

The following ones came to my mind spontaneously:

abdullahtariq1171 commented 6 years ago

tmp => temporary ans => answer

therealklanni commented 6 years ago

Are we also considering shorthands like a/b/x => any, fn => function?

const curry = fn => a => b => fn(a, b)

I see this a lot more in more academic uses, though it does happen in the real world as well.

praveenv4k commented 6 years ago

arg => argument args => arguments rec => record tbl => table db => database

therealklanni commented 6 years ago

args => arguments is almost necessary, since arguments is reserved.

felixfbecker commented 6 years ago

I agree with avoiding abbreviations in general, but I think the problem with this rule is that it will forbid all the abbreviations that are okay because they are widely understood (e.g. err) while allowing all the abbreviations that are not common/context-dependent. It's hard to come up with concrete examples for this, but if I read most Go codebases they have 1-3 letter variable names everywhere, that are not only non-descriptive, they also make reading the code influent.

So I think a rule like this instead of blacklisting abbreviations, should for example check variable names against an English dictionary and/or enforce a minimum character length.

sindresorhus commented 6 years ago

Are we also considering shorthands like a/b/x -> any, fn -> function?

I think x and a & b are usually fine in inline arrow functions if it's clear from the context what they refer to.

sindresorhus commented 6 years ago

args => arguments is almost necessary, since arguments is reserved.

Not in arrow functions, which I personally use most of the time.

sindresorhus commented 6 years ago

I agree the main focus should be to reduce ambiguity, the other cases could potentially be opt-in.

therealklanni commented 6 years ago

Not in arrow functions, which I personally use most of the time.

That's fair, but then you have to ask yourself if you want consistency, too.

sindresorhus commented 6 years ago

@therealklanni If I had to choose, I would pick readability over consistency.

savetheclocktower commented 6 years ago

I think this has the potential to be the worst of all possible bikesheds, and you'll hear from three groups of people:

  1. people who would like this rule and would use it to help them stick to new conventions;
  2. people who already don't abbreviate these things and want to make their collaborators abide by the same rules; and
  3. people who have opinions about abbreviations in code, generally speaking, but wouldn't use this rule.

If the goal is to ship this rule with a recommended set of abbreviations to avoid, my guess is that you won't find a consensus for more than a tiny handful of identifiers.

sindresorhus commented 6 years ago

@savetheclocktower This is just exploratory right now. I'm happy to hear all opinions. The main focus is the ambiguity aspect, which I don't think many would object to. I've personally been using the non-abbreviations in my code for a while now. I'm liking it so far, but it takes some getting used to.

hugmanrique commented 6 years ago

ctx => context mod => module

kalisjoshua commented 6 years ago

I use these pretty often:

_ => when I don't actually care about it - usually in an arguments list - but need it to take up the position indx => index because index is a reserved word in JS attrs => attributes because I'm super laz

therealklanni commented 6 years ago

index is not a reserved word.

loilo commented 6 years ago

If you, for example, disagree with err=>error, I'm curious about why?

I'm not sure why I feel uncomfortable with those expanded terms replacing all those rather well-known abbrevations, but I think I've come up with a possible explanation.

  1. I, personally, as a user of these abbrevations, find them clearer than the long names. That's however only learned behaviour and could certainly be retrained.
  2. What I'm not sure about: I think it makes me uncomfortable because it bloats the code, just in pure character count.

    I feel like my chances of getting the hang of a snippet by taking a quick glimpse are greater for

    let spread = (arr, cb) => cb(...arr)

    than they are for

    let spread = (array, callback) => callback(...array)

    I'd mainly attribute that to the shorter line length, I can register more code at once in the same field of view — but I can't say that for sure. It could as well just be because of 1.

therealklanni commented 6 years ago
  1. What I'm not sure about: I think it makes me uncomfortable because it bloats the code, just in pure character count.

That's what minifiers are for (and character count is largely irrelevant for server-side)

andrepolischuk commented 6 years ago

I see the following sometimes:

ns -> namespace prop -> property ref -> reference dt -> date cmd -> command

loilo commented 6 years ago

That's what minifiers are for (and character count is largely irrelevant for server-side)

@therealklanni I don't mean bloat in the sense of a performance hit. I think my explanations make that clear, maybe read them again. 🙂

10xLaCroixDrinker commented 6 years ago

re => regex

mischah commented 6 years ago

re => regex => regularExpression re => regexp => regularExpression

To be exact @10xLaCroixDrinker 😏

lo1tuma commented 6 years ago

del => delete cmp => compare cnt => count / counter init => initialize num => number dict => dictionary inc => increment / increase / income dec => decrement / decrease fn => function fns => functions grp => group dpl => deploy / deployment w => width h => height cp / cpy => copy acc => account / accumulator pkg => package

CrossEye commented 6 years ago

Why not

?

</sarcasm>

Perhaps I shouldn't be responding here, as I don't have any skin in the game, never having used this unicorn and only having run across the issue on Twitter. But my take is that this is a crazy rule. A good abbreviation makes it easier to read code. I think this is mostly when the variables have to do with coding as coding. When encapsulating business objects, I use the business terms or their widely recognizable abbreviations. But a loop counter, a callback reference, an options object, or any number of other coding artifacts don't need the same.

In fact, they can even collide. Imagine a working in a financial-trading application, and spelling out index, request, options when those have nothing to do with their business meanings. That can lead to confusion.

John Degoes' Descriptive Variable Names: A Code Smell also comes to mind. While clearly it's not always true that more generic code is better than less generic code, it's probably better to lean that way. And as you add abstractions, concrete variable names continually get in the way.

lukechilds commented 6 years ago

Just an idea I wanna throw in here.

I'm still not convinced personally that clear abbreviations are a bad thing. But if this is a rule you want to add, what about instead of blacklisting specific abbreviations (since people will never agree) you just try and detect all abbreviations.

For all variable names shorter than or equal to n (probably 3 or 4), do a lookup on common n letter dictionary words and see if it matches. If it doesn't, it's likely an abbreviation.

This could also be configured to allow common conventions if the user wanted. So err & cb could be whitelisted but app specific abbreviations would not be allowed. So users can abide by (arguably bad) community conventions but keep their app code readable.

Maybe this would be too slow or error prone. But it seems like a warning like:

foo.js:100:4
⚠  100:3   Ambiguous variable name.

would be useful, I just don't like the idea of it being too opinionated.

belchior commented 6 years ago

d => date n => next t => time / timer pt => pointer app => application dec => decimal div => divide sub => subtract curr => current mult => multiply prev => previous

loilo commented 6 years ago
Abbrevation Full name
src source
dist distribution
dst destination/distribution (I never know which one it actually is, but it usually doesn't matter anyway.)
fs filesystem
dir directory
config configuration (seems very controversial to me)
(a)sync (a)synchronous
eval evaluate

The last two ones especially expose some difficulty since they're not only conventions but actually baked into JS.

sebdeckers commented 6 years ago

HTML

a => anchor li => listItem ul => unorderedList ol => orderedList

General

doc => document docs => documents/documentation

stephanschubert commented 6 years ago

coll => collection

ivancuric commented 6 years ago

I'd say just avoid (most) single-letter abbreviations. I want to read code, not a mathematical proof.

Rowno commented 6 years ago

FYI, the main reason I use e instead of event is that event is a global variable in browsers (which has bitten me multiple times because eslint doesn't tell you when it's undefined).

evocateur commented 6 years ago

package is also a reserved word, so pkg is the most suitable replacement.

aymericbouzy commented 6 years ago

How about letting people add / override / remove whatever abbreviation doesn't suit them in an optional configuration? That way you avoid the bikeshed.

The configuration object could look this way : { e: "error", err: false} which is really concise and clear in my opinion.

I also think that having this configurable opinionated rule avoids the bikeshed in teams regarding which abbreviations are okay and which are not.

It could also be a plugin of course.

brandonweiss commented 6 years ago

col => column lat => latitude lon => longitude

brandonweiss commented 6 years ago

idx => index hsh => hash bool => boolean

sholladay commented 6 years ago

Are you going to stop using m to refer to your modules in tests? (Example)

I don't have strong feelings about it, but I always felt that was a bit strange and had to get used to it. Personally, I let my Yeoman generator take care of templating the import and then editor autocomplete takes over from there. Very easy to write and to read with the normal module name.

focusaurus commented 6 years ago

I support this. One reason to throw onto the pile is avoiding confusion when discussing code or pair programming. If I verbally say "OK call callback with null then result" and you have to type cb(null, res) that generates a lot of confusion and stuff like "no not error, eee arr arr".

voxpelli commented 6 years ago

I prefer err over error because I don't like it that error is just a single case away from the built in Error – using err makes it clearer that one is referring to a variable and hasn't just misspelled Error as error.

I prefer obj over object for the same reason.

Also – when it comes to eg. arr/array, obj/object – wouldn't the better suggestion be to suggest a descriptive variable name rather than suggesting a non-abbreviated variable name – as any variable saying just arr/array is fairly ambiguous as to what it refers to.

brandonweiss commented 6 years ago

@voxpelli I think it depends on the context. If you know what the semantic name of the collection/object is and you’re using a generic name then yeah, the improvement is to just not do that. But you could also be working on an abstraction or something where there is no semantic name and the clearest way to refer to it is by what data type it is.