4
\$\begingroup\$

I'm trying to write a complete driver for Visual FoxPro's DBF format for Win32. I found that the way that fields of the DATETIME type are stored in dBase files is a bit strange: it's an 8 byte string where the first four bytes represent the date as the number of days since January 1, 4713 BC, and the last four bytes represent the time as:

(Hours * 3600000) + (Minutes * 60000) + (Seconds * 1000)

The DATE type is much simpler as it represents a YYYYMMDD date as an ASCII string.

Here is my function for converting this date format into a SYSTEMTIME struct:

#include <Windows.h>
#include <sal.h>

_Success_(return == TRUE)
_Ret_range_(FALSE, TRUE)
_Check_return_

BOOL WINAPI FoxProDateTimeToSystemTime(
    _In_reads_(cbFxpTime) BYTE *bFoxProDateTime, 
    _In_ SIZE_T cbFxpTime, 
    _Out_writes_(1) LPSYSTEMTIME lpSystemTime
)
{
    LONG lDate, lTime;
    CONST DOUBLE dConvert = 2440587.5;
    DOUBLE dTemp;
    LONGLONG uxTime;
    LONGLONG llTime;
    FILETIME ft;
    
    if (NULL == bFoxProDateTime || NULL == lpSystemTime)
    {
        SetLastError(ERROR_INVALID_PARAMETER);
        return FALSE;
    }
    ZeroMemory(lpSystemTime, sizeof(SYSTEMTIME));
    if (cbFxpTime < 8)
    {
        SetLastError(ERROR_INSUFFICIENT_BUFFER);
        return FALSE;
    }

    CopyMemory(&lDate, bFoxProDateTime, 4);
    CopyMemory(&lTime, bFoxProDateTime + 4, 4);

    dTemp = (lDate - dConvert) * 86400;
    uxTime = (LONGLONG)dTemp;
    llTime = Int32x32To64(uxTime, 10000000) + 116444736000000000;
    ft.dwLowDateTime = (DWORD)llTime;
    ft.dwHighDateTime = llTime >> 32;
    FileTimeToSystemTime(&ft, lpSystemTime);

    lpSystemTime->wHour = (WORD) (lTime / 3600000);
    lpSystemTime->wMinute = (WORD) ((lTime % 3600000) / 60000);
    lpSystemTime->wSecond = (WORD) (((lTime % 3600000) % 60000) / 1000);

    SetLastError(ERROR_SUCCESS);
    return TRUE;
}

The way the algorithm works right now is a bit clunky since it basically does three conversions in a row to end up with a SYSTEMTIME - first to a UNIX timestamp, then to a FILETIME struct, and finally to a SYSTEMTIME struct. This function works; I've tested it, but I'm wondering if I can get some feedback on how to improve the efficiency of this conversion.

\$\endgroup\$
0

1 Answer 1

3
\$\begingroup\$

meaningful identifier

    CONST DOUBLE dConvert = 2440587.5;

Simonyi and Spolsky would tell you that this is not the original Apps Hungarian Notation. It does not help the Gentle Reader, 'nuff said on that topic, let's ignore the redundant d prefix.

"Convert" is not super helpful. This could be an OK identifier if accompanied by a comment describing which epoch this denotes, or by the URL of some cited calendar reference. Absent such text, prefer something more descriptive like dJulian1Jan1970, or dJd1Jan1970. The "noon --> midnight" aspect of 0.5 still suggests that a brief comment is needed. You might find it more convenient to bury this constant in a brief convertJulianDayToUnixTime() helper function.

    DOUBLE dTemp;

Ok, we weren't even trying to name this quantity. The code eventually makes it clear that it is the dUnixTime of some UTC midnight instant. It's very short lived -- not sure why we chose to name it.

    LONGLONG uxTime;

Aha! Finally, a good name, this is a "unix time".

    LONGLONG llTime;

Yes, it is a time, but that doesn't make it a good name. It appears to be an llTicks in units of 100-nanosecond ticks since 1970.

    llTime = Int32x32To64(uxTime, 10000000) + 116444736000000000;

Ten million tick/sec isn't the easiest number to visually scan when it's not 1e7. Isn't there some convenient 10_000_000 notation available? Maybe not. Consider creating a MANIFEST_CONSTANT for this magic number.

And that other figure is just out of control. We seem to be talking about IDK 1-Jan-1601 or something? In terms of ticks. A software artifact containing such an expression is not maintainable and should never be merged down to main. Doesn't matter if it happens to compute the correct quantity. It's not obvious that it does, and it does not invite us to trace details from source code back to a requirements document or a standards document.

Choosing essentially the same name twice, lTime and llTime, does not help the Gentle Reader to understand Author's Intent.


type punning

    CopyMemory(&lDate, bFoxProDateTime, 4);
    CopyMemory(&lTime, bFoxProDateTime + 4, 4);

The OP introductory prose was very clear and helpful, outlining that we have a Julian Day followed by UTC milliseconds since midnight. Which suggests a two-element struct. Consider using a struct pointer to bFoxProDateTime to accomplish what these two lines are doing.


word width

    llTime = Int32x32To64(uxTime, 10000000) + 116444736000000000;
    ft.dwLowDateTime = (DWORD)llTime;

Even when running on a 64-bit host, this appears to be a uint32 we just cast the sum to.

Notice that there have been significantly more than four billion ticks since 1601. I imagine this line does the right thing, but it's not obvious that it does. Consider adopting types like uint64.

OIC, you meant this:

    LONGLONG mask32 = 0xFfffFfff;
    ft.dwLowDateTime = (DWORD)(llTime & mask);

Consider adding an explicit mask. Not for the machine. For the reader. Generated machine code won't change at all. Though I wouldn't be surprised if original line was UB.

It's unclear why pointer type-punning or a memcpy() couldn't accomplish this in a single line, but maybe the two fields are in incoveniently backward order or something.


define conversion factors

    lpSystemTime->wMinute = (WORD) ((lTime % 3600000) / 60000);

Again, this is more tedious to read than 3600_000, so give it a name like HOUR_TO_MILLISECOND.

We compute these in H,M,S order, but starting with seconds might have been more convenient. Because then we can subtract out a nicely named seconds or minutes that we just computed. Generated machine code likely wouldn't change, though readability would.


test suite

OP includes no unit tests.

This code does timestamp conversion for DBF v1 input files. Suppose another version comes out and a maintainer needs to refactor this code to also parse v2 input files. How would we know if we broke anything in the process?

Adding automated unit tests would be an essential first step for such a maintenance activity.


This code appears to achieve its design goals.

I would not be willing to delegate or accept maintenance tasks on it in its current form.

\$\endgroup\$
1
  • \$\begingroup\$ "so give it a name like HOUR_TO_MSEC" is a good example of when to depart from macros all in upper case: better not a mega-seconds, but as HOUR_TO_mSEC \$\endgroup\$ Nov 3, 2023 at 7:59

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.