4
\$\begingroup\$

I've created a very basic date data type in C simply to practice writing my own data type. Please critique.

#include <stdio.h>
#include <stdlib.h>
enum DATE_FORMAT{DASHES,SLASHES};
typedef enum DATE_FORMAT DFORMAT;
typedef struct {
    unsigned char day;
    unsigned char month;
    unsigned short year;
} DATE;

/* Allocates new DATE with specified values and returns a ptr to it. User must NULL-check */
DATE *create_date(unsigned char month, unsigned char day, unsigned short year)
{
    if((month > 12) || (month < 1))
        return NULL;
    if((day > 31) || (day < 1))
        return NULL;
    if((year > 9999) || (year < 0))
        return NULL;
    DATE *the_date = malloc(sizeof(DATE));
    if(the_date == NULL)
        return NULL;
    the_date->day = day;
    the_date->month = month;
    the_date->year = year;
    return the_date;
}
void printf_date(DATE *input, DFORMAT format)
{
    if(input == NULL)
        printf("Invalid DATE.\n");
    if(format == DASHES)
        printf("%hhu-%hhu-%hu\n",input->month,input->day,input->year);
    if(format == SLASHES)
        printf("%hhu/%hhu/%hu\n",input->month,input->day,input->year);
    return;
}
void free_date(DATE *dtf)
{
    if(dtf == NULL)
    {
        printf("Date free failed.\n");
    }
    else
    {
        free(dtf);
        printf("Date freed successfully\n");
    }
    return;
}


int main()
{
    DATE *my_date = create_date(3,17,2017);
    if(my_date == NULL)
    {
        printf("Please provide a valid date next time... exiting.\n");
        return(EXIT_FAILURE);
    }
    printf_date(my_date, SLASHES);
    printf_date(my_date, DASHES);
    free_date(my_date);
    return(EXIT_SUCCESS);
}
\$\endgroup\$
3
\$\begingroup\$
DATE *create_date(unsigned char month, unsigned char day, unsigned short year)

What if I want to create a DATE on the stack?


    if((month > 12) || (month < 1))
        return NULL;
    if((day > 31) || (day < 1))
        return NULL;
    if((year > 9999) || (year < 0))
        return NULL;

There are many date libraries out there, and they're not consistent on their offsets for months. I would consider creating an enum for the months.

Also, either don't validate the values at all or validate them fully. If I can't create 2017-13-01 then why can I create 2017-02-31? If I can't create a date in 753 BC then why can I create a date in year 0 (whatever that means – 1 BC?)?

Obviously that comes with some caveats, particularly with respect to the transition from the Julian to the Gregorian calendar; the documentation should make it clear what the transition date is and what days don't exist in consequence.


void printf_date(DATE *input, DFORMAT format)

Wouldn't sprintf_date be more useful? You can print to stdout given sprintf, but you can't print to anything other than stdout given only printf.

        printf("%hhu-%hhu-%hu\n",input->month,input->day,input->year);
        printf("%hhu/%hhu/%hu\n",input->month,input->day,input->year);

No. Just no. ISO 8601 is the only date format that should be used unless a locale parameter is supported.

\$\endgroup\$
  • \$\begingroup\$ Strongly agree with "either don't validate the values at all or validate them fully" \$\endgroup\$ – chux Sep 15 '17 at 18:15
  • \$\begingroup\$ "Wouldn't sprintf_date be more useful?" Good idea. Then this code set should also provide a macro like SPRINTF_DATE_N that would be the size needed to accommodate all possible sprintf_date() cases. \$\endgroup\$ – chux Sep 15 '17 at 19:49
2
\$\begingroup\$

It's a nice piece of work. Short, understandable, to the point.

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

After the includes, there should be an empty line. It's like having a paragraph in a long prose text, to help the reader.

enum DATE_FORMAT{DASHES,SLASHES};
typedef enum DATE_FORMAT DFORMAT;

Since you don't use the name DATE_FORMAT, you can also write it in a single line: typedef enum { DASHES, SLASHES } DFORMAT.

typedef struct {
    unsigned char day;
    unsigned char month;
    unsigned short year;
} DATE;

Technically speaking, you are right. The date components fit in an unsigned char or an unsigned short. Nevertheless I would choose unsigned int for all of the fields, simply because I want to express that the date components are integers. I would only use the smaller types when it comes to hardware-related things.

/* Allocates new DATE with specified values and returns a ptr to it. User must NULL-check */
DATE *create_date(unsigned char month, unsigned char day, unsigned short year)
{
    if((month > 12) || (month < 1))
        return NULL;
    if((day > 31) || (day < 1))
        return NULL;
    if((year > 9999) || (year < 0))
        return NULL;
    DATE *the_date = malloc(sizeof(DATE));
    if(the_date == NULL)
        return NULL;
    the_date->day = day;
    the_date->month = month;
    the_date->year = year;
    return the_date;
}

Great code. Well, except for the order of parameters. Almost all other people who designed a similar function used the order year, month, day. It's simple to remember: from large to small. The American order month, day, year is confusing to the rest of the world.

void printf_date(DATE *input, DFORMAT format)
{
    if(input == NULL)
        printf("Invalid DATE.\n");
    if(format == DASHES)
        printf("%hhu-%hhu-%hu\n",input->month,input->day,input->year);
    if(format == SLASHES)
        printf("%hhu/%hhu/%hu\n",input->month,input->day,input->year);
    return;
}

The return at the end is redundant. Just remove it.

If you had chosen unsigned int for the date components, the printf specifiers would be the natural %u here instead of the distracting %hhu and %hu, which is much more common and therefore easier to read.

Since you include the \n at the end of the output, your function should better be called println_date. The standard printf function doesn't append a newline, so neither should your function.

void free_date(DATE *dtf)
{
    if(dtf == NULL)
    {
        printf("Date free failed.\n");
    }
    else
    {
        free(dtf);
        printf("Date freed successfully\n");
    }
    return;
}

The return is redundant.

As soon as your program works, the printf calls must go away.

You can omit the if clause, since the free function can handle a NULL argument. It does nothing in that case.

The parameter name looks weird. Does it mean date_to_free? Don't use abbreviations. The name should simply be date.

So the code should look like:

void free_date(DATE *date) {
    free(date);
}

Continuing with your code:

int main()

Just empty parentheses mean unspecified number of arguments for historic reasons. You should write int main(void) instead.

{
    DATE *my_date = create_date(3,17,2017);
    if(my_date == NULL)
    {
        printf("Please provide a valid date next time... exiting.\n");
        return(EXIT_FAILURE);
    }

Error messages don't belong on stdout. They should be written to stderr, like this:

fprintf(stderr, "Please provide ...");

Continuing with your code:

    printf_date(my_date, SLASHES);
    printf_date(my_date, DASHES);
    free_date(my_date);
    return(EXIT_SUCCESS);
}

Very nice. But since return is not a function, it shouldn't look like it. Instead, write return EXIT_FAILURE without the parentheses.

\$\endgroup\$
1
\$\begingroup\$

When creating a structure such as DATE, which may have a very large number of instances, using types narrower than int/unsigned makes sense for space economy.

typedef struct {
    unsigned char day;
    unsigned char month;
    unsigned short year;
} DATE;

All uppercase tokens like DATE, from a style point-of-view, are typically reserved for macro constants. Still this is valid code. I would recommend something date_ymd instead.


The create_date() signature unnecessarily conforms to the details of DATE. Why does code use parameter type unsigned char for month? Looks like an implementation detail. By using int/unsigned parameters, the function is open to wider use. Else this DATE code obliges the calling code to check for type narrowing issues.

The parameter order may be consistent with a certain locale, yet year, month, day matches the spirit of ISO8601

The following alternative breaks out aspects of calendar functions that will likely prove useful in other DATE functions.

Make public the range of allowable years by adding those limits to date_ymd.h

// date_ymd.h
#define DATE_YMD_YEAR_MIN 1
#define DATE_YMD_YEAR_MAX 9999
date_ymd *create_date(int year, int month, int day);

// date_ymd.c
static int eom(int year, int month) {
  assert(month >= JAN && month <= DEC);
  static const char month_since_January_length[MONTH_PER_YEAR] = {
    31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };
  if (month == FEB && isleapyear(year)) {
    return month_since_January_length[FEB-JAN] + 1;
  }
  return month_since_January_length[month-JAN];
}

static bool valid_date(int year, int month, int day) {
  if(year < DATE_YMD_YEAR_MIN || year > DATE_YMD_YEAR_MAX) {
    return false;
  }
  return month >= JAN && month <= DEC && day >= 1 && day <= eom(year, month);
}

date_ymd *create_date(int year, int month, int day) {
  if (!valid_date(year, month, day)) {
    return NULL;
  } 
  ...

Rather than allocate to the size of the type, allocate to the size of the referenced variable. Easy to code, review and maintain.

 // DATE *the_date = malloc(sizeof(DATE));
 DATE *the_date = malloc(sizeof *the_date);

As printf_date() does not alter the pointer data, best to use const for wider applicability and better emitted code.

IMO, the name printf_date() should reflect the structure's name closer and lead with that.

Let the calling code print a '\n' if it needs to.

Some option should match ISO 8601 with its y-m-d order and minimum 4 digit year and 2 digit month/day - zero filled

A switch statement is appropriate here.

I'd expect a print function to handle or return the return value from printf(), not simply void it.

The "h" and "hh" are OK , but not needed here.

int DATE_printf(const DATE *input, DFORMAT format) {
  switch (format) {
    case MDY_DASHES: 
      return printf("%u-%u-%u", input->month, input->day, input->year);
    case ISO_8601:
      return printf("%04u-%02u-%02u", input->year, input->month, input->day);
    ...

free() allows free(NULL), so recommend to follow that idiom with free_date(). No need to print anything.

void free_date(DATE *dtf) {
    free(dtf);
}
\$\endgroup\$

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.