Remarks about your code
I, too, think that you should change the signature and not pass the year into that function, since basically it doesn't need to know about the year. What it needs to know is whether it's a leap year or not (thus a bool
from stdbool.h
). So pass that in. (A bit like Tell, don't ask
in OOP)
Modifying the months length of February seems like the most "real world like" approach, but I wouldn't do it, because then you can make the months lengths const
(and also static
). Do a correction (+ 1
) for leap years after your calculation, then.
Going further, are you really interested in the lengths of a month? You could as well just store the number of days (in a non leap year) that precede a month (January -> 0
, February -> 31
, March -> 31 + 28
, ...). Then you can get rid of that loop all together.
Lack of assert
ments: When you make an assumption about the value of an function argument (1 <= months <= 12
), then assert
for that (even if the specification of your function does not include argument validation!) such that if an invalid value gets passed, you fail fast and loud. (Ooops .. an advise I should have implemented myself in the code below ...)
naming. monthToDays
... sounds like "convert a month into days" ... I would expect this to accept a month (number) and get the number of days in that month. ordinal_date
? Or a bit more detailed ordinal_date_day
?
Test, test, test!
Next, I'd like to introduce you to TDD (test driven development): Instead of first writing your code and then test it (as all - or almost all - code should be tested!) you start off writing tests, specifying the desired behavior, and then make these tests pass by actually implementing the logic to achieve that behavior.
Normally, you'd look into some testing framework / library, but we can go with a simple macro here for now:
#define check(expr) \
if(!(expr)) { \
printf("[FAILED] " #expr " in %s\n", __func__); \
}
Note that this is the bare minimum of information we need out of a test case: An indication when it fails and which test case failed. Testing frameworks can give you far better information, for example a comparison of the expected value of a function call and it's actual value.
Start with a simple case:
void ordinal_date_in_january_should_be_day_number() {
check(ordinal_date( 1, 1, false) == 1);
check(ordinal_date( 1, 1, true) == 1);
check(ordinal_date(11, 1, false) == 11);
check(ordinal_date(11, 1, true) == 11);
}
That's simple to implement:
int ordinal_date(int day, int month, bool leap_year) {
return day;
}
Going to a slightly more complex case, days in February:
void ordinal_date_in_february_should_be_day_number_plus_days_in_january() {
check(ordinal_date( 1, 2, false) == 32);
check(ordinal_date( 1, 2, true) == 32);
check(ordinal_date(11, 2, false) == 42);
check(ordinal_date(11, 2, true) == 42);
}
Well, KISS:
int ordinal_date(int day, int month, bool leap_year) {
if (month == 1) {
return day;
}
if (month == 2) {
return day + 31;
}
return -1; // Fall-through, always code defensively
}
Fine, tests pass again. Time for new tests. Check against known values:
void ordinal_date_should_match_known_values_for_non_leap_year_march_to_dezember() {
check(ordinal_date( 1, 3, false) == ( 59 + 1));
check(ordinal_date( 1, 4, false) == ( 90 + 1));
check(ordinal_date( 1, 5, false) == (120 + 1));
check(ordinal_date( 1, 6, false) == (151 + 1));
check(ordinal_date( 1, 7, false) == (181 + 1));
check(ordinal_date( 1, 8, false) == (212 + 1));
check(ordinal_date( 1, 9, false) == (243 + 1));
check(ordinal_date( 1, 10, false) == (273 + 1));
check(ordinal_date( 1, 11, false) == (304 + 1));
check(ordinal_date( 1, 12, false) == (334 + 1));
}
Ok, we can go on with these if
.. but this starts to look like a lookup table to me, so better implement that instead:
int ordinal_date(int day, int month, bool leap_year) {
static const int days_up_to_month[] = {
0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334};
return days_up_to_month[month - 1] + day;
}
Note that we can make this change with confidence of not breaking existing requirements due to the previous tests we wrote.
Finally, let's include the known values for a leap year:
void ordinal_date_should_match_known_values_for_leap_year_march_to_dezember() {
check(ordinal_date( 1, 3, true) == ( 60 + 1));
check(ordinal_date( 1, 4, true) == ( 91 + 1));
check(ordinal_date( 1, 5, true) == (121 + 1));
check(ordinal_date( 1, 6, true) == (152 + 1));
check(ordinal_date( 1, 7, true) == (182 + 1));
check(ordinal_date( 1, 8, true) == (213 + 1));
check(ordinal_date( 1, 9, true) == (244 + 1));
check(ordinal_date( 1, 10, true) == (274 + 1));
check(ordinal_date( 1, 11, true) == (305 + 1));
check(ordinal_date( 1, 12, true) == (335 + 1));
}
Of course this fails spectacularly, but is easily fixed with some "domain knowledge" (namely that in a leap year February is a day longer):
int ordinal_date(int day, int month, bool leap_year) {
static const int days_up_to_month[] = {
0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334};
return days_up_to_month[month - 1] + day
+ ((leap_year && (month > 2)) ? 1 : 0);
}
Note that our tests would have revealed this domain knowledge, too. Try the code without the month > 2
condition and see which tests fail.
Finally: Tests pass. Fine. I hope this is of potential use for you.
(Ideone link with above code)
- That
2
is a naked magic number, which is not good. Better define a constant FEB
or something like that.
- You wrote that the code is tested with 65000 random values. Sorry, but that's useless. For this example, it will - of course (with very high probability) - spot any errors in the implementation as there are only
365 + 366
different inputs that the function might take. But what it won't help with is clearly documenting the desired behavior. Moreover, running your test will take a lot more time than running the simpler tests here. That's not an issue for just one function, but within a mid size to large size project these run times will build up quickly. And the longer the test, the less often you will run it! The less often you run it, the more code you write without testing it.
checkIsLeap
for completeness? \$\endgroup\$ – Phrancis Oct 28 '17 at 1:58{31, 59, 89, ...}
, and changing your leap year test slightly. \$\endgroup\$ – user1149 Oct 29 '17 at 17:03if (checkIsLeap(year) && month > 2) {totalDays++}
should do it. Extreme nitpick: if your date is guaranteed to be between 1901 and 2099 (you said 1900 and 2100), the leap year test is simpler (year%4
no exceptions). However, your code is more general which is good. \$\endgroup\$ – user1149 Oct 29 '17 at 17:09