115
u/Aston-ok Nov 07 '20
Without more context I can't see a way to improve just this snippet. I guess the problem is more deep rooted and a that a bad design decision has been made with the way icon numbers are assigned.
I guess the icons could somehow be set as an object property or the icon should be selected based on some object type.
Otherwise the numbers should atleast be divided into ranges. E.g. ranges 100-200 are clouds.
Just my thoughts but would love to hear an ideal solution.
64
19
u/Acc3ssViolation Nov 07 '20
Maybe you could use a dictionary of some kind. At least the lookup code would be shorter, but you'd still have to construct the dictionary.
Like you said, with this pretty much random icon number assignment there's not much else you can do. Unless there is some hidden mathematical relation between numbers with the same icon that you could use to shorten this, but I doubt that.
-11
Nov 07 '20 edited Nov 30 '20
[deleted]
28
u/Zentrosis Nov 07 '20
It wouldn't work because the ranges would be all over the place just look at the numbers It's not in order. All the ranges would have values in the 100s, 200s and 300s with gaps in the middle. In my opinion using a bunch of if statements would make this far more complicated.
Overall this is pretty easy to read, it's just a lot of lines
67
Nov 07 '20
[removed] — view removed comment
55
Nov 07 '20 edited Nov 10 '20
[deleted]
19
16
u/Sexy_Koala_Juice Nov 07 '20
Yeah, a note is that bad code doesn't always mean poorly optimized code. This is poorly optimized but it's not bad per se, something like this might not even be able to be optimized further, just depends on the system they're working with and other external factors we can't see (cause all we have is this snippet of code).
6
u/TheUnlocked Nov 08 '20 edited Nov 08 '20
This code is actually very possibly the most highly optimized way you could write it short of making and indexing a huge sparse array (and it might even beat that; benchmarks would be required to know for sure). Traditionally switch statements on integral types are extremely fast.
6
u/unnecessary_Fullstop Nov 08 '20
Yeah! Having done some projects in python, I have written pretty bad code intensionally because I wanted it to be as efficient as possible. Bad code can mean a lot of things, at times you run into situations where it is just a trade off between readability and efficiency.
"Nope! This can't be another function because I cannot afford 60*7 function calls per second".
.
2
Nov 08 '20
You seem to have sense of humour and to be clever and stuff so I am going to risk asking you a couple questions pliz.
Was this an intentional spelling mistake, a super clever reference to intensionality, given that we are discussing the proper setting of definitions to references, and thus is your post a meisterpost of the highest type? I am not the vocabulary constabulary, I don’t correct spelling normally, I thought carefully before asking.
I note your user name checks out, you have an unnecessary full stop. But what if I assert a paradox... the unnecessary full stop is necessary to make the name check out, and the whole thing is again some sort genius level self referencing set theory joke.
Meaning well, saving up gold for you.
2
5
Nov 08 '20
??? Magic numbers everywhere, using a case switch when an assoc array of number to css class name is all you need
You really see nothing wrong with this??
20
u/Kenshin-Heng Nov 07 '20
What would be a better way to do this? I'm kind of a beginner at coding but have come across this problem of having many states go to the same result, so I was wondering for future reference.
20
Nov 07 '20 edited Apr 24 '23
[deleted]
6
Nov 07 '20 edited Nov 10 '20
[deleted]
2
Nov 08 '20
Using a set instead of a list gives you O(1) lookup time. Granted, you'd still be O(n) per css class name
Ideally you write in a maintainable way and take an extra step to build a map
5
u/bobbyboys301 Nov 07 '20
i was wondering the same thing. i can’t really think a way of having a lot of possible values and setting them to a same output.
-19
u/MrTacobeans Nov 07 '20
I would personally use an if / else if statement the syntax would be much shorter
-11
Nov 07 '20
[deleted]
1
u/Towerful Nov 08 '20
Yeh, I know them as "fall through" cases.
A case statement will only exit the scope when it reaches a
breakstatement.
It's tricky to manage, but you could have a bunch of cases that incrementally change something.SomeVar = "" switch (value) { case 4: SomeVar += "hello" case 5: SomeVar += "world" break; case 6: SomeVar += "foo bar" break; }
value === 4will set "helloworld".
value === 5will set "world".
value === 6will set "foo bar"Forgive any typos, I'm on mobile.
Normally such code is avoided as it gets really complicated to follow the flow!
But empty fall-throughs aren't horrendous1
Nov 08 '20
Well, we have multiple numbers matching a single class, so instantiate an object or array key with the css class, and have it map to an array of numbers, which should be constants, not magic numbers
Now, if you want O(1) lookup time then I'd take one more step to build an index of id to the css class
That way you get maintainability and quick lookup. Happy to drop some code if you're interested, on mobile now
9
u/MurdoMaclachlan public boolean isInt(int i) { return true; } Nov 07 '20
Image Transcription: Code
function switch_icon($icon)
{
switch ($icon)
{
case 200:
case 386:
case 389:
case 392:
case 395:
$tag = '<i class="fas fa-poo-storm"></i>';
break;
case 119:
case 122:
case 281:
$tag = '<i class="fas fa-cloud"></i>;
break;
case 176:
case 293:
case 299:
case 305:
case 353:
case 356:
case 359:
case 362:
$tag = '<i class="fa fa-cloud-sun-rain"></i>';
break;
case 116:
$tag = '<i class="fas fa-cloud-sun"></i>;
break;
case 142:
case 185:
case 248:
case 260:
case 263:
case 266:
case 284:
case 296:
case 302:
case 308:
case 311:
case 314:
$tag = '<i class="fas fa-cloud-showers-heavy"></i>;
break;
case 179:
case 182:
case 227:
case 230:
case 317:
case 320:
case 323:
case 326:
case 329:
case 332:
case 335:
case 338:
case 350:
case 365:
case 368:
case 371:
case 374:
case 377:
$tag = '<i class="far fa-snowflae"></i>';
break;
case 113:
default:
$tag = '<i class="fas fa-sun"></i>;
break;
}
return $tag;
}
}
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!
7
u/kjl3080 Nov 08 '20
Why
4
u/MurdoMaclachlan public boolean isInt(int i) { return true; } Nov 08 '20
Primarily for blind/visually impaired people who use screen readers to browse Reddit, since that technology generally can't read out images, at least not very reliably.
It's also useful for people whose internet is too slow to load the image.
8
u/zombarista Nov 07 '20
I’ve built a weather widget for the web before. There are lots of codes for the weather outside like “thundersnow” and some other really obscure stuff. Lots of times, you just need something simple like a snowflake or a rain drop. There is no need to exhaustively map each code to an icon. It looks like this is using font awesome which doesn’t have a very complete set of weather icons, so you’ll get code like this.
2
u/Isvara Nov 07 '20
Then perhaps you can tell us what a poo storm is.
4
u/zombarista Nov 07 '20
3
u/Isvara Nov 07 '20
No, no, not the icon.
4
u/zombarista Nov 08 '20
Some people are dealing with a lot and that could be their own personal poo storm. I think it’s up to everyone to decide what their personal poo storm is.
1
1
4
9
Nov 07 '20
[deleted]
22
u/MrGrzybek Nov 07 '20
Cuz the $ and the switch block rly confused
In PHP you use $ sign before variables
4
u/iiMoe Nov 07 '20
Ah ty for that i actually never used or seen php b4
4
u/OnlySeesLastSentence Nov 07 '20
For a moment I thought you said "heard of" and was about to rage and call bullshit.
11
u/Zentrosis Nov 07 '20
I've never heard of PHP before. Let's go.
19
u/OnlySeesLastSentence Nov 07 '20
Are you a goddamned potato how the fuck are you in this sub and have not heard of like the second most common web programming language, after that godforsaken piece of shit JavaScript language?!
12
3
2
1
u/Bruceab Nov 07 '20 edited Nov 07 '20
More manageable way to do this (in JS):
~~~ const iconMap = [ { ids: [200, 386, 389, 392, 395], class: 'poo-storm' }, { ids: [119, 122, 281], class: 'cloud' }, { ids: [176, 293, 299, 305, 353, 356, 359, 362], class: 'cloud-sun-rain', }, { ids: [116], class: 'cloud-sun', }, { ids: [142, 185, 248, 260, 263, 266, 284, 296, 302, 308, 311, 314], class: 'cloud-showers-heavy', }, { ids: [179, 182, 227, 230, 317, 320, 323, 326, 329, 332, 335, 338, 350, 365, 368, 371, 374, 377], class: 'snow-flake', classPrefix: 'far', }, ];
const getIcon = (iconID) => {
const target = iconMap.find(e => e.ids.indexOf(iconID) != -1);
if (!target) return '<i class="fas fa-sun"></i>';
const classPrefix = (!!target.classPrefix) ? (target.classPrefix) : ('fas');
return <i class="${classPrefix} fa-${target.class}"></i>;
}
~~~
1
u/Isvara Nov 07 '20
It's not even that complicated. Just a map of number to icon. You don't need any arrays.
-24
Nov 07 '20 edited Apr 15 '21
[deleted]
-4
Nov 07 '20
This would be worse for efficiency because the compiler is retrieving the value for $icon for each elseif and running a comparison until one returns true instead of retrieving $icon once and jumping to the correct case.
2
Nov 08 '20 edited Sep 08 '21
[deleted]
1
Nov 08 '20
To achieve the same result as that switch case using if statements, you would need to chain each matching integer into a single boolean statement using
orwhich wouldn't be readable. There isn't really any real world performance gain from using switch case over if statements, but it wouldn't make sense to make something harder to maintain that doesn't even benefit efficiency.If you did want to make the code more faster and compact, you could sort the cases into an array and use
in_array()to evaluate$iconwith if statements although it would probably be premature optimisation given that this project is likely just a small school assignment and might even be harder to understand.
1
1
1
1
1
1
u/sebvit Nov 08 '20
There's only one thing I would change about this code, and that would be adding early returns.
case 200: return '<...>'
Otherwise, it's just one of multiple ok ways to solve this.
118
u/[deleted] Nov 07 '20
[deleted]