Function to calculate the ordinal day

Question

I just handed in my first assessed coursework for my Computer Science degree. Part of it was to take in a date (dd mm yyyy) between 1900 and 2100 and output the ordinal date. My function to do that is below.

I’m new to C - I’ve used Python, JavaScript and more recently Pascal, so I might be approaching this from the wrong direction, and there could be a more traditional method I should be using.

Any suggestions to make this more standard with C guidelines would be great - in addition to other algorithms I could have used.

int monthToDays(int day, int month, int year) {
  int monthLengths[11] = {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30}; // only 11 long because january -> 0 not 31

  monthLengths[1] = monthLengths[1] + checkIsLeap(year); //increase febuaray by 1 if it's a leap year

  int iMonth = 0;
  int totalDays = day;
                          // reduce by 1 because array is 0 indexed
  for (iMonth = 0; iMonth < month - 1; iMonth++) {
    totalDays = totalDays + monthLengths[iMonth];
  }

  return totalDays;
}

int checkIsLeap(int year) {

  int leap = 0;

  if (year % 4 == 0) {
    leap = 1;
    if (year % 100 == 0) {
      leap = 0;
      if (year % 400 == 0) {
        leap = 1;
      }
    }
  }

  return leap;
}

Answer 1

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 assertments: 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.

Answer 2

Rather than supplying year, you might have the caller pass in the result of checkIsLeap(). Six one way, half dozen the other.

It is important to pair this code with some unit test code which exercises it.

You might report errors if the caller passes invalid input values.

Otherwise, it looks good, ship it.