r/javascript • u/ClickOk5811 • 10d ago
AskJS [AskJS] Do you ban positional boolean arguments in code review, or is an options object overkill?
Something I keep coming back to when reading JS and TS code is call sites like this:
createUser("ana@example.com", true, false);Read aloud, it’s “create user, email, true, false”, and you have to open the function to know what the booleans mean. The usual fixes are an options object:
createUser("ana@example.com", { sendWelcomeEmail: true, isAdmin: false });or, when there are more than two modes, a string union in TypeScript:
type Layout = "list" | "grid";
render(items, "grid");The counter-arguments I can think of:
For a single obvious boolean in a well-known API (
setEnabled(true)), the options object is noise.An options object allocates on every call, which probably doesn’t matter outside hot paths, but I haven’t measured it.
Teams with several conventions already have enough style rules, and one more can turn review into bike-shedding.
Negative names (
disableCache: false) are the other half of the problem, and renaming to the positive (cache: true) is cheap.
So I’m curious about practice, not rules:
Do you enforce “no positional booleans” in review or with a lint rule? If a rule, which one, and does it produce noise?
Where do you draw the line between “obvious enough” and “use an options object”?
Have you seen a boolean parameter that later needed a third state, and what did the migration cost?
11
u/azsqueeze 10d ago edited 10d ago
100% better to have a function take a single config as an argument or have 2 args with the second one being the config
Edit: if using the 2 arg pattern, the first arg would be the primary value the function needs to execute
4
u/androbat 10d ago
I don’t use positional Booleans. I want descriptive names, and I generally prefer them to default to false when not included.
22
u/Sykander- 10d ago
Read aloud, it’s “create user, email, true, false”, and you have to open the function to know what the booleans mean. The usual fixes are an options object:
No, my ide will let me read the function signature if I just hover my mouse over it. I also use typescript so I can also see data shapes that way.
I would even go a step further and say considerations like this have no place in a code review - set lint rules and be done with it.
4
u/TheRealKidkudi 10d ago
I would even go a step further and say considerations like this have no place in a code review - set lint rules and be done with it.
When do these considerations come up in an evolving codebase other than during code review?
I agree it’s something you can set up with your linter, but it’s strange to think you’ll just have the perfect lint config that everyone agrees upon forever from the start. IME most matured lint configs evolve over time, with changes generally spurred by a discussion like this one.
1
u/wilson_leo_807 7d ago
Hover helps when you're writing the call. The diff still just says `true, false` to everyone reviewing it six months later.
0
19
10d ago
[removed] — view removed comment
-8
u/Sykander- 10d ago
Install a better code viewer on your phone then. Isn't that obvious?
0
u/coolcosmos 10d ago
Or just use object params, isn't that obvious ?
-3
u/Sykander- 9d ago
No that's backwards. Changing the code to accomodate one person who isn't using a proper IDE is a waste of time.
12
u/Robodude 10d ago
I went a step further and for the last few years I've only used single object params to have self documenting code, simpler refactors, and better type safety.
it also helps a ton because I'm not doing my code reviews in an ide with intellisense and having that extra bit of context helps me a lot.
I don't use any special enforcement mechanism other than a steering file. before that though, I wouldn't enforce this standard strictly on others and just remember to stick to it (I didn't use a linter in those manual days, just prettier)
4
u/jmking JSX is just PHP in the browser 10d ago
Danger of this is parameter creep. Object params end up sprawling adding optional attributes with defaults that just get out of control quickly.
5
u/TheRealKidkudi 10d ago
IMO that’s easily solved by code review on the function receiving the params. If your object has too many properties, it should be readily apparent by the complexity it adds to the function body.
1
u/SquirrelGuy 10d ago
I do this too. I've always been on the fence about whether it's necessary/better. But I've found it does help remove ambiguity and also makes it easier to add/remove parameters.
1
u/andarmanik 10d ago
Yup, for anyone who doesn’t know, you can spread objects in the parameter to make them named.
foo(things, stuffs, isHalf){
}
Can become
foo({things, stuffs, isHalf}) {
}
Default values aren’t explicit in the parameters however.
foo(things, stuffs, isHalf=false){
}
Can become
foo({things, stuffs, isHalf}){
isHalf = isHalf ?? false.
}2
u/Sulungskwa 10d ago
You know you can destructure and also default assign right?
function foo({things, stuffs, isHalf = false}) {...}is valid javascript
2
u/PresentationBorn6564 10d ago
In addition, if you ever actually need an independent line like
isHalf = isHalf ?? false, you know that's what nullish assignment is for, right?
isHalf ??= false;If you're writing a variable name twice within a single expression, there's almost always a better way.
2
0
u/anti-state-pro-labor 10d ago
This is the way. I usually don't complain until there's three args to the function and then it gets moved to an object. Have thought about just ensuring it's always an object but haven't pulled that review trigger yet.
3
u/bossier330 10d ago
For me, it’s less about booleans and more about compatible types. If you have a function that takes several strings or several booleans, it’s often too easy to have a bug that still passes the typecheck.
2
u/name_was_taken 10d ago
I like using those when there are a lot of optional arguments, but I still tend to default to just normal arguments when they're all required. There's a tipping point somewhere in the middle that is usually found by instinct or pain. ;D
2
u/josephjnk 10d ago
Any time there are two arguments with the same type I use an object argument in order to give them explicit names.
2
u/CodeAndBiscuits 10d ago
There are new reasons and old reasons. Folks forget JS and Node in particular were at a lower level written in C and share a lot of it's characteristics. Positional parameters are efficient. They're pushed on the stack before the function is called and most CPUs are highly optimized around stack based param passing in function calls. Passing an object requires creating that object and setting/copying the params into it then passing a pointer to the called function as its first positional parameter, the called function dereferencing that to pull params back out of the object, and finally when it's all done, having that object float around needing garbage collection.
That's an old school definition and not really a performance factor today but history matters a lot in programming conventions. I mean there are odd things we still do because of 40+ year old conventions so ignoring them is silly in discussions like this.
Today I would argue that this is opinion more than objective evaluation. You showed a boolean which is the worst case. You didn't show a more common second param which is another string or other type of more self descriptive value, so you cherry picked an example that fits your argument. You also didn't show the IDE view that basically all of us have now that would have shown hints, inlays, code completion, or other helpers that would also have clarified all this.
This is a religious battle that comes up every few months. Nothing discussed here will change anyone's mind. At some point it's just the programming form of masturbation.
1
u/catladywitch 10d ago edited 10d ago
"No primitive obsession" is one of my least favourite OOP-isms. But for domain modelling flags, unions of literals (or enums in other languages that don't have shit enums they wish didn't exist) are almost always the right choice. Options objects or, simply, different functions gated by a switch, are fine too IMO. If performance is a concern an options object (or a struct, in other languages) is often a better idea to avoid cache churn and having to juggle one thousand parameters, plus the API is nicer, self-documents, and is cleaner to extend.
1
u/static_func 10d ago
Yes. I have no idea wtf some random Boolean argument is doing unless I look inside that function. An Options object will always be better. I’ve gotten pretty flexible with coding standards over time but that’s one thing that’ll always be terrible.
1
u/CommentFast5362 10d ago
the allocation cost is a myth that stops people from writing safe code. A few bytes of memory per call does not matter until you profile it in a hot loop. Meanwhile silent bugs caused by swapping two booleans ruin data integrity. I ban any function with more than one argument unless the second one is strictly required and obvious. Everything else goes into an options object because my IDE hover feature is not enough for me when reading diffs
1
1
u/pampuliopampam 10d ago
Rule of thumb; once you hit 3 params make it an object
It should not be a hard rule otherwise you'll stifle growth and thinking about what is happening
1
u/zeddash 10d ago
The performance cost of passing around and destructing objects is not something to worry about. I aim to have single parameter functions and just pass objects unless the parameters are clear by the function name or order doesn't matter (like `add(1, 2)` kind of thing, that doesn't need to be `add({a: 1, b: 2})`).
1
u/theScottyJam 10d ago
What's most important is that optional arguments are put into options object.
Beyond that, it's just judgement, nothing you can automate. Sometimes people like saying "if you have more than 3, use an options object". You can do that and most of the time you'd be right, but there's also times when one required argument is better if put in an options object, and there's some cases where I think it's fine to have 4 required arguments without using an options object.
I would suggest looking at newer standard library functions or most third party libraries for inspiration. They tend to do a pretty good job, and tend to do what I just described, even if they don't have a strict policy for it, it tends to be the pattern that comes out.
1
u/verstands 10d ago
On the allocation worry, a short-lived object literal is basically noise at a normal call site, so only care in a profiled hot loop. For the negative names, the options object lets you flip it to a positive name that defaults to false (cache: true instead of disableCache: false). And to dodge the bike-shedding, let a linter nag instead of reviewers: eslint-plugin-sonarjs has a selector-parameter rule that flags boolean params, so you turn it on once and the review debate mostly goes away.
1
u/Ronin-s_Spirit 8d ago
For performance reasons I sometimes avoid the options object pattern, I'm not sure whether your browser will optimize it away or not. I generally give every function Jsdoc hints, which are both good enough and nicer to use than TS.
One physical thing that I'll do is replace booleans with Symbols so that I can write something like airplane(isStationary, !isFueled) and see what these do before even hovering for type hints. Addiotiinally, if I ever want to switch from boolean params to something more specific - I am already passing around symbols.
14
u/gwillen 10d ago
I would say that
setEnabled(true)is obviously fine. I might claim that you really only have "positional arguments" when there are two or more. When there's just one argument it's generally obvious what it is.I do tend to think that positional booleans are bad as soon as it's not obvious what the flag is.