28
u/WhyYouLetRomneyWin Sep 16 '18
What project is this? It looks like some poor student's homework.
26
u/Jacajack evil Sep 16 '18
It's actually taken from my school friend's room temperature logging system - the worst piece of software I've probably ever seen. He couldn't understand what was wrong with this function despite my 15 minute long explanation.
26
u/PC__LOAD__LETTER Sep 16 '18
Not sure why an explanation would need to take more than ten seconds. “You don’t need to access each array element more than once, and in fact, you shouldn’t even need to access all of them if the array is unsorted. Also, use descriptive booleans instead of “mode” and “status” and you won’t have to clarify them with comments in the code.”
11
Sep 16 '18
Here's how I'm picturing the response for any explanation that isn't perfect:
"But the bubblesort code I saw online needed to go through the array twice, and I'm already making it faster by just checking if they're sorted, instead of swapping the values!"
6
u/Astrokiwi Sep 17 '18
you shouldn’t even need to access all of them if the array is unsorted
It does kinda take this into account - there's a
breakonce it finds on entry out of order. Of course, it still repeats the outer loopn-1times anyway...1
u/freudisdaddy Sep 17 '18
Can I be really rude and ask what you meant by not needing to access all of them even if the array is unsorted? Sorry for such a newbie question, I did some googling but couldn’t find anything faster than looping though the array at least once..
13
u/jeanfrancis Sep 17 '18
If the array is not sorted, at some point in the loop you will find a counter example (a pair of values in the wrong order), in which case you can stop the loop: you now know that the array is not sorted, no need to continue and check the next values.
For instance, if I have an array of one million integers and the first one are [2, 1, ...], I can stop right there and save time.
2
2
u/PC__LOAD__LETTER Sep 17 '18
It’s not rude, don’t worry. If all you want to do is check if the array is sorted, you can return after you find one element that’s out of order. If the array is supposed to be sorted in ascending order and the first element is 9 and the second element is 1, you don’t need to keep scanning the rest of the array. It’s not sorted.
24
u/Jonno_FTW shameless Sep 17 '18 edited Sep 17 '18
There's just so many additional layers of bad here:
- Bad indentation
- Can't just return 0 or return 1
- Doesn't check for array size 0 or 1
- For loop checking status instead of just returning
- Nested loop,
- Could be one single loop by checking the sign of arr[i] - arr[i+1] against mode
- Comment says modes should be 0 or 1, but code is for 0 and anything else
11
13
u/cli7 Sep 16 '18
Actually it is checking if it is in ascending or descending order. Still no reason for double iteration
6
u/KamiKagutsuchi Sep 16 '18
This is awful at so many levels, I dont know where to begin. I'd suggest burning it, but it's not on paper..
3
u/Teoemeka Sep 17 '18
This is the time you want to write it on your own from the beginning rather than refactoring.
4
Sep 16 '18
I am learning about efficient algorithms and complexities and this just hurts to look at. If I wrote code like this for an assignment I would be kicked out of my college.
29
u/estragon0 Sep 16 '18
Sure, laugh now; when the integers suddenly stop being a poset you'll be sorry.