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

boolean naming #48

Open
psimk opened this issue Dec 30, 2020 · 6 comments
Open

boolean naming #48

psimk opened this issue Dec 30, 2020 · 6 comments

Comments

@psimk
Copy link

psimk commented Dec 30, 2020

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
Copy link

Why closed and not opened?

@psimk
Copy link
Author

psimk commented Dec 30, 2020

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Author

psimk commented Dec 30, 2020

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants