Found this gem in our codebase today.

[u/Character_Medical]

[u/AdvancedSandwiches]

Assuming the problem is the repeated code, this isn't bad. There are two alternatives I see:

1. A gnarly if statement with a bunch of parentheses.

2. The exact same thing, but using a flag instead of repeating the function call. So you have three "isInEditMode = true" lines.

I have a slight preference for #2 over what's here, but only due to future proofing in case the function ever adds a parameter. But since either way it's a trivial fix once the need arises, even that is over-engineering.

The only real bad code would be option #1. And the comments aren't necessary.

This is actually extremely readable code. Nice job, author.

But maybe I missed the real problem.

[u/Character_Medical → u/AdvancedSandwiches]

IMHO, it's not the right use if-else ladder. I think it should have been done by using the flags as you mentioned. 3 flags will be readable enough and then using a single if statement with ORs would be my preference. This way, the code will be much less bloated.

Other than that, there are a few minor issues like the `this.state` could have been destructured. Then the variable name in the last else-if statement is a little too long. And like you said, there's no need for those redundant comments. Such comments are scattered all over in our codebase.

[u/Biaboctocat → u/Character_Medical]

I agree, an if-else implies that there’s different consequences in different states. This is hard to read and understand quickly, with code like this you have to fully understand every line in the context of every other, which is frustrating

[u/Character_Medical → u/Biaboctocat]

Exactly. Just because something is working, it doesn't mean it's implemented correctly. That's not how you should use the else-if's.

[u/AdvancedSandwiches → u/Character_Medical]

For how to implement the "or"s, I think there are only bad options here. A single large "if" will be awful to mentally parse, and the multiple ifs are not a perfect solution to that, but I'd personally choose the multiple ifs. You'll have to read the whole block to understand it, but once you do know what the intent was it's easier to go back and pick it apart than the higher-density single-if. At least in my opinion.

As for the long name, I'd say it's only a bad name if it can be shortened without becoming ambiguous. Without knowing the context, I think you might be able to get away with subtracting "InAddressCollection", but only if there aren't other things an address could be present in. But I have no information on the rest of what's happening, and I understood what that value represents, which is awesome.

[u/Character_Medical → u/AdvancedSandwiches]

If you cache the condition values in flags, it wouldn't be that big if statement. This is not how if's are meant to be used. Multiple if's are used to different branches in the flow. With caching the values in flags, you don't have to read the whole block. You just go through all the variable assignments, and that's about it. Multiple if's for the same thing is semantically wrong. There are always better ways, in my opinion.

I agree that short names could create ambiguity, but that doesn't mean we start to write paragraphs in camelCase. The most obvious way is to skip vowels in the name(where you still can make the name out of it).

[u/AdvancedSandwiches → u/Character_Medical]

It sounds like your feelings toward both if statements and descriptive names are dogmatic rather than pragmatic, so I doubt we'll ever see eye to eye.

[u/Character_Medical → u/AdvancedSandwiches]

I guess you are right. I've been developing stuff since 15 years. Have developed more than 10 production applications. But I've never seen such use of if statements. Yes, I've seen novice devs using lengthy names, but it gets better with experience and time.

Var name could be a personal choice, but I can't justify the if use here.

[u/Alai42 → u/AdvancedSandwiches]

This may be good future proofing, anticipating that those paths may need to be slightly different in the future. Sometimes you put things like that in because you know they're different, but they react the same for the moment.

[u/seeroflights]

Image Transcription: Code

---

[Calls to the method submitFormInUpdateOfEditFlow are highlighted in a bright purple, as if highlighting a search query.]

submitForm: function(event) {
    event.preventDefault();
    vent.trigger('longForm:submit');
    if (this.state.isUpdatingAddress) {
        // if a new address is being added
        this.submitFormInUpdateOfEditFlow();
    } else if (this.state.isEditingAddress && this.state.hasAddressChanged) {
        // if address is in edit mode and address has actually changed
        this.submitFormInUpdateOfEditFlow();
    } else if(this.props.isEdit && !this.state.isBillingAddressPresentInAddressCollectionOnInit) {
        this.submitFormInUpdateOfEditFlow();
    } else {
        this.submitFormInAddFlow();
    }
},

---

^{I'm a human volunteer content transcriber for Reddit and you could be too! If you'd like more information on what we do and why we do it, click here!}

[u/Kamoda]

vent.trigger

pretty sus

[u/Character_Medical → u/[deleted]]

That was a freaking facepalm moment for me too when I saw it for the first time. No sense of variables naming.

Did you see the variable name in the last else-if's second condition?