Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've been trying to write a really basic function to determine the difference (in days) between two dates. I am very new to C++ so as you can imagine I am very happy with my work. This function is part of a much larger code so I have missed off my includes, but the function does compile without warnings.

Just looking for some constructive criticism that a reasonable beginner can take on board.

I should add, this function takes two dates as strings in British format DD/MM/YYYY.

int dateDiffs(std::string date1, std::string date2){

    int startYear, endYear;
    int startMonth, endMonth;
    int startDay, endDay;

    int startInDays, endInDays;

    std::map<int,int> daysInMonth;

    daysInMonth[1]=31;
    daysInMonth[2]=28;
    daysInMonth[3]=31;
    daysInMonth[4]=30;
    daysInMonth[5]=31;
    daysInMonth[6]=30;
    daysInMonth[7]=31;
    daysInMonth[8]=31;
    daysInMonth[9]=30;
    daysInMonth[10]=31;
    daysInMonth[11]=30;
    daysInMonth[12]=31;

    startYear = atoi(date1.substr(6,4).c_str());
    startMonth = atoi(date1.substr(3,2).c_str());
    startDay = atoi(date1.substr(0,2).c_str());

    endYear = atoi(date2.substr(6,4).c_str());
    endMonth = atoi(date2.substr(3,2).c_str());
    endDay = atoi(date2.substr(0,2).c_str());

    startInDays = (startYear * 365) + (startMonth * daysInMonth[startMonth]) + startDay;
    endInDays = (endYear * 365) + (endMonth * daysInMonth[endMonth]) + endDay;

    return (endInDays - startInDays);
}
share|improve this question
1  
Would be a lot easier to calcularte the difference in seconds. Then convert that to days. –  Loki Astari Jun 23 at 19:13

3 Answers 3

@Snowbody has addressed many important points. I'll just talk about the std::map.

The function initializes daysInMonth each time, which is just unnecessary. You should have it initialized only once elsewhere, and it should also be const since it's not to be modified within the code.

If you have C++11 (which you should), you can initialize the map with an initializer list:

const std::map<int, int> daysInMonth = { {1, 31},
                                         {2, 28},
                                         {3, 31},
                                         {4, 30},
                                         {5, 31},
                                         {6, 30},
                                         {7, 31},
                                         {8, 31},
                                         {9, 30},
                                         {10, 31},
                                         {11, 30},
                                         {12, 31} };

(it can be formatted in any way you'd like)

You should also use an unsigned type for dates since they cannot be negative.

share|improve this answer

I see a number of problems with this:

  • The code as provided does not compile; you haven't shown what #include directives you've used.
  • You don't validate the input to make sure it is in the proper DD/MM/YYYY format.
  • You don't validate the parsed year, month, and day to make sure it makes sense. (For instance, you allow negatives, and any month number or day number)
  • You don't handle the case when the month number is outside the [1..12] range. This will lead to undesirable behavior (it returns 0 for the month length and expands the map)
  • You assume all years have 365 days. This completely ignores leap years (366 days with February having 29 days)

And a few stylistic issues:

  • This code should be part of a class rather than just a bare function
  • Questionable data type choice. You use a std::map<> instead of a regular array (an int[]); this slows things down with the only benefit being that you assume the month length is 0 instead of referencing memory outside the array.
  • Duplicated code (the date parsing). This should be abstracted out to a separate function (hopefully including the validation and checking)
  • Unnecessary parens around the return value.
  • You initialize the data table each time; this should be stored in a separate class so it can be created once and then referenced.
share|improve this answer
    
Well, in the interest of baby steps: I've ditched the std::map and have incorporated an array into part of my validation by making a "months limits" array: unsigned int dayLimits[13] = {1, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31}; –  rhysowen Jun 23 at 20:15

I will not be marking this as the "answer to my question" but wanted to show how I have taken your advice on board:

I have written the following class:

unsigned int dayLimits[13] = {1, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};

class userDate {

    public:             
        unsigned int uYear, uMonth, uDay;

        bool valiDate(unsigned int uYear, unsigned int uMonth, unsigned int uDay){
            if ((uYear >= 2000) && (uYear < 2100)) {
                if ((uMonth > 0) && (uMonth <= 12)){
                    if((uDay >= dayLimits[0]) && (uDay <= dayLimits[uMonth])){
                        return true;
                    }
                }
            }

        };

}startDate, endDate;
share|improve this answer
    
If you'd like this reviewed, post it as a new question. –  Jamal Jun 23 at 22:24
    
You don't account for leap years. Search the web for "Leap year calculation" and there will be many. There is even an example in Wikipedia. –  Thomas Matthews Jul 9 at 0:38

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.