9
\$\begingroup\$

Below I have code that calculates epoch seconds. It is built to try to change run time checks into asserts in order to give performance in production. I want to make sure all the edge cases are correct, though.

#define DAYSPERWEEK (7)                                                                      
#define DAYSPERNORMYEAR (365U)                                                               
#define DAYSPERLEAPYEAR (366U)                                                               

#define SECSPERDAY (86400UL) /* == ( 24 * 60 * 60) */                                        
#define SECSPERHOUR (3600UL) /* == ( 60 * 60) */                                             
#define SECSPERMIN (60UL) /* == ( 60) */                                                     
#define USECPERSEC (1000000UL)                                                               
#define USECPERMILLI (1000UL)                                                                

#define LEAPYEAR(year)          (!((year) % 4) && (((year) % 100) || !((year) % 400)))       

inline static uint32_t getSecsSinceEpoch(uint32_t aMonth,                             
                                         uint32_t aDay,                               
                                         uint32_t aYear,                              
                                         uint8_t aHour=0,                             
                                         uint8_t aMinute=0,                           
                                         uint8_t aSecond=0) {                         

    const static int leapYearMonthTable[2][12] = {                                    
            {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31},                         
            {31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31}                          
    };                                                                                

    assert(aMonth < 13);                                                              
    assert(aDay < 32);                                                                
    assert(aYear > 1969 && aYear < 2100); // between epoch start and 2100             
    assert(aHour < 24);                                                               
    assert(aMinute < 60);                                                             
    assert(aSecond < 60);                                                             

    uint16_t epoch = 1970;                                                            
    uint16_t aYears = aYear-epoch;                                                    
    uint32_t countleap = 0;                                                           
    for(uint16_t i = 0; i < (aYears - 1); i++) {                                      
        if (LEAPYEAR((epoch + i)))                                                    
            countleap++;                                                              
    }                                                                                 

    uint32_t secs = aYears * (SECSPERDAY * 365);                                      
    secs += (countleap * SECSPERDAY);                                                 
    secs += aSecond;                                                                  
    secs += (aHour * SECSPERHOUR);                                                    
    secs += (aMinute * SECSPERMIN);                                                   
    secs += ((aDay - 1) * SECSPERDAY);                                                

    if(aMonth > 1) {                                                                  
        uint8_t aDaysPeraMonth = 0;                                                   

        // Only counts when we're on leap aDay or past it                             
        if(LEAPYEAR((epoch + aYears))) {                                              
            if(aMonth > 2) {                                                          
                aDaysPeraMonth = 1;                                                   
            } else if(aMonth == 2 && aDay >= 29) {                                    
                aDaysPeraMonth = 1;                                                   
            }                                                                         
        }                                                                             

        for(uint8_t i = 0; i < aMonth - 1; i++)                                       
            secs += (leapYearMonthTable[aDaysPeraMonth][i] * SECSPERDAY);             
    }                                                                                 
    return secs;                                                                      
} 
\$\endgroup\$
3
  • 1
    \$\begingroup\$ You could also add main() to show some testing for this code. \$\endgroup\$
    – Jamal
    Jan 26, 2015 at 16:47
  • \$\begingroup\$ assert(aDay > 0);assert(aMonth>0); \$\endgroup\$ Jan 26, 2015 at 19:36
  • \$\begingroup\$ Is this code part of a larger library? DAYSPERWEEK, USECPERSEC, and USECPERMILLI are irrelevant to this task. \$\endgroup\$ Jan 26, 2015 at 21:16

2 Answers 2

7
\$\begingroup\$

Get rid of all your #define

#define DAYSPERWEEK (7)                                                                      
#define DAYSPERNORMYEAR (365U)                                                               
#define DAYSPERLEAPYEAR (366U)                                                               

#define SECSPERDAY (86400UL) /* == ( 24 * 60 * 60) */                                        
#define SECSPERHOUR (3600UL) /* == ( 60 * 60) */                                             
#define SECSPERMIN (60UL) /* == ( 60) */                                                     
#define USECPERSEC (1000000UL)                                                               
#define USECPERMILLI (1000UL)                                                                

#define LEAPYEAR(year)          (!((year) % 4) && (((year) % 100) || !((year) % 400))) 

These can all be replaced with scoped variables of const. You can also make the multiplications inline (which you absolutely should do). Those comments are dangerous. What happens if the 86800UL was the value but the comment was /* == ( 24 * 60 * 60) */ did you notice that the value here was wrong? The compiler can not check that the value matches the comments. So never do that (put the calculation in the code).

unsigned long constexpr SecsPerMin   = 60;
unsigned long constexpr SecsPerHour  = (60 * SecsPerMin);
unsigned long constexpr SecsPerDay   = (24 * SecsPerHour);

All of these are compile time evaluated.

Also the macro that expands. This should be a function:

bool isLeapYear(int year) {return !(year % 4) && ((year % 100) || !(year % 400));}

This is a premature optimization that makes the code hard to reason about:

if(aMonth > 1) { 

The code should work just fine without that statement.

This is over-complicated:

    uint8_t aDaysPeraMonth = 0;                                                   

    // Only counts when we're on leap aDay or past it                             
    if(LEAPYEAR((epoch + aYears))) {                                              
        if(aMonth > 2) {                                                          
            aDaysPeraMonth = 1;                                                   
        } else if(aMonth == 2 && aDay >= 29) {                                    
            aDaysPeraMonth = 1;                                                   
        }                                                                         
    }         

You can just say:

    uint8_t aDaysPeraMonth = isLeapYear(epoch + aYears)
                                   ? 1
                                   : 0;

Remember that you are counting the days of the current month separately with this statement:

secs += ((aDay - 1) * SECSPERDAY);  

So your loop only has to deal with months that you have used all the days in.

Why do you try and use every single unsigned format?

inline static uint32_t getSecsSinceEpoch(uint32_t aMonth,                             
                                         uint32_t aDay,                               
                                         uint32_t aYear,                              
                                         uint8_t aHour=0,                             
                                         uint8_t aMinute=0,                           
                                         uint8_t aSecond=0) {   

Pick one that can be used by all the values: unsigned long should do. Use this type everywhere. Then you don't need to worry about extra code converting the types in nearly every expression.

Couple of missing asserts

assert(aDay > 0);
assert(aMonth>0);

Also the use of assert() is not that great. It only works in debug environments. In production code they don't exist. What do you expect to happen if your invariants are broken in production?

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the review this helps. I didn't see any comments on the logic being wrong for edge cases and this was my primary concern but also the clean up of types, extra assertions and removal of macros helps the code also. \$\endgroup\$
    – bjackfly
    Jan 29, 2015 at 13:39
10
\$\begingroup\$

In C++, you should use const instead of macros for constants:

const int DAYS_PER_WEEK = 7;                                                                    
const int DAYS_PER_NORM_YEAR = 365;                                                               
const int DAYS_PER_LEAPYEAR = 366;                                                               

const long int SECS_PER_DAY = 24 * 60 * 60;                               
const long int SECS_PER_HOUR = 60 * 60;
const unsigned long int SECS_PER_MIN = 60;
const unsigned long int USEC_PER_SEC = 1000000;
const unsigned long int USEC_PER_MILLI = 1000;

This will allow you to use explicit types instead of adding type literals. I've also added underscores to the names as that's the proper convention for compound words in all-uppercase macro names. If these are to be reused, then they could also be wrapped in a namespace to prevent any name-clashing.

As for LEAPYEAR, use a function instead:

int getLeapYear(int year)
{
    return !((year) % 4) && (((year) % 100) || !((year) % 400))
}

Overall, this code looks extremely C-like. If you must use macros for whatever reason, then just program this in C. But if you're indeed using C++, and you have C++11, you can even utilize some newer features from the <chrono> library.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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