9
\$\begingroup\$

Here is some code of mine that prints out the relative date according to the current time:

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
int main (int argc, char *argv[])
{
  int i = 1;
  struct tm date = {0};
  char relativeDays[80];

  char* temp = strtok(argv[1], "/");
  while(temp != 0)
  {
    switch(i++)
    {
                case 1:
                        date.tm_mon  = atoi(temp) - 1;
                        break;
                case 2:
                        date.tm_mday = atoi(temp);
                        break;
                case 3:
                        date.tm_year = atoi(temp) - 1900;
    }
    temp=strtok(NULL, "/");
  }
  i = (int) difftime(time(NULL), mktime(&date))/86400;
  sprintf(relativeDays, "%d", abs(i));
  if (i > 0) printf("%s\n", strcat(relativeDays, " days ago."));
  else if (i < 0) printf("%s\n", strcat(relativeDays, " days from now."));
  else printf("Today\n");
  return 0;
}

Sample input and output:

$ ./date 12/8/2013
2 days ago.
$ ./date 12/10/2013
Today
$ ./date 12/24/2013
13 days from now.

Any thoughts on how to improve the code, specifically how to make it shorter?

\$\endgroup\$
5
\$\begingroup\$

Date-time handling is tricky to implement correctly. Unless you want to reinvent the wheel, use strptime(3) to parse dates. Your code will be simpler, and you should automatically get validation to reject dates like "12/32/2013".

When converting the input date to a Unix epoch value using mktime(&date), you always interpret the date as midnight in the UTC time zone. That leads to two bugs in calculating the date difference, in my opinion:

  1. The user would expect the input date to be interpreted in the local time zone.
  2. When specifying tomorrow's date (and interpreting it in the local time zone), the output will be "Today", since 00:00:00 of the following day is less than 86400 seconds in the future.

The output routines could also use some slight improvement. You need neither abs() nor strcat(). Also, it would be human-friendly to handle singular numbers.

The magic number 86400 should be explained better.

#include <stdio.h>
#include <time.h>

#define SECONDS_PER_DAY (24 * 60 * 60L)

int main(int argc, char *argv[]) {
    char *input_str;
    time_t now;
    struct tm input_tm = { 0 };
    long diff_days;

    if (argc <= 1) {
        fprintf(stderr, "Need MM/DD/YYYY input\n");
        return 1;
    }
    input_str = argv[1];

    /* When calling difftime(), both arguments should have the same
       hr:min:sec, else you might get off-by-one-day errors depending on
       the time of day when you run the program.  We could either use
       00:00:00 midnight for both, or the current hr:min:sec for both.
       Let's use the current time for both. */
    time(&now);
    localtime_r(&now, &input_tm);

    /* strptime() will fill in MM/DD/YYYY, but leave the hr:min:sec and
       time zone fields from above alone. */
    if (!strptime(input_str, "%m/%d/%Y", &input_tm)) {
        fprintf(stderr, "Bad date: %s\n", input_str);
        return 1;
    }
    diff_days = (long)difftime(now, mktime(&input_tm)) / SECONDS_PER_DAY;

    if (diff_days > 0) {
        printf("%ld day%s ago.\n", diff_days, (diff_days > 1 ? "s" : ""));
    } else if (diff_days < 0) {
        printf("%ld day%s from now.\n", -diff_days, (diff_days < -1 ? "s" : ""));
    } else {
        printf("Today\n");
    }
    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Note: With 16-bit int, 86400 is recognized as needing a larger type, such as long. Thus the division occurs with (long) 86400. With the more understandably (24 * 60 * 60), would such a compilation also come up with the same product or (int) (24 * 60 * 60) which is 20864? \$\endgroup\$ – chux - Reinstate Monica Dec 11 '13 at 15:47
  • \$\begingroup\$ @chux Good observation. I've widened the data types accordingly. \$\endgroup\$ – 200_success Dec 11 '13 at 18:49
5
\$\begingroup\$

I've commented your code:

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

int main(int argc, char *argv[])
{
    int i = 1;
    struct tm date = {0};
    char relativeDays[80];

Check For Proper Input

    if(argc != 2)
    {
        printf("Two arguments required!\n");
        return -1;
    }

    char *temp = strtok(argv[1], "/");

temp will be NULL if empty. Also, add a break after case 3: for consistency. If you think you'll need to handle unexpected cases, use default at the end.

    while(temp != NULL)
    {
        switch(i++)
        {
            case 1:
                date.tm_mon  = atoi(temp) - 1;
                break;
            case 2:
                date.tm_mday = atoi(temp);
                break;
            case 3:
                date.tm_year = atoi(temp) - 1900;
                break;
            default:
        }

Be consistent with spacing.

        temp = strtok(NULL, "/");
    }

Add comments so people know what your intentions are. Why are you dividing by 86400?

    i = (int)difftime(time(NULL), mktime(&date)) / 86400;
    sprintf(relativeDays, "%d", abs(i));

Use spaces to make everything more readable.

    if(i > 0)
        printf("%s\n", strcat(relativeDays, " days ago."));
    else if(i < 0)
        printf("%s\n", strcat(relativeDays, " days from now."));
    else
        printf("Today\n");

    return 0;
}

Edit:

This while block can be changed to the following. Though it doesn't change the operation, it may improve the aesthetics.

while(temp != NULL)
{
    int num = atoi(temp);    
    switch(i++)
    {
        case 1:
            date.tm_mon = num - 1;
            break;
        case 2:
            date.tm_mday = num;
            break;
        case 3:
            date.tm_year = num - 1900;
            break;
        default:
            printf("Unexpected input. Blowing up now!\n");
            break;
    }

    temp = strtok(NULL, "/");
}
\$\endgroup\$
  • \$\begingroup\$ NULL is #defined as 0, so that shouldn't be the case. Perhaps a bug has been exposed. \$\endgroup\$ – Fiddling Bits Dec 11 '13 at 2:24
  • \$\begingroup\$ You are right, it works. I thought putting it in as !temp would give the same result, but it didn't. Good advice overall (+1), but I was really looking for a way to shorten the code. \$\endgroup\$ – syb0rg Dec 11 '13 at 2:26
  • 1
    \$\begingroup\$ Hi, I've upvoted your answer - however be careful with comment-in-code answers, they might eventually get flagged as code-only posts - which are frowned upon. Take the time to format your answer, make it look good, lay down your thoughts in plain text - and of course include improved code blocks if you want to :) \$\endgroup\$ – Mathieu Guindon Dec 11 '13 at 2:31
  • \$\begingroup\$ Probably can't be shortened and still be readable. Short code doesn't necessarily mean good code. \$\endgroup\$ – Fiddling Bits Dec 11 '13 at 2:31
  • 4
    \$\begingroup\$ @BitFiddlingCodeMonkey: anyone working on this code will probably know, or easily deduce, that 86400 is the number of seconds in a day (60x60x24). Not sure it needs a comment. Making it a named constant on the other hand... \$\endgroup\$ – jmoreno Dec 11 '13 at 5:38
4
\$\begingroup\$

Concerns if argv[1] represents a local time

  1. Should add date.tm_isdst = -1;. Although the h:m:s are set to 0, can't think of a dst change that crossed a day, so this is pedantic.

Other concerns.

  1. Should check range of year, easy for folks to enter only last 2 digits and expect 13 to be 2013.
\$\endgroup\$
  • \$\begingroup\$ @syb0rg Alos: In effect, you are comparing the days elapsed since zero-hour of the supplied date (the first moment of that day) to "now". Then using (int) difftime(), you are rounding toward 0 that result. The asymmetry of using "first moment" and "round toward 0" may be of concern. \$\endgroup\$ – chux - Reinstate Monica Dec 11 '13 at 15:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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