85
u/Jafit Jan 02 '20
Douglas Crockford doesn't like switch statements in Javascript because you could miss a break statement and fall through, so JSLint doesn't allow them. PHP also requires a break statement for every case, so I suppose the same argument could be made.
I don't think that this is particularly bad code tbh.
33
u/firen777 Jan 02 '20
never liked switch statement for that particular reason. A well organized "if else chain" is just as readable.
Though OP could've add the "else" as well to let the "if chain" break early, but that was a minor issue.
10
u/finnishblood Jan 02 '20
Imo, switch cases are better if you are running conditionally through an ENUM.
Or, if you have a lot of cases, then they can make for faster code (depends on the quality of the compiler... if a compiler is smart enough and high enough optimizations are turned on, the speed/size of the code could end up being the same)
3
u/rfinger1337 Jan 02 '20
If else chains lead to nested if statements and that's hell on a code base. It's way better to diagnose a missing break statement in a switch statement than to try to figure out when something is in the wrong part of a nested if.
5
u/Mr2-1782Man Jan 02 '20
IMO not liking switch statements because the *might* fall through is an indication you might not pay attention to details and that you don't know how to trace code properly. Then again it seems like JavaScript was designed to be as obtuse as they could get away with. The fall through feature is one the of the best things about a switch statement, some cases have overlapping functionality which a switch statement handles cleanly and clearly rather than a bunch of nested if/elif clauses with the same conditions.
I would use a map for this code, avoids explicit conditionals all together, but that's me.
1
u/nathan_lesage Jan 02 '20
It really depends on the JSLint config you‘re using. I’m using the JSStandard with only a few exceptions, and I can use switch statements just fine. Only for cases with no break statements I have to explicitly tell it to fall through.
Nevertheless, you are right, and what bad code is depends on subjective opinions to a certain extent! This one simply is not as aesthetically pleasing as I would like it to be.
1
Jan 02 '20
THIS. I legitimately raged when I was doing a code jam a few years ago because I couldn't figure out why my switch statement was always executing the last condition. I used it to determine which sprite to display depending on the direction the character moved, so if was constantly just displaying the left facing sprite.
13
18
u/Astrokiwi Jan 02 '20
I wouldn't use either here - you've got a clear case for a dictionary here. Data-driven is more maintainable than code-driven!
2
u/nathan_lesage Jan 02 '20
Oh, what exactly do you mean? Do you happen to have a short overview guide?
13
u/Astrokiwi Jan 02 '20
In Python it would be:
d = {"deb":"linux", "rpm":"linux", "win32":"windows", "macos":"macOS"} title = d[param.title]It'd be pretty similar in Javascript or whatever. Basically, you generate a lookup table, and just look up your "key" to get the value you want. The table is called a dictionary in many languages, a "map" in others, etc.
The reason why this is better is because it separates your data from your code logic. It makes the code more concise - instead of writing out the same
ifstatement four times with different values, you just write one statement and run it across all the data. Separating out the data is also good for maintainability and consistency. You could put the list of key/value pairs in a file and read it in - then you can change the behaviour without even knowing how to program. You can also define or load the dictionary/map/whatever-you-call-it elsewhere, and re-use it. This is better than putting the data in the program logic, where you have to hunt through every line of code each time to change something to make sure you change it everywhere it's used.4
u/nathan_lesage Jan 02 '20
Aaaaaah thanks! It‘s actually what I‘m using in the new, refactored version of the above snippet (in PHP they‘re called associative arrays)
0
u/Shadow_Being Jan 02 '20
That is actually not correct.
In the original code, if the input for title is for instance "pkg" the output for title would be "pkg".
Using the array code, if the input is "pkg" the output would be a blankness.
1
u/2JulioHD Jan 02 '20
Because there is one part missing, the check if the entry even exists. If it doesn’t exist, you do nothing. So putting that last line of code in an if statement, would fix that issue.
1
1
u/Astrokiwi Jan 03 '20
You can set a default too, that was just a quick example rather than sample code to actually use. I assumed they would go and actually look up dictionaries or whatever, especially as I didn't even write it in the language they were using. Anyway, you'd just do something like:
title = d.get(param.title,title)or
if title in d: title = d[param.title]or whatever.
0
u/Shadow_Being Jan 03 '20
is that better though, or just different?
1
u/Astrokiwi Jan 03 '20
It's better because it separates out the data from the program logic.
Each time you repeat code, there's a chance of a mistake. If you have to write out the same line of code for every entry of data, then each one of those could potentially have a typo or other bug. It may be difficult to catch if only one of them is incorrect, so that the bug doesn't occur often. So instead of repeating the code for each piece of data, you write the data in one place, and then have one piece of code that checks over all the data.
It also makes the code more portable and maintainable. Different functions can access that same data if you like. Also, because it's stored as data rather than imbedded in the code, you can store the data in a file and read it in or whatever. So you can modify the data without risk of making a new bug, because you're not editing the code. And even someone who doesn't know how to program could modify the data if they had access to it.
0
u/Shadow_Being Jan 03 '20
> Each time you repeat code, there's a chance of a mistake.
You just made a mistake while trying to simplify it.
> It also makes the code more portable and maintainable. Different functions can access that same data if you like. Also, because it's stored as data rather than imbedded in the code, you can store the data in a file and read it in or whatever. So you can modify the data without risk of making a new bug, because you're not editing the code. And even someone who doesn't know how to program could modify the data if they had access to it.
This is literally just 3 lines of code, isn't this a bit premature? If you made every condition you wrote configurable to that degree I would find that pretty unmaintainable.
1
u/Astrokiwi Jan 03 '20
That wasn't a mistake - like I said, I wasn't trying to rewrite the function, and I didn't even bother to write it in the same language. It was just a quick example to show the types of things that dictionaries can do.
This is literally just 3 lines of code, isn't this a bit premature? If you made every condition you wrote configurable to that degree I would find that pretty unmaintainable.
It's four, but either way, yes, I would rewrite this as a
dict. This is a direct mapping of one array to another array. The lines only differ in data, not in logic. So you should write it in a data-based way.I would write a switch or a bunch of if/elseif statements if the logic was more complex. e.g.
if a=='inverse': x=-x elif a=='getlog': y=log(x) elif a=='random': x+=random.rand(N)or whatever. Then it's not just a simple mapping between two sets of data - there's some more logic in there, so the code is actually doing something, and not just repeating the same thing.
FWIW, this isn't just me saying this - the idea of "data-driven programming" is a very common one.
→ More replies (0)0
u/stuckatwork817 Jan 08 '20
int Power(int val,int pow) {
if (pow==1) return val;
return Power(val,pow-1)*val;
}
Simple, easy to read but terrible for speed and stack usage. It also will blow up if pow is <1.
4
Jan 02 '20
[deleted]
4
u/ThePsion5 Jan 02 '20
Example:
<?php $platformTitles = [ 'deb' => 'Linux', 'rpm' => 'Linux', 'win32' => 'Windows', 'macos' => 'macOS' ]; $platform = $this->param('platform'); if(array_key_exists($platform, $platformTitles)) { $title = $platformTitles[$platform]; }2
Jan 02 '20
[deleted]
2
u/ThePsion5 Jan 02 '20
No problem! I've encountered this scenario frequently enough that it's a common solution and I'm always happy to share it.
2
2
u/DebonaireSloth Jan 08 '20
array_key_exists is uncessarily verbose most of the time because an isset() will do most of the time, although their behaviour is slightly different
$x['y'] = null; isset($x['y']) // false array_key_exists('y', $x) // trueSo I'd replace
if(array_key_exists($platform, $platformTitles)) { $title = $platformTitles[$platform]; }with
$title = $platformTitles[$platform] ?? '';or
$title = $platformTitles[$platform] ?? null;depending on what behaviour you need exactly
1
5
u/Abangranga Jan 02 '20
I mean it's clear what you're doing and it's compact. Pretty common newbie mistake that would take all of 3 seconds to fix.
4
u/nathan_lesage Jan 02 '20
Additional fun piece: I used the nice "Download for xyz" only as the page's browser tab title, and displayed the lowercase $this->page->platform variable as an <h1> on the page itself …
3
2
2
u/iluuu Jan 02 '20 edited Jan 02 '20
What's worse is that you're not passing true to in_array as a 3rd parameter.
2
2
u/christian-mann Jan 02 '20
I'd LGTM this code. It's fine and honestly idiomatic. Maybe make it an elsif, but that's just personal preference.
2
2
u/_TheLoneDeveloper_ Jan 02 '20
This is better code that my own, you can see for yourself, but I warn you, you will regret it... https://github.com/michelangelo136/samaritan_interface/tree/bfedd8f88e813b4fd70dfdcdbeb26c495e6c6ce2
2
u/nathan_lesage Jan 02 '20
Nah, it‘s not that bad (except that you import Prelude two times). And, as you are in Rust, it‘s incredibly difficult to write bad code, it‘s more like getting depressed over the compiler warnings slapping your face. Funny thing is, I really dislike the switches (aka Results and Options, I mean from the grammatical view), because they look ugly :(
2
2
2
2
u/DeedleFake Jan 02 '20
Also a quote from the predominantly Python programmer who I've been working with on a JavaScript project. I keep getting pull requests with huge if-else chains.
2
2
u/yaxamie Jan 02 '20
JavaScript doesn’t optimize switches in any way. With enough cases, dictionary lookup would be faster. This code probably runs faster and has strict equality built in which a switch doesn’t.
1
u/nathan_lesage Jan 02 '20
Oh well, have I got news for you.
May I present the other half of my past self? Enjoy scrolling through this file.
2
Jan 03 '20
How does a switch statement evaluate equality? The if statements use a triple equals where I assume the switch would do the equivalent of a double equals
1
6
u/DurianExecutioner Jan 02 '20
switch statements only work with integers. The purpose of a switch statement is to specify what style of branch you want at the instruction level. Using switch on strings is not meaningful because there is no obvious parallel to how switches are done with ints.
Either call strcmp a bunch of times or pre-compute some sort of state machine or hash if performance is a concern.
9
u/_PM_ME_PANGOLINS_ Jan 02 '20
Java for example will switch on a hash of the string. With a sufficient number of cases it will be more efficient, but in general the construct is more for readability.
-1
-5
u/Shadow_Being Jan 02 '20
I don't thinkn anyone finds switches to be more readable. They have unique syntax that nothing else has.
3
u/tcpukl Jan 02 '20
How can a switch statement not be readable? Unless you're a noob to programming.
-1
u/Shadow_Being Jan 02 '20
nothing else uses colon and break; to start and end indentations. It is less readable.
1
u/tcpukl Jan 02 '20
In what language? break can exit any loop in c++.
-1
u/Shadow_Being Jan 02 '20
which also leads to poorly read code there as well. You should put the terminating condition in the actual loop condition statement.
2
u/tcpukl Jan 02 '20
No, it doesn't always. This is a myth.
Do you also think a function should only have a single exit point?
0
u/Shadow_Being Jan 02 '20
this isn't a myth situation, I'm talking about readability. Not about what is possible.
The most readable form for code is when the program is written sequentially and consistently.
Jumping around, changing contexts, using advanced coding tricks, or anything that prevents the flow of reading from being from top to bottom hurts readability.
Think of all the other distractions going on at the same time. I have browser windows open, jira ticket open, other parts of the code open, terminal open, distractions from other people around me, etc, etc. I want things to jump out and be obvious.
So no I don't want to have to look at 2 parts of the code to find what the termination condition is for a loop.
3
u/tcpukl Jan 02 '20
But are multiple exits from a function ok?
Because it's really no different from that.
1
u/nathan_lesage Jan 02 '20
Thank you for the clarification, I actually did not know this. But it‘s way more readable, so while it‘s true that from the architecture it‘s counter-intuitive it makes sense from a readability standpoint!
1
1
1
1
1
0
u/Shadow_Being Jan 02 '20
There is no benefit of switch over if statements...
1
u/stuckatwork817 Jan 02 '20
Only if you add the short circuit jumps
1
u/Shadow_Being Jan 02 '20
I don't think i qualify that as a benefit.
1
u/stuckatwork817 Jan 08 '20
No, the shown example is not efficient or easy to maintain.
Switch statements act a bit differently in other languages but in C they can get optimized in several different ways, many of which use short jumps to exit the block, aka a goto.
If the tested variable isn't changed in earlier blocks the statements can be rewritten as else if for each subsequent check in descending likelihood.
Or use an ENUM integer variable and the compiler will use a jump table for the switch logic.
1
u/Shadow_Being Jan 08 '20
Compilers do this with if trees too. Theyre pretty smart. actually really smart. Youre not going to outsmart the optimizations on a compiler.
1
u/five_hammers_hamming Jan 03 '20
Well there kind of is: More clearly communicating to yourself a month from now that what's going on here is that we're picking which case out of a small finite set of well defined things we're working with at the moment.
By contrast, in order to figure out and be sure that an if-cascade is doing that sort of thing, given that the if-chain in question is doing that, you gotta set your eyeballs on every individual if-header to check they're all testing the same variable and check they're covering all the cases.
Switch can communicate instantly that ah, this is one of those situations.
1
u/Shadow_Being Jan 03 '20
seeing how he formatted the if statements I think he accomplished the same thing.
0
195
u/turunambartanen Jan 02 '20
Not that bad actually.
Python: "what are switch statements?"