Edit: I very wrongly accused Jacobi of posting a flawed algorithm. So I removed my ranting and replaced by:
I had a try compiling your code and I worked on it to represent how I would have written it. See below. I hope this gives you hints about a different possible coding style. I hope to demonstrate some benefits. The main differences are:
- Comment for readability. In six months you might be very much in doubt yourself of all the intricacies of an algorithm. I had a hard time deciphering your algorithm. I am also in favor of including references to the definitions of formulas etc, which allows reviewers to check them more quickly.
- A good practice that I still don't manage to discipline myself to is to write commenting immediately for doxygen or another doc generator, so you can save yourself the hassle later. It usually forces you to document all parameters, return values, ... which is good practice anyway.
- Code formatting to represent the structure of the code. I am a big fan of lots of spacing and horizontal alignment to spot the difference between similar lines easily.
- I would refactor to avoid repeating the same code twice, even in the case of isLeapYear. I think the algorithm also becomes more readable when you change the condition by a clear function name. For reading how the algorithm works, I actually don't care about the implementation of the leapyear test.
- Instead of testing for
m < 2 to add or remove extra leapyears, I think it is beneficial to try and represent the reality in your code. I do this here by creating N_DAYS_IN_LEAP_MONTH. This benefits not only in simplifying the code, it also serves in multiple places, as now the input validation is more correct as well.
- Returning -1 on error is misleading because it could also be a valid outcome. I would propose in any case to return 0 on error instead of -1, so it can be distinguished from valid output with a Boolean test as
if( x = getUnixTimeStamp(...) ) -> valid. If you want to garantee no ambiguity, you should probably return a struct with an error status, an error message and a result.
- Personally I prefer longer variable names for clarity, but I suppose that has to do with taste. I also changed nleapyears to nleapdays, which corresponds more to what is is.
- I try to use const where I can. In this example it is a bit trivial, and compilers would probably compile it to the exact same assembler code without these consts, so you have to see if you find it worth the extra code. If your parameters aren't basic types, but objects however starting to take them as const refernces really starts making a difference.
Not implemented but advised:
- Allow supporting inputs for the whole range of double. This makes your function more general, and thus more useful. If a program logic needs to restrict more, that should not be done in this function.
#include <algorithm>
#include <iostream>
double getUnixTimeStamp( int year, int month, int day, int hour, int minute, int second );
int main()
{
std::cout << std::fixed << getUnixTimeStamp( 1979, 12, 31, 23, 59, 59 ) << std::endl;
return 0;
}
// start of getUnixTimeStamp code
static const int N_DAYS_IN_MONTH [] = { 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };
static const int N_DAYS_IN_LEAP_MONTH[] = { 31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };
// Is the passed in year a leap year?
// https://en.wikipedia.org/wiki/Leap_year#Algorithm
inline
bool
isLeapYear( const int year )
{
return
year % 400 == 0
|| ( year % 4 == 0 && year % 100 != 0 )
;
}
// returns the epoch timestamp for a given date in UTC
// reference: https://en.wikipedia.org/wiki/Unix_time
// current minimum supported date = 0000-01-01 00:00:00
// current maximum supported date = 2999-12-31 59:00:00
// returns 0 on faulty input, as well as for epoch input
// TODO: support full range of double rather than limiting arbitrarily
double
getUnixTimeStamp( const int year, const int month, const int day, const int hour, const int minute, const int second )
{
// Do some bounds checking
// Note that double will still not be indefinite, and you should check what the boundaries for double are.
// (this also changes per implementation, so you need to check limits.h or something like that)
// It would be more logical and correct to limit to the range permitted by double than to arbitrarily limit a 0y.
// If your program needs this limit, you should enforce it outside this function, because it doesn't belong here.
const int* const DAYS_PER_MONTH = isLeapYear( year ) ? N_DAYS_IN_LEAP_MONTH : N_DAYS_IN_MONTH;
if
(
year < 0 || year > 3000
|| month < 1 || month > 12
|| day < 1 || day > DAYS_PER_MONTH[ month-1 ]
|| hour < 0 || hour > 24
|| minute < 0 || minute > 60
|| second < 0 || second > 60
)
return 0;
// it is problematic to do error handling with the return value, as all possible return values could be valid epochs as well.
// at least by taking 0 you can use a boolean check like if( x = getUnixTimeStamp(...) ) -> valid.
// The check will fail for 1970-01-01-00-00-00
// Calculate the seconds since or to epoch
// Formula for dates before epoch will subtract the full years first, and then add the days passed in that year.
int epoch = 1970 ; // UNIX epoch 01/01/1970
int sign = year > epoch ? 1 : -1 ; // So we know whether to add or subtract leap days
int nleapdays = 0 ; // The number of leap days
int monthsNDays = 0 ; // The number of days passed in the current year
// Count the leap days
for( int i = std::min( year, epoch ); i < std::max( year, epoch ); i++ )
if( isLeapYear( i ) )
++nleapdays;
// Calculate the number of days passed in the current year
for( int i = 1; i < month; i++ )
monthsNDays += DAYS_PER_MONTH[ i - 1 ];
return
(
( year - epoch ) * 365 // add or subtract the full years
+ sign * nleapdays // add or subtract the leap days
+ monthsNDays // The number of days since the beginning of the year for full months
+ day - 1 // The number of full days this month
) * 86400 // number of seconds in 1 day
+ hour * 3600
+ minute * 60
+ second
;
}
mktime()\$\endgroup\$