2
\$\begingroup\$

I'm learning about time structs. Without assigning either in code or input routines I want the current time. Please help me refactor this, if it should be.

This code just tries to make a time struct with the current UTC time. How am I doing?

// better_current_gmtime_struct_builder.c
// compile with :
// gcc -Wall -g better_current_time_struct_builder.c -o better_current_time_struct_builder

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

/* function declarations */
void print_struct_parts(struct tm* t);

int main()
{

    time_t now;

    /* set the time for now */
    now = time( NULL ); /* NULL is a control character */

    /* set up time ponter */
    struct tm *tp;
    tp = gmtime(&now);

    /* quick check of time expected */  
    printf("%s\n", asctime(tp));

    /* assign the current time struct from the time pointer */
    struct tm t = *tp;

    /* print the parts */
    print_struct_parts(&t);
    return 0;   
}

/* check the struct parts */
void print_struct_parts(struct tm* t)
{
    printf ("%i\n", t->tm_wday);
    printf ("%i\n", t->tm_mon);
    printf ("%i\n", t->tm_mday);
    printf ("%i\n", t->tm_hour);
    printf ("%i\n", t->tm_min);
    printf ("%i\n", t->tm_sec);
    printf ("%i\n", t->tm_year);
}
\$\endgroup\$
  • \$\begingroup\$ Please do not modify the original code based on answers. \$\endgroup\$ – Jamal Jul 18 '15 at 5:22
  • 2
    \$\begingroup\$ @DouglasG.Allen Code Review is, in its foundation, a Q&A site. Changing your question based on an answer causes confusion about what has been review, which version has been reviewed, and what has not been reviewed. You are very welcome to ask a follow-up question, and including some information about what has been changed. The community has found that follow-up questions are the best way to handle iterative reviews. \$\endgroup\$ – Simon Forsberg Jul 19 '15 at 14:25
  • \$\begingroup\$ Alright then. I guess what I'm getting at is that the modifications that I made are posted here github.com/DouglasAllen/C_Programming_Refference/blob/master/… It now shows that there isn't any daylight savongs for UTC time and since I found what all parts are included. Sorry for the confusion. I was just waking up and had this revelation that I should go here and check what I did. To my surprise when I saw that my new code was gone I was a bit upset. Good thing that I saved it on github. \$\endgroup\$ – Douglas G. Allen Jul 19 '15 at 18:15
  • \$\begingroup\$ Hi Douglas, please take some time to read this discussion: What you may and may not do after receiving answers \$\endgroup\$ – glampert Jul 19 '15 at 18:37
5
\$\begingroup\$

Being brutally honest with you: your comments are useless. All of them could be erased. In special, this one is also misleading:

now = time( NULL ); /* NULL is a control character */

Wut? No NULL is not a control character. You can pass a time_t pointer to time() and get the output written to the given variable, or you can pass NULL and get the output from the returned value. It has nothing to do with 'control characters', whatever that means.

Just yesterday I answered this question that had the same problem with noisy comments. I'll quote my own answer:

Don't write noisy and obvious comments. The overall rule about code commenting is to use them to explain why you solved a problem in a given way, or to give any extra information (web links, mathematical formulas, etc) that will aid the understanding of a piece of code. Explaining what the code is doing should not be necessary if the code is well written. Well written code is self explanatory.


Apart from that, I would also give print_struct_parts() a better name that reflects what kind of struct it is printing. print_time() comes to mind...

Maybe also consider taking a FILE stream as parameter so that the caller is not bound to stdout.

\$\endgroup\$
  • \$\begingroup\$ In standard ASCII NULL is a control code and since C can only use a 0 (zero) as there aren't any Booleans why not think of it that way? 0 or NULL will do the same thing. But anyway see the posted comment above for the modified code. It still may need comments fixed. \$\endgroup\$ – Douglas G. Allen Jul 19 '15 at 18:25
  • \$\begingroup\$ @DouglasG.Allen, Yes true, character 0x00 in the ascii table is the "null" (usually spelled NUL, with just one L), but in the C lang, NULL is the null pointer literal, so two different concepts better not mixed together, IMHO. \$\endgroup\$ – glampert Jul 19 '15 at 18:35
2
\$\begingroup\$

This code isn't that bad, once you remove the inane comments. There is also an unnecessary copying of a struct with struct tm t = *tp; that should be eliminated.

In addition, I propose some minor tweaks:

  • Listing includes in alphabetical order
  • Putting main() last to avoid having to declare print_struct_parts()
  • Combining variable initialization with the declarations
  • puts() instead of printf("%s\n", …)
  • Declare that print_struct_parts() takes a const
  • Coalesce consecutive printf() calls
#include <stdio.h>
#include <time.h>

void print_struct_parts(const struct tm* t)
{
    printf("%i\n%i\n%i\n%i\n%i\n%i\n%i\n",
           t->tm_wday, t->tm_mon, t->tm_mday,
           t->tm_hour, t->tm_min, t->tm_sec, t->tm_year);
}

int main()
{
    time_t now = time(NULL);
    struct tm *tp = gmtime(&now);

    /* quick check of time expected */
    puts(asctime(tp));

    print_struct_parts(tp);
}
\$\endgroup\$
  • \$\begingroup\$ Looks great. I'll have to try that and a few tweeks as mentioned below. \$\endgroup\$ – Douglas G. Allen Jun 12 '15 at 1:59

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.