code review

This is a discussion on code review within the C Programming forums, part of the General Programming Boards category; Hi, I did an c program exercise where I enter a date in format DD MM YYYY (eg. 31 12 ...

  1. #1
    Registered User hamsteroid's Avatar
    Join Date
    Mar 2007
    Location
    Waterford, Ireland
    Posts
    62

    code review

    Hi,
    I did an c program exercise where I enter a date in format DD MM YYYY (eg. 31 12 2003) to output 31st December 2003. Restrictions at this stage in the book I am reading are: no strings, loops or functions.

    After accepting the input, I check the date format is correct and check for leap days. If that is ok, I then use 2 switches to append "st, nd, rd, th" to the day and substitute the month name for the month number and finally print out the year. Here is my code. I feel that it is long winded and could be optimized. Care to criticize my code? Maybe the switch could be improved, ie is possible to have 1st 21st and 31st all on 1 line?

    Code:
    int main(void)
    {
      int day = 0;                    /* capture user input day, month, year */
      int month = 0;
      int year = 0;
      bool isLeapYear = 0;                             /* check for leap day */
      bool validDate = 1;                      /* flag for processing output */
     
      /* user input */
      printf("\nEnter a date in the format DD MM YYYY. eg 31 12 2003\n> ");
      scanf("%d %d %d", &day, &month, &year);
       
      /* validate entry */
      if ( ((day<1)||(day>31)) || ((month<1)||(month>12)) || (year<1))
      {
        printf("Sorry invalid date entry.\n");
        validDate = 0;
      }
      /* check day entered against the 5 months do not exceed 30 days */
      else if ( (((month==4)||(month==6)||(month==9)||(month==11)) && (day>30))
                || (month==2)&&(day>29))
      {
        printf("\nInvalid day %d for month %d.\n", day, month);
        validDate = 0;
      }
      else if ((month==2)&&(day==29)) /* check if leap year */
      {
        isLeapYear = ( ((year%4==0)&&(year%100 != 0)) || (year%400 == 0));
        if (!isLeapYear)
        {
          printf("\nInvalid day %d for non leap year %d.\n", day, year);
          validDate = 0;
        }
      }
      
      if(validDate)   /* date entry is correct proceed with internal processing */
      {
        printf("Date in long format is: %d",day);  
        switch(day)       /* print date postfix */
        {
          case 1:
            printf("st");
            break;
          case 2:
            printf("nd");
            break;
          case 3:
            printf("rd");
            break;
          case 21:
            printf("st");
            break;
          case 22:
            printf("nd");
            break;
          case 23:
            printf("rd");
            break;
          case 31:
            printf("st");
            break;
           
          default:
    	printf("th");
            break;
        }
     
        switch(month)     /* print month */
        {
          case 1:
            printf(" January");
            break;
          case 2:
            printf(" February");
            break;
          case 3:
            printf(" March");
            break;
          case 4:
            printf(" April");
            break;
          case 5:
            printf(" May");
            break;
          case 6:
            printf(" June");
            break;
          case 7:
            printf(" July");
            break;
          case 8:
            printf(" August");
            break;
          case 9:
            printf(" September");
            break;
          case 10:
            printf(" October");
            break;
          case 11:
            printf(" November");
            break;
          case 12:
            printf(" December");
            break;
          
          default:
    	printf("<undefined month>");
            break;
        }
        printf(" %d.\n\n", year);  /* print year */
      }
      return 0;
    }
    /*
    Output:
    Enter a date in the format DD MM YYYY. eg 31 12 2003
    > 1 12 2009
    Date in long format is: 1st December 2009.
    */

  2. #2
    Registered User
    Join Date
    Sep 2006
    Posts
    835
    Yes, you can say
    Code:
      case 1: case 21: case 31:
        printf("st");
        break;
    You're just taking advantage of switch's fallthrough.

    Edit: You could also replace switch(month) by creating a 12 or 13-element array of strings (you need 13 if you want indexing to start at 1). You could initialize it like
    Code:
      char *months[] = {"January", "February", /* blah blah */, "December"};
    Last edited by robatino; 04-03-2007 at 06:28 PM.

  3. #3
    Registered User hamsteroid's Avatar
    Join Date
    Mar 2007
    Location
    Waterford, Ireland
    Posts
    62
    Thanks robatino. I was thinking I could do that but I was trying case 1 : 21 : 31.

    I am using <stdbool.h> but the compiler doesn't like me assigning validDate = TRUE; - even though validDate is type bool. It works though if I assign validDate=1; (but I may as well use int then).

    Other ideas was to enum for the months but I need to refer to the vars as number in the end.
    Last edited by hamsteroid; 04-03-2007 at 06:30 PM. Reason: bool

  4. #4
    Registered User
    Join Date
    Sep 2006
    Posts
    835
    On my box, <stdbool.h> has lower-case, not upper-case, true and false defined as 1 and 0.

  5. #5
    Registered User hamsteroid's Avatar
    Join Date
    Mar 2007
    Location
    Waterford, Ireland
    Posts
    62
    robatino, good ideas, I will not be using arrays for two more chapters. But will certainly keep it in mind. In the meantime I'll take advantage of the switch's fall through and make the switch look smaller and more readable. eg. case 1: case 21: case 31:

    Thanks again.

    actually, strange about <stdbool.h> I'll try ValidDate = true; Maybe it was capital case it didn't like. I'm using gcc under mandriva 2006.1

  6. #6
    Registered User
    Join Date
    Sep 2006
    Posts
    835
    I know that in C++, a bool type has values (lower-case) true and false, so it's reasonable to assume that <stdbool.h> does the same (I think <stdbool.h> is from C99 which is after C++98).

  7. #7
    Frequently Quite Prolix dwks's Avatar
    Join Date
    Apr 2005
    Location
    Canada
    Posts
    8,048
    I know that in C++, a bool type has values (lower-case) true and false, so it's reasonable to assume that <stdbool.h> does the same (I think <stdbool.h> is from C99 which is after C++98).
    That is correct. C99 has a variable type called _Bool. In <stdbool.h> bool is defined as _Bool, and true and false are defined as well.

    In Windows programming one uses BOOL and TRUE and FALSE, and TRUE and FALSE are often defined in older C programs.
    dwk

    Seek and ye shall find. quaere et invenies.

    "Simplicity does not precede complexity, but follows it." -- Alan Perlis
    "Testing can only prove the presence of bugs, not their absence." -- Edsger Dijkstra
    "The only real mistake is the one from which we learn nothing." -- John Powell


    Other boards: DaniWeb, TPS
    Unofficial Wiki FAQ: cpwiki.sf.net

    My website: http://dwks.theprogrammingsite.com/
    Projects: codeform, xuni, atlantis, nort, etc.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Code Review for upcoming contest
    By Darryl in forum Contests Board
    Replies: 2
    Last Post: 02-28-2006, 02:39 PM
  2. Problem : Threads WILL NOT DIE!!
    By hanhao in forum C++ Programming
    Replies: 2
    Last Post: 04-16-2004, 02:37 PM
  3. True ASM vs. Fake ASM ????
    By DavidP in forum A Brief History of Cprogramming.com
    Replies: 7
    Last Post: 04-02-2003, 04:28 AM
  4. Interface Question
    By smog890 in forum C Programming
    Replies: 11
    Last Post: 06-03-2002, 06:06 PM
  5. Replies: 0
    Last Post: 02-21-2002, 06:05 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21