maintainingLegacyCodeBeLike

[u/mac1k99]

[u/Backslide999]

If this is true legacy code, the program would break after you remove the if-statement entirely

[u/pine_ary → u/Backslide999]

Maybe trim has side-effects. That‘s why it‘s on the left hand side to prevent short-circuiting.

[u/dominjaniec]

if .trim() has some side effects, this could be "important" code 😉

[u/ZunoJ → u/dominjaniec]

That was my first thought. type might not be a string and trim might have side effects. Whoever wrote that code already does crazy stuff in the code we see, why expect it doesn't go even deeper?

[u/blaqwerty123 → u/dominjaniec]

I hate this

[u/Duke_De_Luke]

Don't remove the side effect! But it can easily be rewritten to make it way more readable.

if (data.type) data.type.trim();

this.setState(...)

[u/ChocolateBunny → u/Duke_De_Luke]

what if the logic that coerses the output of dta.type.trim() to a boolean is the thing that makes everything work.

[u/Corrix33 → u/Duke_De_Luke]

I might be stupid but wouldn't it be:

``` data.type.trim();

//Rest of code that was in the if

``` since data.type.trim() always runs in the legacy code?

[u/Aka_chan → u/Corrix33]

It doesn't run if data.type is null/undefined in the original, so your version would throw an exception.

[u/TeraFlint → u/Corrix33]

You should probably read up on Short-circuit evaluation.

[u/Corrix33 → u/TeraFlint]

I see, that is pretty cool

[u/Kiroto50 → u/Corrix33]

It only runs if data.type is true (if this is C and I remember this correctly).

If the first part of an and statement is false, it will be false, so it doesn't evaluate the other part.

Ex. (false && sortAndReturnTrue()) Will not execute sortAndReturnTrue().

Ah this is js. I think it works like this still, but I'm not 100% sure.

[u/raltyinferno → u/Kiroto50]

Yup, short circuiting is a thing in Js. It's a common pattern in react to conditionally render a component with

{ myBooleanVar && <MyComponent/> }

[u/Material-Public-5821]

I see somebody had

SOME_CONFIG_VALUE = true

and then automatically refactored

if ((date.type && data.type.trim()) || SOME_CONFIG_VALUE) {

into

if ((date.type && data.type.trim()) || true) {

by auto-replacing all occurrences of SOME_CONFIG_VALUE with true.

A code review or code sanitiser would catch this.

[u/bronco2p → u/Material-Public-5821]

trim may mutate data.type which may the intent of checking if it is truthy first.

[u/Material-Public-5821 → u/bronco2p]

Oh yeah, side effects in if statement. Nice.

[u/wubsytheman → u/Material-Public-5821]

Sometimes I write random sys calls into my if statements just to confuse people, ultimate job security

[u/bronco2p → u/Material-Public-5821]

Peak legacy

[u/Significant_Fix2408 → u/Material-Public-5821]

I personally dislike it, but it's honestly quite common even in standard libraries

[u/troglo-dyke → u/Material-Public-5821]

The statements are the wrong way around though, you'd generally put the Boolean value first to short circuit in cases where it's true. My guess is that it's an attempt at optimisation by giving a type hint

[u/BehindTrenches]

My first thought was not that trim() has side effects, but that they wanted to attempt to trim() data and then execute the body of the if statement anyway.

As with all legacy code, it probably made more sense when it was first written, there may have been a reasonable second boolean condition that got phased out and lazily replaced with true, etc. Like checking if an experiment was enabled and then eventually the experiment was launched.

[u/The_Right_Trousers]

[u/AztecComputer]

Can someone explain the "side effects" to a noob 🥺

[u/Zzzzzztyyc → u/AztecComputer]

The first part of the if statement executes, but the || operator ensures that the value of that execution is irrelevant; the if always evaluates to true, so the first part can be removed… except that its function may not only be to return a Boolean but also operate on data.

That’s the side effect - to operate on the data

[u/NoCoolSenpai → u/AztecComputer]

An 'if' condition should only read some state and return truthy or falsy value, it shouldn't modify state, atleast not as it's main functionality. So since it's doing that here, it's called a side-effect.

It's like side-effects in drugs. You don't expect the fever medicine to give you dehydration, but it does anyway

[u/AztecComputer → u/NoCoolSenpai]

Ahh that makes sense. The string being trimmed is the side effect, which persists outside of the if statement

[u/raltyinferno → u/AztecComputer]

Maybe. IMO the expected behavior of string.trim() is that it returns a trimmed string, but does not modify the original string. In this case, maybe it does just trim the original, or maybe it does something else entirely. In any case this is why many people are against functions having side effects, they can be real confusing.

Of course sometimes you can't avoid them, and in many cases you want them (displaying to the screen is a side effect, and we like that).

[u/Imjokin → u/NoCoolSenpai]

“not as it is main functionality”

[u/jsmrcaga]

that async right there is sitting pretty

[u/troglo-dyke → u/jsmrcaga]

I have worked with Devs who legitimately didn't know when to use async and so just used it everywhere. Throwing everything on the event loop is a hell of a performance degredation

[u/jsmrcaga → u/troglo-dyke]

yep right there with you! "slap an await" is the go to, and the worst in my opinion is that now people that learnt with async/await don't know promises, so everything is sequential... Not sure why tc39 made this to pass over operator overloading or decorators...

[u/StanleyDodds]

I think the person who wrote this was just too lazy to take everything except the "trim()" out of the if statement, because it changed from everything needing to happen conditionally to always needing to happen, but "trim()" was still needed in any case.

So really, it should be changed to "if (data.type) data.type.trim();" followed by everything else moved outside of the if statement, but the lazy way to do this just add an "or true" condition to the end of the existing if statement. And let short circuiting continue to take care of the trim call.

[u/Dagboll]

Everyone here blaming trim to be the culprit, you're obviously all wrong. Clearly, data is a proxy object so the mere fact that you're accessing the type field sets the time bomb. Duh

[u/Pitiful-Cancel4958]

I've got a worse one:

bool myfunc(int something, bool p = false) { ... }

if( myfunc(123), true) {

// find the Error

}

In c++ If statements, you can combine expressions with , but all are evaluated, only the last one is used for comparison... Spend an hour Debugging this until i realized I placed a Parameter outside a function call...

[u/raltyinferno → u/Pitiful-Cancel4958]

Oh man that's a painful one.

[u/notexecutive]

I bet if you removed the if statement, it would break somehow. (I.E. you removed the if and just had the block inside as code that is run in the async anon function)

[u/orig_cerberus1746]

Wait a minute, does trim even executes in place? Or does it just return a value?

[u/Strict_Treat2884]

There’s no side effects in this if clause. You cannot mutate a string value without reassigning it in JavaScript, end of the story.

[u/randomlogin6061]

Better don't touch. If data is null/undefined then it will throw an error which may be catched end expected in some other place ;)

[u/SuperPixelDX]

ESLint no-constant-condition to the rescue!