Changing the view based on these if/else statements

Question

If the user has not responded = edit mode.
If there are responses present but the overall status is 'Completed' = lock everything into display mode.
If the stage is less than or equal to 'editable in stage', it is considered editable and if also the response given has a code of 0 or 2 (standardized responses via "coding" them).
Otherwise set it to display.

This code works as desired but it's hard to read/maintain.

 if (typeof response !== 'string') { mode = edit; } else { if (status.indexOf('Completed') !== -1) { mode = display; } else { if ((stage <= isEditable) && (code == 0 || code == 2)) { mode = edit; } else { mode = display; } } }

You haven't given us anywhere near enough code to help. Here at code review, the more code you post the better. Looking at this I can tell you with a very high degree of certainty that your problem isn't these few lines of code but that your controllers are a jumbled mess. We can't even see the declaration/initialization of the variables that are being used in this block of code. Post everything and then update your title to explain what the code is supposed to do. Then we can do something for you. — Conor Mancone
– Conor Mancone, Commented Dec 28, 2017 at 19:34
It's too much to explain in the title, so I thought stating the goal of my post was better. It's just a matter of cleaning up these if statements; I've given a decent amount of context here. Edit: There isn't a problem anywhere in the code and I stated that it works. It's merely that this is confusing. — Ryan
– Ryan, Commented Dec 28, 2017 at 19:41
Short of using guards, there isn't much to be cleaned up here. Again, I can just about promise that the reason why this seems confusing is because the rest of your code is confusing, and any changes suggested as a result of what you have posted will be nothing more than superficial. If you want to really get some help here, then post more code. — Conor Mancone
– Conor Mancone, Commented Dec 28, 2017 at 19:50
Here's the issue I see, which may be the source of 'confusion' others comment on: You are using four different parameters to control a state machine with 2 states. Consider using a state machine pattern (yes, even in javascript) where your 'mode' can transition from EDIT to DISPLAY as conditions change. What I read in your code says that status isn't needed at all, stage may not be needed either. The machine can likely be driven entirely by the values in 'code' and 'response' variables. — JasonInVegas
– JasonInVegas, Commented Dec 28, 2017 at 20:56

Igor Soloydenko · Accepted Answer · 2017-12-29 23:30:28Z

As others (@conor-mancone, @jasoninvegas) have pointed out, the code is having bigger problems than purely syntax related ones...

There're too many things this little piece of code is looking at once.
It's written in a very low level, instead of abstracting things out. (I.e. why should I bother with code === 0 ...especially in the same line where stage and isEditable are used? Are those even related concepts of the same level of abstraction?)
0 and 2 codes may be familiar to you, but to a regular users they are "magic numbers".

You may not be ready or want to jump into @jasoninvegas' state machine proposal, but you should consider it among the alternatives you have. As the very first step, I'd recommend looking at plain old extract method refactoring...

Syntax-wise I only can suggest a little improvement if you have a luxury of ES6.

Screw the old school endless if-else if-else if-else chains. They don't allow immediate assignment to the target variable/constant. In fact, that construction enforces using var or let instead of a const (which helps avoiding bugs).

Extract the if conditions into tiny predicate arrow functions.
Use those arrow functions in a ternaries chain. This will allow you to have a united const declaration together with an immediate assignment.

Oh, and by the way, use === consistently. Not sure why in some places you use == instead...

const isNotAString = response => typeof response !== 'string'; const isCompleted = status => status.indexOf('Completed') !== -1; const isEditableAndInEditMode = (stage, isEditable, code) => (stage <= isEditable) && (code === 0 || code === 2); // Make `0` and `2` named constants! const mode = isNotAString(response) ? edit : isCompleted(status) ? display : isEditableAndInEditMode(stage, isEditable, code) ? edit : display;

Update 1, for those who are unfamiliar with functional programming. If ternary chaining was that bad, it wouldn't be mentioned Mozilla Developer Network

By breaking the ternary onto multiple lines and making use of extra whitespace, the ternary operator can be used to very cleanly replace a lengthy series of if/else statements. This provides a syntactically light way of expressing the same logic:
var func1 = function( .. ) { if (condition1) { return value1 } else if (condition2) { return value2 } else if (condition3) { return value3 } else { return value4 } } var func2 = function( .. ) { return condition1 ? value1 : condition2 ? value2 : condition3 ? value3 : value4 } 

Update 2 A related blog post. and a comment on it, and another one tell more about ternaries in functional programming.

Another way to look at it is through the prism of a truly FP language, such as Haskell. Here's a link to comparison of if-vs-pattern-matching. In fact, a chain of ternaries resembles pattern matching functionality a lot (yet, not necessarily the same).

Do not stick to the old dogma of "ternary is bad"! Harness it for the win!

Thanks a ton for this response. I used == in a few spots because numbers return as strings in certain cases (Microsoft SharePoint) — Ryan
– Ryan, Commented Dec 28, 2017 at 23:26
@Ryan a better approach would be to clean-up the response from the SharePoint so that the data is standardized in a known format. Otherwise, you can do the conversion inline: (+code === 0 || +code === 2), which will relay your intent better. — Igor Soloydenko
– Igor Soloydenko, Commented Dec 28, 2017 at 23:32
What makes you consider that the better method? I'm not second guessing you - just curious because I've seen it done both ways. — Ryan
– Ryan, Commented Dec 28, 2017 at 23:40
@Ryan if you clean up the data as soon as you get it from the SharePoint, you're introducing a "gate" after which you don't need to guess about what's the type of the thing in the response object. So, cleaning up is preferable... As far as the === goes, it's better because it's stricter than ==. JavaScript is known for it's funny behavior related to == depending on various operand type combination. I personally prefer deterministic and reader-intuitive code to something that may easily break just because the data happened to be different in runtime form the expected... — Igor Soloydenko
– Igor Soloydenko, Commented Dec 29, 2017 at 0:10
I agree and try to use === when I can. Data gets passed around in SharePoint in a lot of ways, so while I agree that adding that 'gate' is smart, I'd have to be careful I'm not manipulating user data unnecessarily. Thanks a lot for answering my question. — Ryan
– Ryan, Commented Dec 29, 2017 at 3:24

A Bravo Dev · Accepted Answer · 2017-12-31 12:54:14Z

First step, remove that first else

if (typeof response !== 'string') { mode = edit; } else if (status.indexOf('Completed') !== -1) { mode = display; } else if ((stage <= isEditable) && (code == 0 || code == 2)) { mode = edit; } else { mode = display; }

After this, your text and your code dont look the same to me... you need better variables or having methods:

var noResponse = typeof response !== 'string'; var statusIsNotCompleted = status.indexOf('Completed') !== -1; var stageIsEditable = (stage <= isEditable) && (code == 0 || code == 2); if (noResponse) { mode = edit; } else if (statusIsNotCompleted) { mode = display; } else if (stageIsEditable) { mode = edit; } else { mode = display; }

After this, is there an option in real life where putting else if (stageIsEditable) before else if (statusIsNotCompleted) make something not work? Otherwise, you could join them in a variable:

var yourThingIsEditable = noResponse || stageIsEditable;

Yep, @Iwrestledabearonce.. Now I've already fixed the indentation to my way (tabs FTW) — A Bravo Dev
– A Bravo Dev, Commented Dec 31, 2017 at 12:56

Malachi · Accepted Answer · 2017-12-28 20:01:33Z

This piece of Code can be nested one level less:

if (typeof response !== 'string') { mode = edit; } else { if (status.indexOf('Completed') !== -1) { mode = display; } else { if ((stage <= isEditable) && (code == 0 || code == 2)) { mode = edit; } else { mode = display; } } }

I believe this is exactly the same as what you have written and it is indented only one time:

if (typeof response !== 'string') { mode = edit; } else if (status.indexOf('Completed') !== -1) { mode = display; } else if ((stage <= isEditable) && (code == 0 || code == 2)) { mode = edit; } else { mode = display; }

Stack Exchange Network

Changing the view based on these if/else statements

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Changing the view based on these if/else statements

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions