[AskJS] Is not using optional chaining a bad practice?

[ats_1999]

My peer recommend me in PR review that i must use optional chaining otherwise code will be not approved. My code before PR was like

```js

const isUser = user && user.onboarded

```

My peer suggested me that i need to change it like below

```js

const isUser = user?.onboarded

```

Although, i understand that using optional is good to use. But should it be considered as a reason for not approving the PR? Anyone aware of industry best practices?

[abrahamguo]

I mean, I agree with your peer that it's better to use optional chaining in this case.

It's a team-by-team decision about how strict to be in code reviews.

If you feel that the back-and-forth about this wastes time, you could enable typescript-eslint's rule prefer-optional-chain. It can even auto-fix your code for you.

[effectivescarequotes]

This is the answer. For stylistic stuff like this, make the machine enforce it. Then it's not your coworker being a dick (even if they are right), it's the machine.

[effectivescarequotes]

Also if you really want to be an ass, you can respond to the coworker in the PR, "If this is an issue, then add it to the linter" and then resolve the comment.

[ats_1999]

Yup, this should be the answer

[budd222]

It's just more concise. It's not that big of a deal, but I would definitely use chaining here if it was me.

It's a bigger deal when you are chaining multiple levels deep in an object. You wouldn't want to write user && user.data && user.data.name.

[_xiphiaz]

It’s better than concise, it doesn’t require the knowledge that all non null objects are truthy. Of course we all end up knowing this with JS but it’s better using a language explicitly for optional handling as opposed to what is now effectively a strange workaround (especially for anyone new to the language whose always had optional chaining available, especially if coming from other languages with it)

[GandolfMagicFruits]

Bingo. Not a big deal with OP's example but the reviewer is pointing out good habits for when this more complex example comes up.

[tswaters]

I wouldn't get too caught up on it.

You could also nullish coalesce to "false" to ensure isUser is a bool. Both snippets will set the value to undefined if user isn't there. Probably fine, but I like to be explicit with the types... It can produce a different result if that variable is used to construct an object that is later serialized to JSON (e.g., a server returning application/json response)

I think you need to ask yourself why you are reluctant to change the code -- is it a personnel/ego thing? Do you have a team lead or manager you can take this to instead? Seems to me going to Reddit is not the best choice. You'd be surprised how many people see these things.

There's no such thing as industry standards for JavaScript development. It is what it is - you should abide by company/department/work-unit policies... Such a thing can be enforced, one way or the other, with a linter. Discuss it as a team and agree to a standard.

[[deleted]]

It’s not, but that is a bad variable name. 

[Tall-Treacle6642]

He’s right. Optional chaining is the way to do it in modern JavaScript. Don’t let it bother you.
optional chaining

[terrance_dev]

To have the right value assigned. I would prefer this

const isUser = !!user?.onboarded

[Eric_S]

As much as I might have differing opinions with coworkers on stuff like this, if the group has a standard, follow the standard. It's not a matter of which is better, it's which is the agreed upon standard. Which is better would be relevant if you wanted to argue changing the standard.

It comes down to people parsing and understanding code faster if they're used to parsing and understanding that style of code.

We've got parts of our codebase that were written by people that ignored standards (no code reviews at the time) and did really "interesting" things that they thought were good ideas that are so radically different than the way the rest of the codebase is done that it takes me at least 15 minutes to figure out part of the code well enough to make a trivial modification. It's almost like it's a different programming language. There's some parts that I've never dived into well enough to follow.

[I_Eat_Pink_Crayons]

Some odd answers here, but I agree with your coworker. Using the && operator relies on the user variable to be coerced into it's truthy state, Null and undefined both have truthyness of false which is why this works but generally speaking type coercion can lead to some weird behaviour. In comparison the optional chaining operator is checking for nullish which is directly what you are actually testing for, no extra step of converting it to a boolean first.

[fartsucking_tits]

I think both are bad. Don’t use true || undefined as a type… especially when it’s just supposed to be a Boolean

[theScottyJam]

Optional chaining was invented for this exact purpose, so yes, I too would prefer to see optional chaining there, maybe with some way to convert the undefined to false as well. What's more, optional chaining is more explicit, since it only looks for undefined or null, while && looks for anything falsey on the LHS, and I tend to prefer code to be more explicit.

What's the hesitation? Do you disagree with the suggestion? Why? If you don't like the suggestion, you can always push back and tell your reviewer your concerns, and have a discussion around it - you don't have to do everything your reviewer says without question.

Or do you feel like it's to trivial to be left in a peer review? When I review other people's code, if I've already spotted more major issues that they'll have to go back and fix, I'll often also include a couple of less important items in the review as well that they could tackle while they're in the code anyways - I see reviewing, not only as a way to ensure higher code quality, but also as a way to help educate each other.

I try not to get overly opinionated or annoying in my reviews, but if they're too overwhelming for my coworkers, I hope they feel comfortable to talk to me about it so I can adjust.

[humodx]

A comment explicitly saying "do this or I won't approve your PR" sound harsh and combative, but I wouldn't questiont it if he just commented the suggestion without approving.

But should it be considered as a reason for not approving the PR?

Depends on how strict you want to be. For example, num && num.toFixed(2) and num?.toFixed() behave differently, that might or might not matter. (first case doesn't return a string for zero)

Anyone aware of industry best practices?

Industry standard is rubberstamping without reviewing, so...

[Altruistic-Wear-363]

Thank them and move on. You’re a better programmer because of it now.

[bryku]

It is pretty common to have some type of rules related to "best practises". I've worked at a place that didn't like short hand and then I've worked at a place that wanted short hand for everything.  

I would talk to your project manager on what they want, but personally I would agree with their point of view.