timeleft utility

Question

This utility tells you how much time is left until the time you set before.

Setting time:

> timeleft set '2017/04/25;15:00:00'

Using utility:

> timeleft
2 days, 10 hours, 40 minutes, 25 seconds remaining...

Code:

#include <string.h> // strcmp
#include <stdio.h> // fopen, fclose, fprintf, fscanf, sscanf
#include <time.h> // time, mktime
#define TIME_PATH "time.txt"
static void printRemainingTime(time_t t) {
 const char *s = "%d days, %d hours, %d minutes, %d seconds remaining...\n";
 int days = t / 86400; t -= days * 86400;
 int hours = t / 3600; t -= hours * 3600;
 int minutes = t / 60; t -= minutes * 60;
 int seconds = t;
 fprintf(stdout, s, days, hours, minutes, seconds); 
}
int main(int argc, char **argv) {
 int status = 0;
 if (argc <= 1) {
 FILE *f = fopen(TIME_PATH, "r");
 if (!f) {
 fprintf(stderr, "Could not open time\n");
 status = 1;
 } else {
 long t;
 if (fscanf(f, "%ld", &t) != 1) {
 fprintf(stderr, "Could not read time\n");
 status = 1;
 } else {
 time_t ref = (time_t) t;
 time_t now = time(NULL);
 if (now >= ref) fprintf(stdout, "It's over!\n");
 else printRemainingTime(ref - now);
 }
 fclose(f);
 }
 } else if (argc == 3 && strcmp(argv[1], "set") == 0) {
 int Y, M, D, h, m, s;
 if (sscanf(argv[2], "%d/%d/%d;%d:%d:%d", &Y,&M,&D,&h,&m,&s) != 6) {
 fprintf(stderr, "Invalid time format\n");
 } else if (Y >= 2038) {
 fprintf(stderr, "Do not plan too far ahead!\n");
 } else {
 struct tm t = { s, m, h, D, M - 1, Y - 1900, -1, -1, -1 };
 time_t ut = mktime(&t);
 if (ut < 0) {
 fprintf(stderr, "Parsing/range error\n");
 status = 1;
 } else {
 FILE *f = fopen(TIME_PATH, "w");
 if (!f) {
 fprintf(stderr, "Failed to save time\n");
 status = 1;
 } else {
 fprintf(f, "%ld", (long) ut);
 fclose(f);
 }
 }
 }
 } else {
 fprintf(stderr, "Usage: ");
 if (argv[0]) fprintf(stderr, "%s ", argv[0]);
 fprintf(stderr, "[set YYYY/MM/DD;hh:mm:ss]\n");
 }
 return status;
}

• chux: struct tm t = { s, m, h, D, M - 1, Y - 1900, -1, -1, -1 }; assumes a struct tm member order - which is not specified.

Answer 1

1. It doesn't compile without warnings under clang -Wextra:

   $ clang -Wall -Wextra cr161559.c
   cr161559.c:43:69: warning: missing field 'tm_gmtoff' initializer
    [-Wmissing-field-initializers]
    struct tm t = { s, m, h, D, M - 1, Y - 1900, -1, -1, -1 };
   

2. Expecting a semi-colon in the command-line arguments is inconvenient: it means that you always have to quote the argument when running the program from the shell:

   $ timeleft set 2017年04月25日;15:00:00
   Invalid time format
   bash: 15:00:00: command not found
   

   It would be better to use some other input format, one that doesn't use shell meta-characters.

3. [Recommendation deleted following threat of violence in comments.]

4. Separating the declaration of s from the call to fprintf means that you won't get a warning (at least not from current versions of Clang or GCC) if the format string doesn't match the arguments. For example, if you accidentally omitted one of the arguments:

   const char *s = "%d days, %d hours, %d minutes, %d seconds remaining...\n";
   /* ... */
   fprintf(stdout, s, days, hours, minutes);
   

   then the compilation succeeds without warning under both Clang and GCC, but the output is nonsense:

   $ timeleft
   1 days, 22 hours, 33 minutes, 142630783 seconds remaining...
   

   But if you had written instead:

   printf("%d days, %d hours, %d minutes, %d seconds remaining...\n",
    days, hours, minutes); 
   

   then Clang detects the problem:

   cr161559.c:12:45: warning: more '%' conversions than data arguments [-Wformat]
    printf("%d days, %d hours, %d minutes, %d seconds remaining...\n",
    ~^
   

• IMSoP: Beware, you may be bludgeoned to death with a giant 'S' for point 3 ;) meta.stackexchange.com/a/63791/237313

Answer 2

Just a few suggestions:

Standard Macros

The file stdlib.h contains two macros that are very good to use when terminating the program, EXIT_SUCCESS and EXIT_FAILURE. The might make main() a little more readable.

#include <stdlib.h>
int main(int argc, char **argv) {
 int status = EXIT_SUCCESS;
 if (argc <= 1) {
 FILE *f = fopen(TIME_PATH, "r");
 if (!f) {
 fprintf(stderr, "Could not open time\n");
 status = EXIT_FAILURE;
 } else {
 long t;
 if (fscanf(f, "%ld", &t) != 1) {
 fprintf(stderr, "Could not read time\n");
 status = EXIT_FAILURE;
 } else {
 time_t ref = (time_t) t;
 time_t now = time(NULL);
 if (now >= ref) fprintf(stdout, "It's over!\n");
 else printRemainingTime(ref - now);
 }
 fclose(f);
 }

Put Each Statement on its Own Line

This code might be more readable if each statement was on its own line:

static void printRemainingTime(time_t t) {
 const char *s = "%d days, %d hours, %d minutes, %d seconds remaining...\n";
 int days = t / 86400; t -= days * 86400;
 int hours = t / 3600; t -= hours * 3600;
 int minutes = t / 60; t -= minutes * 60;
 int seconds = t;
 fprintf(stdout, s, days, hours, minutes, seconds); 
}

Example:

static void printRemainingTime(time_t t) {
 const char *s = "%d days, %d hours, %d minutes, %d seconds remaining...\n";
 int days = t / 86400;
 t -= days * 86400;
 int hours = t / 3600;
 t -= hours * 3600;
 int minutes = t / 60;
 t -= minutes * 60;
 int seconds = t;
 fprintf(stdout, s, days, hours, minutes, seconds); 
}

Compiler Warnings

The compiler I am using is flagging the following lines for implicit type conversion from long to int:

 int days = t / 86400;
 int hours = t / 3600;
 int minutes = t / 60;
 int seconds = t;

Perhaps it would be better to declare days, hours, minutes and seconds as long to match the same type as t can mutate to.

Inconsistent Variable Naming Conventions

In the function printRemainingTime() the code has the variables t, s, days, hours, minutes, seconds. This is inconsistent. There are some good variable names such as days, hours, minutes and seconds and then there are the variable names t (?time?) and s (?string?). Neither of these variable names are very descriptive. If s is for string, it might be better called formatString and if t is for time perhaps inputTime might be a better name.

Within the main() function there is the variable declaration int Y, M, D, h, m, s;. Perhaps more descriptive names would be year, month, day, hour, minute, second.

Reduce Complexity, Follow SRP

The Single Responsibility Principle states that every module or class should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility.

Robert C. Martin expresses the principle as follows:

A class should have only one reason to change.

While this is primarily targeted at classes in object oriented languages it applies to functions and subroutines in procedural languages like C as well.

The main function could be broken up into at least 3 functions:

int getInputFromStdin(char outputStringReadFromStdi[]) // return EXIT_SUCCESS or EXIT_FAILURE
int getInputFromFile(char inputFileName[], char outputStringReadFromFile[]) // return success 
void usage(char programName[]); // prints the error message in the final else clause.

The Error Message Isn't Quite Correct

These two error messages don't use the complete filename of time.txt:

 fprintf(stderr, "Could not open time\n");

and

 fprintf(stderr, "Could not read time\n");

The user could be mislead to add the file time, rather than the file time.txt

Answer 3

• You don't really need fprintf() for printing to stdout; printf() does this by default and saves you an argument. You do still need fprintf for stderr, though.

• I think you are missing status = 1; for some of the stderrs in main().

• If t is time-related, then you should declare it as a time_t. You can then cast it to a long in fscanf instead.

• This isn't consistent with your other condition formatting:

  if (now >= ref) fprintf(stdout, "It's over!\n");
  else printRemainingTime(ref - now);
  

  It should be:

  if (now >= ref) {
   fprintf(stdout, "It's over!\n")
  } else {
   printRemainingTime(ref - now);
  }
  

  There's also this:

  if (argv[0]) fprintf(stderr, "%s ", argv[0]);
  

  which should be:

  if (argv[0]) {
   fprintf(stderr, "%s ", argv[0]);
  }
  

• Vorac: One possible advantage of commenting which functions are used form an include is the following. When changing code, it becomes easier to remove unneeded includes.
• chux: "If t is time-related, then you should declare it as a time_t. You can then cast it to a long in fscanf instead." --> fscanf() would need a long * cast not a long cast. As time_t is not specified to be long that casting is problematic. time_t is not even specified to be an integer. Portable code cannot *scanf() into time_t directly. OP's approach has its short-comings like range, yet "cast it to a long in fscanf instead" is less portable.

Answer 4

Since this has not been mentioned yet, this part is problematic:

else if (Y >= 2038) {
 fprintf(stderr, "Do not plan too far ahead!\n");
}

Firstly, hard-coding the date requires you to change your program in the future.

Secondly, why the limit at all? If it's a technical reason, you should tell the user. If not, it just limits the usefulness of the tool.

• user63132: It's because of the year 2038 problem. time_t could be either 32-bit or 64-bit integer, so in case it's 32-bit I'm limiting the range to year 2038. But I guess you're right and mktime would handle it internally just fine.
• user1118321: At the very least, don't use a magic number like 2038. Use a named constant that happens to equal 2038. Something like const int kMaxYear = 2038;.
• Roland Illig: The name kMaxYear has no value since it doesn't explain what is going on here. A comment linking to the Wikipedia article would be more helpful.
• Taemyr: @user1118321 UnixEpochEndYear is a better name.
• user1118321: @RolandIllig That's a fair point, but it's still better than a magic number with no context. At least with kMaxYear you have some idea that there's some limit, even if you don't know why. I agree with Taemyr that UnixEpochEndYear is better. It might be even better to have it calculated in code along the lines of calculating the time corresponding to 0xFFFFFFFF and then pulling the year out of it.

Answer 5

1. Let mktime() validate range. The 2038 test is not needed as mktime() will report an error if the year is too great.

   //} else if (Y >= 2038) {
   // fprintf(stderr, "Do not plan too far ahead!\n");
   //} else {
    struct tm t = { s, m, h, D, M - 1, Y - 1900, -1, -1, -1 };
    time_t ut = mktime(&t);
    if (ut < 0) {
   

2. Code assumes time_t is measured in seconds with printRemainingTime(time_t t). time_t is a scalar. To portably convert to a seconds difference, use difftime().

   double difftime(time_t time1, time_t time0);
   

   The difftime function computes the difference between two calendar times: time1 - time0. C11 §7.27.2.2 3

3. Other code assumes time_t is a seconds count bounded by long range like long t; ... time_t ref = (time_t) t; and fprintf(f, "%ld", (long) ut);. At a minimum, code could use long long or intmax_t to cope with systems that use a wider than long time representation. Highly portable code would use double, yet extra care is need to handle conversion issues. Consider that the platform that writes the file may not be the platform the reads the file.

4. Code assumes the order of struct tm, which is not specified by C. Could use below or How to initialize a struct in accordance with C programming language standards

   // struct tm t = { s, m, h, D, M - 1, Y - 1900, -1, -1, -1 };
   struct tm t = {0 };
   t.tm_year = Y - 1900;
   ...
   t.tm_sec = s;
   t.tm_isdst = -1;
   

5. "%d/%d/%d;%d:%d:%d" with its ; in the middle is a curious separator choice. The standard YMDhms format is more like "2017-04-27T12:44:32" yet that is not universal well known. Code that could handle a variety of date formats (and with corresponding error detection) would be desirable.

    if (sscanf(argv[2], "%d/%d/%d;%d:%d:%d", &Y,&M,&D,&h,&m,&s) != 6) Good();
    else if (sscanf(argv[2], "%d-%d-%dT%d:%d:%d", &Y,&M,&D,&h,&m,&s) != 6) Good();
    else if (sscanf(argv[2], Other formats, ...) != 6) Good();
    else Bad();
   

6. sscanf(argv[2], "%d/%d/%d;%d:%d:%d", &Y,&M,&D,&h,&m,&s) != 6 allows input with trailing text like "2017/04/27;12:34:56xyz". A simple solution is to use %n

    int n
    n = 0; 
    sscanf(argv[2], "%d/%d/%d;%d:%d:%d %n", &Y,&M,&D,&h,&m,&s,&n);
    if (n > 0 && argv[2][n] == '0円') Good();
   

7. Nit: if (ut < 0) { is not the specified error condition. Highly portable code would use a test against (time_t)(-1).

   If the calendar time cannot be represented, the function returns the value (time_t)(-1). C11 §7.27.2.3 3.

8. Nit: int days = t / 86400; can overflow 16-bit int, maybe in the year 3535