6
\$\begingroup\$

This code works exactly as the prompt and the code predict. Is my style good, my implementations, or what should I change, or what? I'm trying to improve my code, and writing more of it helps... Any and all relevant feedback is good.

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

#define SECONDS_PER_MINUTE 60
#define MINUTES_PER_HOUR 60
#define HOURS_PER_DAY 24
#define PROMPT_STRING "Type any key to show time, q to show time then quit: "

#define TIME_TYPE unsigned long 

char getChar() ;
void updateTimes() ;
void initTimes() ;
void printPrompt() ;
void printTime (TIME_TYPE timeInit) ;

TIME_TYPE start, end;

int main (const char argc, char* argv[]) {
        char command = 0;
        initTimes();
        while (command != 'q') {
                printPrompt();
                command = getChar();
                updateTimes();
                printTime(end-start);
        }
        return 0;
}

char getChar() {
        char retVal = -1;
        char trashVal = -1;
        retVal = getc(stdin);
        while ( trashVal != '\n' && retVal != '\n') {
                trashVal = getc(stdin);
        }
        return retVal;
}

void updateTimes() {
        end = time(0);
}

void initTimes() {
        start = time(0);
        end = start;
}

void printPrompt() {
        printf(PROMPT_STRING);
}

void printTime (TIME_TYPE timeInit) {
        /* Basic declarations */
        TIME_TYPE time = timeInit;
        int days, hours, minutes, seconds;
        int timeDifferential = 0;
        /* Assigning crap, reassigning remaining seconds to 'time' */
        /* Days */
        timeDifferential = (SECONDS_PER_MINUTE *
                                MINUTES_PER_HOUR *
                                HOURS_PER_DAY);
        days = (time/timeDifferential);
        time -= timeDifferential*days;
        /* Hours */
        timeDifferential = (SECONDS_PER_MINUTE *
                                MINUTES_PER_HOUR);
        hours = (time/timeDifferential);
        time -= timeDifferential*hours;
        /* Minutes */
        timeDifferential = (SECONDS_PER_MINUTE);
        minutes = (time/timeDifferential);
        time -= timeDifferential*minutes;
        /* Seconds */
        seconds = time;
        /* Check for validity of times, then print.
         * Also print error if need be.*/
        if (!(seconds < 60 && seconds >= 0 &&
                        minutes < 60 && minutes >= 0 &&
                        hours < 24 && hours >= 0 )) {
                printf ("ERROR! TIME FORMAT SCREWY!\n");
        }
        printf ("Total time (s): %u\n %2dd %2dh %2dm %2ds\n", timeInit, 
                        days, hours, minutes, seconds);
}
\$\endgroup\$
11
\$\begingroup\$

Compiler Warnings

Firstly, always enable maximum warning settings for your compiler, it will catch things that you miss. For example, with gcc -Wall -Wextra, the following is produced:

timer.c:20:5: warning: first argument of 'main' should be 'int' [-Wmain]

timer.c: In function 'main': timer.c:20:22: warning: unused parameter 'argc' [-Wunused-parameter] timer.c:20:34: warning: unused parameter 'argv' [-Wunused-parameter]

timer.c: In function 'printTime': timer.c:92:21: warning: format '%u' expects argument of type 'unsigned int', but argument 2 has type 'long unsigned int' [-Wformat]

Nothing major, which is good, but we can fix some of these up. Firstly, as it says, main should have signature:

int main (int argc, char* argv[]) 

not the current signature of

int main(const char argc, char* argv[])

The next warnings complain about unused parameters of argc and argv. If you aren't going to utilize command line arguments in your program, you can simply leave them out:

int main()

Fixing the printf warning is a simple matter of replacing the %u\n with %lu\n.

Code Style

Generally, there is too much whitespace. Very few programmers use 8 space indentation - I'd recommend sticking to 4. Also, leaving a space between method declarations and the trailing semicolon is slightly odd - char getChar() ;. None of this is wrong, it's just unorthodox, and will probably be a bit jarring to most C programmers (and programmers are a picky bunch). Pretty much the first thing I did with your code is modify the indentation and remove any whitespace for trailing semicolons.

Never, ever #define a type. Utilize typedef instead. This also means types should not be ALL_CAPS.

typedef unsigned long time_type;

There are very good reasons for this. For example, if we define a pointer type:

#define ptr_int int *

Now, what happens if we do the following:

ptr_int x, y;

The preprocessor will expand this to:

int *x, y; //Uh-oh!

This is declaring a pointer to integer (x) and a normal integer (y). This is sure to introduce annoying and hard to track down bugs. If instead we used a typedef, this will fix the above problem (although it shows you one must be careful when having more than declaration per line).

Comments are sparse (then again, this program probably doesn't really need them). However, you've fallen into the "obvious comment" trap:

/* Basic declarations */
/* Assigning crap, reassigning remaining seconds to 'time' */
/* Days */

and so on. None of these is really helpful. We can see that up the top the basic declarations happen, and that then some assignment takes place. The variable names of days, minutes and seconds are descriptive enough that they make the comments superfluous.

Other Quibbles

getChar actually returns an int. Pretty much anywhere you have a char, you should replace it with an int:

int getChar()
{
    int retVal = -1;
    int trashVal = -1;
    ...
}

int main() 
{
    int command = 0;
    ....
}

Also, why getc(stdin) over simply getchar()? Either is fine really, but getchar is slightly more idiomatic when reading from stdin.

With any slightly more modern version of C, prefer const declarations to using #define. It's good that you didn't simply write magic numbers all over the place, but prefer things like:

const int seconds_per_minute = 60;

to their #defined counterparts. (They should probably also be static, but don't get too hung up on this while learning).

There are a number of reasons for this. The biggest reason is the fact that #define is just a dumb text replacement mechanism. Say you have a larger program and you are trying to debug it - since #define simply replaces a textual pattern with a given value, all symbols are lost. It's great in the source code to not have any magic numbers, but in a debugger, you won't have that luxury with #define - it'll be back to magic numbers all over again.

The other more minor reasons are that const guarantees it won't change, and if you need to take the address of any of these variables for any reason, well, you're totally out of luck with #define.

That being said, you cannot always get away with using const instead of #define. There's a big post about this on StackOverflow that is worth reading.

Finally, try to avoid using global variables (variables outside the scope of any function, so TIME_TYPE start, end;). In a small program like this it doesn't matter too much, but it's a good habit to get into.

I've typed a bit of a wall of text, but most of this stuff is pretty minor. Summing up:

  • Always compile with all warnings enabled.
  • Prefer 4 space to 8 space indentation.
  • Don't use #define to introduce a new type or a type alias; use typedef.
  • Prefer to use const variables to #define'd variables.
  • Try to avoid global variables if possible.
\$\endgroup\$
  • 1
    \$\begingroup\$ I use 8 space indentation as per the [Linux Kernel Coding Guidlines][kernel.org/doc/Documentation/CodingStyle]. I like the typedef recommendation, I will attempt to adopt that in the future. I do not, however, understand defining constants as variables, even static variables, when a #define makes it perfectly clear what is what. As for the global variables, I concur. However, for what sorts of programs would a global variable help? Also, getc(stdin) works for specifying that it is stdin, and enables a possible file stream in a future revision. Thanks for your input! \$\endgroup\$ – Aviator45003 May 9 '13 at 6:17
  • \$\begingroup\$ @T.C. Fair enough on the indentation - who am I to argue with kernel developers I suppose :). I'll add some more information regarding why you should prefer to use const static over #define. \$\endgroup\$ – Yuushi May 9 '13 at 6:21
  • 1
    \$\begingroup\$ @T.C. I've edited my post. For global variables, particular state changes that need to be seen by large swathes of the system would likely be global - however, this in itself is (highly) questionable design. Globals are something of a code smell - generally if you think through the design more carefully, you can avoid them completely. \$\endgroup\$ – Yuushi May 9 '13 at 6:38
3
\$\begingroup\$

I was about to post a list similar to that posted by @Yuushi, but since he beat me to it, I'll just post some bits and pieces he did not cover.

  • defining your own TIME_TYPE is unnecessary - just use time_t.

  • start and end should be local to main not global. Note also that although breaking programs into functions is good, and one-liners definitely have their place, in this case getTime and updateTime are unnecessary. For example you could do:

    int main(int argc, char* argv[])
    {
        const time_t start = time(0);
        int command = 0;
        do {
            printPrompt();
            command = getChar();
            printTime(time(0) - start);
        }
        while (command != 'q');
        return 0;
    }
    
  • getChar (which is too close in name to the std library getchar) saves the first char typed and returns it once the user types \n. It seems as if you really wanted unbuffered, raw input and ended up having to read all characters until the end of the line. To me, this seems clearer:

    static int getChar(void)
    {
        int retVal = getc(stdin);
        if (retVal != '\n') {
            while (getchar() != '\n') {
                /* nothing */
            }
        }
        return retVal;
    }
    

    Note that chars are of type int because they must be able to express the value EOF on end of file - and EOF does not fit into a variable of type char

  • printTime can be done with the library function strftime, although the numbers will be zero-padded. For example this:

    static void printTime (time_t diff)
    {
        char buf[20];
        struct tm *tm = gmtime(&diff);
        strftime(buf, sizeof buf, "%jd %kh %Mm %Ss", tm);
        printf("%s\n", buf);
    }
    

    will produce

    001d  0h 00m 01s
    

    EDIT - I just noticed the 1 in the day count! So it is not so clever after all!

And some pedantry:

  • Errors are best printed to stderr with fprintf(stderr, "format", ...)
  • The message printed is wrong. The user must type a carriage return to show the time.

  • Prototypes are unnecessary if you define main last. The prototypes and functions without parameters should all take a void parameter list (see https://stackoverflow.com/a/51080/119114, @Nate)

  • And, something that I may be unique in bothering about, opening braces should be in column 0 (just my opinion).

\$\endgroup\$
  • \$\begingroup\$ Best to include a link, for example, to explain the point about void params. Also, where the leading { goes is personal preference. An organization that cares would specify which way they want it in a coding standards doc. It shouldn't be left to the individual reviewer's style choices. \$\endgroup\$ – Nate May 8 '13 at 11:13
  • \$\begingroup\$ Thanks, I added the link. I also noticed (and noted) that the day-count in my printTime is wrong :-( \$\endgroup\$ – William Morris May 8 '13 at 12:12
  • \$\begingroup\$ My normal style has method opening braces in column 0, but this program was written in under a single class period, so I neglected my normal design a little (by accident). Also, I like organizing a program in the order it will be run. Having main() at the end doesn't easily lend itself to that. And similarly, thanks for the fprintf(stderr,string); remark. \$\endgroup\$ – Aviator45003 May 9 '13 at 6:25

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.