Clock in C language

Question

Ok, so here's the code: Please, comment if there are mistakes

#include <stdio.h>
#include <windows.h>

main()
{
    int h, m, s;
    int d = 1000;


    printf("Set the Clock time: \n");
    scanf("%d%d%d", &h,&m,&s);


    if (h > 24 || m > 60 || s > 60) {
        printf("ERROR!");
        exit(0);
    }
    while(1) {

        s++;
        if (s > 59) {
            m++;
            s = 0;
        }
        if (m > 59) {
            h++;
            m = 0;
        }
        if (h > 24) {
            h = 1;
        }
        printf("\n Clock: ");
        printf("%02d:%02d:%02d", h, m, s);
        Sleep(d);
        system("cls");
    }
}

So, honestly, I don't know whether this code is good and readable :( That's why I posted it here.

Answer 1

24 hours is OK?

Instead of

if (h > 24 || m > 60 || s > 60),

I'd expect

if (h >= 24 || m >= 60 || s >= 60)

... although allowing 24:00:00 as a special case is OK.

Since h,m,s are int, range checking of negative values is warranted.

if (h >= 24 || h < 0 || m >= 60 || m < 0 ....

Perhaps you want to allow leap seconds like 23:59:60 for universal time?

---

Use modern C

// main()

int main()
// or 
int main(void)

---

Validate user input

// scanf("%d%d%d", &h,&m,&s);
if (scanf("%d%d%d", &h,&m,&s) != 3) {
    printf("ERROR, not 3 integers\n");
    exit(0);
}

---

Error output best on stderr with a '\n'

    //printf("ERROR!");
    //exit(0);
    fprintf(stderr, "ERROR!\n");
    return EXIT_FAILURE;

---

Consistency with numbers

Rather than 60 and 59, how about 60 as that is common knowledge as in 60 seconds/minute, etc.

if (h >= 24 || m >= 60 || s >= 60) {
    ...
}
while(1) {
    s++;
    // if (m > 59) {
    if (s >= 60) {

Avoid naked magic numbers

#define SEC_PER_MIN 60

if (... s >= SEC_PER_MIN) {
    ...
}
while(1) {
    s++;
    if (s >= SEC_PER_MIN) {

---

code is ... readable (?)

• I find the double line spacing excessive.

• Big lesson here. Do not manually format your code. Life is short. Use an IDE with an auto-formatter.

---

Flush when done

printf("%02d:%02d:%02d", h, m, s); does not certainly output as stdout is often line buffered.

Either

printf("%02d:%02d:%02d", h, m, s);
fflush(stdout);
// or
printf("%02d:%02d:%02d\n", h, m, s);

Answer 2

Incorrect input validation

Your code allows someone to enter the time "24:60:60", and your program will accept it. I also see that the clock is meant to display times from 01:00:00 to 24:59:59, but it will also accept 0 as the hour. To ensure you only accept inputs for times that you would display, write:

if (h < 1 || h > 24 || m >= 60 || s >= 60)
    ...

Although it's rather more common that a 24-hour clock goes from 00:00:00 to 23:59:59.

Proper error reporting

When you encounter an error, the proper thing to do is to print an error message to stderr, and then exit the program with a non-zero exit code, preferrably EXIT_FAILURE.

Sleep(1000) does not sleep exactly 1 second

The Sleep() function might not sleep the exact amount of time you specify, and even if it did, printing the time and clearing the screen also takes some time. So most likely, your clock would run a little slow. To make it work correctly, you want to check what the actual time is, and right before calling Sleep(), determine how much you need to sleep until a second has elapsed. The most portable way to get some idea of the current time is to use the clock() function. Your loop should look like this:

clock_t next_time = clock();

while (1) {
    /* Print clock */
    ...

    clock_t current_time = clock();
    next_time += CLOCKS_PER_SEC;
    clock_t difference = next_time - current_time;
    Sleep(difference * 1000 / CLOCKS_PER_SEC);
}

Note that while clock() might work in practice, it's also not guaranteed to run as fast as the "wall time". Platform-dependent functions that you could use are GetSystemTime() for Windows, and gettimeofday() or clock_gettime() for Linux and macOS. The principle is the same though.

Avoid calling system() unnecessarily

system() will cause a new shell process to be created, and then that shell will parse and execute the command you give it. This is quite inefficient for something simple as clearing the screen, and is also not platform independent (on most other operating systems, you would have to use "clear" instead of "cls" as the command).

There are more efficient ways to clear the screen, for example using ANSI escape codes, but there is an even simpler alternative: never go to the next line, just overwrite the current line. You can do this by using \r in the format string, which will move the cursor back to the start of the line. I suggest just writing:

printf("\rClock: %02d:%02d:%02d", h, m, s);
fflush(stdout);

The fflush() command is necessary to ensure the line is written to the screen immediately, as normally output to stdout is line-buffered.

Making it more platform independent

Unfortunately, there is no standard C function to sleep for a given amount of time. On Windows, you have to use Sleep(), on most other operating systems you have to use the POSIX nanosleep() function. See this StackOverflow post for some suggestions to make your program compile for multiple operating systems.