- Feature Articles
- CodeSOD
- Error'd
- Forums
-
Other Articles
- Random Article
- Other Series
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
I suspect that the original developer saw some similarities between a person's birthday and the use case, found a bad birthday snippet, and used it.
Admin
An off by one error is that they are declared as being one year old before their frist birthday. Variable 'now' not used. If run on their birthday, it adds an extra year since the reference time is midnight.
Admin
StackOverflow strikes again.
Admin
I wonder if the simplified version will correctly handle leap years.
Admin
At least they used .Net's built-in DateTime class which means they don't need to special case the Feb 29 babies/cases. You know they'd have fouled that up if they'd done it stringly or in Java.
As LCrawford says, bonus points for initializing and then not using the
now
var. Which means that if we're iterating around thatwhile
loop on midnight of the birthday we the result is dynamic. Said another way, if they used thenow
var the caller would get the last birthday as of the time of the method call(-ish) but as it's written the result is as of the time of the method return(-ish). At least the result isn't unstable or inverted.Admin
IMO the off-by-one is more a matter of naming. The problem is that
former
¤t
are ambiguous words.If you consider them as meaning
priorIteration
andcurrentIteration
relative to thewhile
loop algorithm there's no error. The algorithm depends on overshooting by one iteration and then returning thepriorIteration
interim result as the final result.If instead you think of them as meaning
priorToMostRecentBirthday
andmostRecentBirthday
then when thewhile
gets to the overshoot condition and terminates, the outcome is thatmostRecentBirthday
is the one in the future and thepriorToMostRecentBirthday
is the one most immediately in the past.Variable naming is hard because there's always an embedded point of view. Are we global-centric and thinking about the birthdays in the flow of time or are we local/algorithm centric and thinking about the flow of the values inside the algorithm?
IME the less skilled the developer the more they're totally focused at the make-my-chosen-algorithm-work level and are not thinking of the global problem in the global context. Which results in near-sighted naming. IME Christian's comment about sort-of- off-by-one is him seeing (like an optical illusion) the alternating interpretation of the variable names but not quite tumbling to the cause of his vague unease that's it's almost right despite passing any testing he might have done.
Admin
I think my ex-boss may have written this code.
He did that all the time. I'm skeptical, though, because he would have set up all the variables and then created a loop that counts to an arbitrary value and come up with this:
Was always a joy working on his code.
Admin
On the topic of naming things, "last" is a terrible word to use regarding an ongoing sequence. I don't know when my last birthday is, because hopefully I've got many more to go. The meaning is obvious in the context here, but I still prefer in general to use more specific names like "latest" or "most recent" to minimize any ambiguity.
Admin
"they don't need to special case the Feb 29 babies/cases"
I'd guess they do need to. If you start with Feb 29th and keep adding 1 year, you'll first get bumped to Mar 1st, then stay in Mar 1st for later years, even for years that have a Feb 29th that should be the birthday. Unless this date addition is more subtle than I realise.
Admin
We used to deal with the off by one errors quite a lot when I worked with pension scheme calculations. Different schemes and different rules were worded slightly differently as to whether something happened when you reached a date/anniversary, or once you were a certain age, or every day accrued after (including/excluding) etc. They were usually a bit vague and had been "clarified" by actuaries and then "amended" following class actions challenging the calculations. And that's before we get into cases where people had switched schemes with a guarantee to be "no worse off" or some correction was being retrospectively applied due to a prior mistake.
Using a loop counting forward from one agreed day zero until "whatever" trigger event while accruing the days in relevant counters was the only way to do it and keep sane. Trying to implement the rule as an algorithm and then add and deduct all the exceptions until you get the right answer is a world of pain, I inherited a mess of code that was attempting to do it that way (and failing), a big name HPC firm was trying to replace me and my code with another single formula approach that was also a failure.
Admin
I suspect that there may be a business rule that says the case year start shall be Mar 1 for all Feb 29 birthdays, whether or not the current year is a leap year. If so the old code is correct and the new code is (likely) not.
Further, I'm not sure what the big deal is about the name of the method. It does what it says it does. Just because the clients of the method use the last birthday in other contexts, doesn't make it a bad name. Suppose they named it calculateCaseYearStart(). Now they add a module concerning vacation accrual and the business rule is that vacation starts accruing on your last birthday. So vacationRestDate = calculateCaseYearStart(). Certainly not better than what we had before.
Admin
This definitely messes up with leap babies but there's another questionable one: If today is your birthday it returns today as your last birthday. I think most people would disagree with this.
I also second the notion that this is recycled birthday logic.
Admin
Honestly the whole article makes me feel a bit dated, but answering the easy reader especially.
Admin
...huh, a number on the first line causes alignment issues.
I guess today's code snippet isn't the only thing with off-by-1 errors.
Admin
And here was me thinking he was only 8 years old. Or 104.
Admin
Odds are that someone who copies bad code from StackOverflow would have written even worse code on their own. As long as we can agree that code quality is a spectrum, not a boolean, then StackOverflow has a positive effect even when bad code is posted.
Admin
This is a peculiar bug.
Admin
1 Frist.
Without a preview button it's hard to tell how much entertainment this bug can provide.
Admin
"If you start with Feb 29th and keep adding 1 year, you'll first get bumped to Mar 1st, then stay in Mar 1st for later years, even for years that have a Feb 29th that should be the birthday"
Oh, that's easy to fix. Just add an incrementing counter (or change the while to a for loop with an arbitrarily large upper limit) and add counter years to the original date to get the new current date. No need to throw this fine code away.
Admin
There is a word that covers all of this "anniversary"
Admin
Some people think code should be self-documenting. But in this case, your function needs to start with a big comment explaining exactly what should be calculated. For example, what does “next birthday” return on your birthday, today or next year? What if your birthday is Feb 29?
I can figure out what code does, but that doesn’t help if I don’t know what it should do.
Admin
But that's not true - if you started on Feb 29, it would first increment to Feb 28, and then remain there even if the current year was a leap year.
Admin
Remy, you're 40?! But you look so young in your picture.