9
\$\begingroup\$

I was tasked with writing a function, that checks whether a date is valid in the following format: DD.MM.YYYY. Here's what I wrote:

bool isValidDate(const char* date)
{
    int a, b, c;
    size_t len = strnlen(date, 255);
    for (size_t i = 0; i < len; i++)
    {
        if (!isdigit(date[i]) && date[i] != '.') return false;
    }
    int validConversions = sscanf_s(date, "%2d.%2d.%4d", &a, &b, &c);
    return validConversions == 3 ? true : false;
}

From my testing I can conclude that this function does reasonably well: it doesn't accept anything except dots and digits, it expects to read exactly 3 numbers and it knows how many digits to read for the days and months.

Something it doesn't check however, is whether the days are less than 31 or the months are less than 12. Another thing it doesn't do is that it doesn't check correct looking, but invalid dates such as 30.02.2013 or 31.06.2012.

Solving that problem is trivial, but I don't wanna write 12 ifs for each month to check the dates. I'm starting to think I should just generate a data structure of valid dates and use that to check, i.e. if the passed date is there, then it's valid.

Is that the best way to do it or are there other ways?

\$\endgroup\$

closed as off-topic by Mast, Toby Speight, Linny, konijn, Dannnno Oct 22 at 13:46

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – Toby Speight, konijn, Dannnno
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ One quick remark is that you should probably use strnlen instead of strlen. If you get a string that does not contain a '\0', your function will have an undefined behavior. \$\endgroup\$ – Luke Skywalker Oct 21 at 9:33
  • \$\begingroup\$ @LukeSkywalker changed to strnlen \$\endgroup\$ – Sergey Teryan Oct 21 at 9:43
  • 1
    \$\begingroup\$ strnlen(date, 0) will always return 0. \$\endgroup\$ – G. Sliepen Oct 21 at 9:51
  • 1
    \$\begingroup\$ @SergeyTeryan You can learn more about strnlen here but to summarize, strnlen takes a maximun length. strnlen(date, 9) means that it will go up to 9 character to find '\0'. So if your date is 6 bytes long for example, it will return 6. If your date is 12 bytes long, it will return 9 anyway. This prevent your code to read memory "indefinitely" if the string passed to your function is corrupted and does not contain a '\0'. \$\endgroup\$ – Luke Skywalker Oct 21 at 11:40
  • 1
    \$\begingroup\$ I'm voting to close this question as off-topic due to code not working since the function will always return false (strnlen will always return zero). \$\endgroup\$ – pacmaninbw Oct 21 at 13:29
9
\$\begingroup\$

Use descriptive variable names

Instead of int a, b, c, give them more descriptive names:

int day, month year;

sscanf() will ignore trailing garbage

The function sscanf() will stop parsing after the last conversion. So the string "1.2.3...." will be cleared by your check for digits and period characters, and then sscanf() will read 3 integers, and returns 3. But obviously, this is an invalid date.

It would be best if you could have sscanf() determine the validity of the whole input string. One way is to use the %n conversion to check how many characters of the string were parsed so far, and then check that this corresponds to the whole string. This is how you can do that:

int day, month, year, chars_parsed;

if (sscanf(date, "%2d.%2d.%4d%n", &day, &month, &year, &chars_parsed) != 3)
    return false;

/* If the whole string is parsed, chars_parsed points to the NUL-byte
   after the last character in the string.
 */
if (date[chars_parsed] != 0)
    return false;

You don't need 12 ifs to check the month

You can just write:

if (month < 1 || month > 12)
    return false;

And similar for days and perhaps even years if you want to limit the allowed range.

Avoid redundant checks

It is likely that your program will normally handle valid date strings. So you want to optimize for this case. In your code, you are checking for the string to be empty at the start, but this is not necessary; if the input string is empty, then sscanf() will not return 3, so it will already correctly return false. Since most input strings will be valid, checking the string length is just a waste of CPU cycles.

Similarly, checking for each character to be a digit or a period is redundant if you just use sscanf() with the %n method to check that there was no trailing garbage after the year.

Better date checking

Just checking whether the month is between 1 and 12 and day between 1 and 31 is not enough. A given month might have less than 31 days. There are also leap years to consider. And if you want to allow dates far in the past, you run into the problem that we have had different calenders. To give an idea of how difficult the problem is, watch: https://www.youtube.com/watch?v=-5wpm-gesOY

One way to validate the date is to use the C library's date and time conversion routines. After scanning the day, month and year, create a struct tm from that, then convert it to seconds since epoch using mktime(). This might still accept invalid dates, but if you convert that back to a struct tm, you can check whether that conversion matched the original input:

int day, month, year,
sscanf(date, "%2d.%2d.%4d", &day, &month, &year);

struct tm input = {
    .tm_mday = day,
    .tm_mon = month - 1,
    .tm_year = year - 1900,
};

time_t t = mktime(&input); /* note, this might modify input */
struct tm *output = localtime(&t); /* prefer localtime_r() on systems that support it */

if (day != output->tm_mday || month != output->tm_mon + 1|| year != output->tm_year + 1900)
    return false;

These routines will probably still not handle dates hundreds of years in the past correctly, but it should suffice for recent dates.

\$\endgroup\$
  • 2
    \$\begingroup\$ How do we handle cases like 31.02.2019 which are syntactically valid? \$\endgroup\$ – Sergey Teryan Oct 21 at 9:59
  • \$\begingroup\$ I thought mktime() would return an error if the date was invalid, but it does not. Perhaps using a roundtrip would work: create a struct tm, convert it to seconds with mktime(), then convert that back to struct tm using localtime_r(), and then checking whether the day/month/year match the results you got from sscanf(). \$\endgroup\$ – G. Sliepen Oct 21 at 10:05
  • 1
    \$\begingroup\$ btw the function still accepts bogus dates like 31.02.2019 \$\endgroup\$ – Sergey Teryan Oct 21 at 11:01
  • 3
    \$\begingroup\$ @pacmaninbw Hm, I don't see any mention of off-topic or Lack of Concrete Context on this page. I only see your comment from half an hour ago about voting to make it off-topic. Am I missing something? Also, I think these comments are getting completely off-topic... \$\endgroup\$ – G. Sliepen Oct 21 at 14:00
  • 1
    \$\begingroup\$ An alternative all-in-one check (for the "syntactical correctness" part) could be if sscanf(date, "%2d.%2d.%4d%c", &day, &month, &year, &dummy_char) != 3 \$\endgroup\$ – Hagen von Eitzen Oct 21 at 19:23

Not the answer you're looking for? Browse other questions tagged or ask your own question.