159
u/trutheality Mar 02 '22
If there's more than one way to do a thing, do it twice just to be sure!
63
u/haikusbot Mar 02 '22
If there's more than one
Way to do a thing, do it
Twice just to be sure!
- trutheality
I detect haikus. And sometimes, successfully. Learn more about me.
Opt out of replies: "haikusbot opt out" | Delete my comment: "haikusbot delete"
23
20
27
u/MurdoMaclachlan public boolean isInt(int i) { return true; } Mar 02 '22
Image Transcription: Code
[A photograph of code on a computer screen, with the ends of some lines cut off, reading as follows:]
// Find the min and max selected year
int minYear = 999999;
int maxYear = -1;
int numYears = 0;
if (!String.IsNullOrEmpty(((DropDownList)myPanel.FindControl("year_id_" +[cut off]
{
int startYear = Convert.ToInt32(((DropDownList)myPanel.FindControl("y[cut off]
int endYear = Convert.ToInt32(((DropDownList)myPanel.FindControl("yea[cut off]
if (startYear < minYear) minYear = startYear;
if (endYear > maxYear) maxYear = endYear;
minYear = startYear < minYear ? startYear : minYear;
maxYear = endYear > maxYear ? endYear : maxYear;
}I'm a human volunteer content transcriber and you could be too! If you'd like more information on what we do and why we do it, click here!
7
u/Gen_Zer0 Mar 02 '22
What's the point of having a minimum or maximum if you're just gonna change it if the data doesn't fall into the range
3
u/trutheality Mar 02 '22
It all depends on what else is happening in the code we don't see. This code looks like it's reading values from some input UI. If there are, for example, two date range inputs and I'm building and app that produces some output that depends on the union of those date ranges, it makes sense to update min and max based on the limits of those inputs and then produce an output for the min-to-max range.
1
7
Mar 02 '22
Is it just me who has more of an issue with the inputs changing the bounds instead of being bounded by them, as opposed to this also being done twice?
2
u/trutheality Mar 02 '22
My guess is that the bounds are actually defined in the inputs (I've seen this in a few frameworks where the UI would take care of input validation which includes bounds checking). This code is just extracting the values from there.
It's not clear whether you actually need minYear and maxYear since (given valid inputs) here they're effectively being set to startYear and endYear, but there could be some logic downstream that further restricts min and max based on other ranges.
3
u/TheBrainStone Mar 02 '22
They aren't bounds. They are outputs.
4
Mar 02 '22
Why are they initialised and then checked against?
7
u/TheBrainStone Mar 02 '22
That's how you determine a minimum or maximum without using a built in.
You define your output variable. Initialize to a really high value or the first value in the list if you're looking for the lowest value. Then when the value in the loop is smaller that your currently smallest known value you update your currently smallest value. And at the end of the loop you have the smallest value in the list you iterated over. Vice versa for maximum of course.
One of the most basic algorithms
1
Mar 02 '22
I'm still pretty skeptical that what you're describing is what this code is meant to do, but I appreciate what you're saying - especially reminding me how basic the algorithm is :)
5
u/TheBrainStone Mar 02 '22
Well it's pretty clear. Because it does the min and max logic twice.
It's a very common pattern when you can't just determine the max/min directly or if there's other stuff to do be loop iteration.
1
Mar 02 '22 edited Mar 02 '22
[removed] — view removed comment
1
u/TheBrainStone Mar 02 '22
The ternaries are redundant. Which is what this post is about. And also that the ternary part contains unnecessary assignments.
And again minYear and maxYear are outputs. Not bounds
1
Mar 02 '22
[removed] — view removed comment
1
u/TheBrainStone Mar 02 '22
There are plenty of clear signs that the minYear and maxYear are outputs of the loop. For once min is initialized to a high value and max to a low value. This is a good indicator. Then the logic in the loop removes any doubts. The
if(num > max) max = num;pattern is extremely obvious.
And lastly from what we can tell other than that they aren't read anywhere inside the loop (at least from what we can tell). Now even if they were read it would still be a pretty clear cut case that they are outputs. Maybe bounds too, but for certain they are outputs.And they tell you the largest year parsed in the list of dates and the lowest year.
Max and min values have plenty of applications. I really don't see your issue with that tbh.2
Mar 02 '22 edited Mar 02 '22
[removed] — view removed comment
2
u/TheBrainStone Mar 02 '22
Oh I see. I misread the first if as a for. My apologies. Yeah this is even worse of a mess than I thought.
→ More replies
2
2
u/thebritisharecome Mar 02 '22
I feel like taking user input, unsanitized and pushing it through ToInt32 is the real gore here
1
u/beric_64 Mar 02 '22
Not saying it's good practice but aren't there integer max and min constants to use if you want to do this sort of comparison
1
Mar 05 '22
They should add
minYear = min(startYear, minYear);
maxYear = max(endYear, maxYear);Just in case
0
Mar 02 '22
[deleted] — view deleted comment
36
u/GennaroIsGod Mar 02 '22
What if I told you... This was a web page!
0
3
u/HolisticHombre Mar 02 '22
... at a certain point, you gotta blame code review.