mediamonks / frontend-coding-standards

Media.Monks - Frontend Coding Standards
60 stars 23 forks source link

boolean naming #48

Open psimk opened 3 years ago

psimk commented 3 years ago

Currently, we sort of have a generally accepted rule, that all booleans are prefixed with is e.g. isDragging, isOpen, isVisible. I understand that this rule is in place as it makes it very clear that a variable is a boolean, however personally the is always feels ugly.

The problem that is is trying to solve is that some names could be mistaken for verbs. Example of the issue: open is a two fold definition, it can mean "open something" (an action) and also "something is open" (a description). Same is true for dragging, "dragging something" (an action) and "something is dragging" (a description)

However, this is not always true; example being visible, which is just an adjective and wouldn't be confused for anything else. Moreover, we can also define better names for booleans, for example, instead of the confusing open or the prefixed isOpen, we can use the inverted closed, which again, wouldn't be mistaken for anything other than an adjective.

To illustrate the ugliness of using is, here's some React hooks code:

with the prefix

const [isOpen, setIsOpen] = useState(false);

const open = () => setIsOpen(true);
const close = () => setIsOpen(false);

with better naming

const [closed, setClosed] = useState(true);

const open = () => setClosed(false);
const close = () => setClosed(true);

Even though the latter example, flips the definition of the variable, to me it's a lot more pleasant to read.

ThijsTyZ commented 3 years ago

Why closed and not opened?

psimk commented 3 years ago

I guess I had a brain freeze and forgot about opened. So the logic wouldn't have to change at all then.

const [opened, setOpened] = useState(false);

const open = () => setOpened(true);
const close = () => setOpened(false);
ThaNarie commented 3 years ago

I agree that in some cases the is isn't needed, and indeed can become "ugly" - although most of that is also because of the convention that the state "setter" should include the full "getter" name inside of it. _Using isOpen and setOpen could also work_.

Personally, I'm fine with not prefixing some of these if they are unambiguous when the name is clearly describing a state, and not an action. Small related side note, in component props we often already do this, since component props should only contain state (and events).

What I don't like at all, is reversing the name of a boolean expression to its counterpart because the name itself sounds better, but the usage of it becomes weird (double negative conditions and the like). But as Thijs mentions, most of the time this isn't needed. (opened and closed can both be used just fine).

In general, if we pick "states" (opened vs isOpen) we could mostly circumvent the ambiguity there. Although, isOpen is the current state, while opened could suggest a historic state (has ever been opened), so that could add a different kind of unambiguity... and some states might sound less "nice", like isActive vs activated

skulptur commented 3 years ago

It can be ugly but it also makes it so obvious that I much much prefer the first option. Also agree with opened being more confusing because of the past tense. Thinking about time in programming is never intuitive, having the present isOpen is easier to reason about.

psimk commented 3 years ago

Using isOpen and setOpen could also work.

This would sort of throw a wrench into searching through a file. If for instance I wanna view all usages of isOpen including the "sets", searching for isOpen wouldn't work as the is no longer is part of setOpen. I also think it would be difficult to get used to this.

and some states might sound less "nice", like isActive vs activated

the prefix-less equivalent for isActive would actually just be active as it denotes just an adjective and nothing else.

Also agree with opened being more confusing because of the past tense.

I do agree with both of you that throwing a different tense can make it more confusing.

I think the the best approach would be to simply prepend the is, whenever the name is ambiguous and for cases when the name denotes just an adjective, we should keep it as is. Perhaps, with a strong recommendation for the latter.

Would be nice to document this, so it would be clear for all that it is ok to not prefix is in certain (umambiguous) cases.

ThijsTyZ commented 3 years ago

The advantage of the is (and has, will, should, etc.) prefix is that it is clear and explicit for everyone. Otherwise the rule must be something like: "A boolean name should be a past participle or a adjective or start with is (and has, will, should, etc.) dependent on what looks best" I am afraid that, since there is too much room for personal opinions and interpretation, this will lead to inconsistent names.