File stream functions

Question

Is this good or not? Input arguments for OsFile_Open must be in UTF8 format.

#include <stdio.h>
#ifdef WIN32
 #include <Windows.h>
#endif
// Types declaration, originally they are located in header file
//
typedef int Bool_T;
typedef signed long long Int64_T;
#define true 1
#define false 0
#define IN
#define OUT
Bool_T OsFile_Open( IN const char * name, IN const char * mode, OUT FILE ** file )
{
 FILE * handle; 
#ifdef WIN32
 wchar_t unicodeMode[MAX_PATH];
 wchar_t unicodeName[MAX_PATH];
 int unicodeNameLen;
 int unicodeModeLen;
 unicodeNameLen = MultiByteToWideChar( CP_UTF8, 0, name, -1, unicodeName, 0 );
 if( unicodeNameLen != 0 )
 {
 if( MultiByteToWideChar( CP_UTF8, 0, name, -1, unicodeName, unicodeNameLen ) != 0 )
 {
 unicodeModeLen = MultiByteToWideChar( CP_UTF8, 0, mode, -1, unicodeMode, 0 );
 if( unicodeModeLen != 0 )
 {
 if( MultiByteToWideChar( CP_UTF8, 0, mode, -1, unicodeMode, unicodeModeLen ) != 0 )
 {
 handle = _wfopen( unicodeName, unicodeMode );
 if( handle != NULL )
 {
 *file = handle;
 return true;
 }else{
 return false;
 }
 }else{
 return false;
 }
 }else{ // if( unicodeModeLen != 0 )
 return false;
 }
 }else{ // if( MultiByteToWideChar( CP_UTF8, 0, name, -1, unicodeName, unicodeNameLen ) != 0 )
 return false;
 } 
 }else{ // if( unicodeNameLen != 0 )
 return false;
 }
#else
 handle = fopen64( name, mode );
 if( handle != NULL )
 {
 *file = handle;
 return true;
 }else{
 return false;
 }
#endif
}
Bool_T OsFile_Close( IN FILE * file )
{
 return ( fclose( file ) == 0 ) ? true : false;
}
Bool_T OsFile_Seek( IN FILE * file, IN Int64_T offset, IN int origin )
{
 int result;
#ifdef WIN32
 result = _fseeki64( file, offset, origin );
#else
 result = fseeko64( file, offset, origin );
#endif
 return ( result == 0 ) ? true : false;
}
Bool_T OsFile_GetPos( IN FILE * file, OUT Int64_T * curPos )
{
#ifdef WIN32
 *curPos = _ftelli64( file );
#else
 *curPos = ftello64( file );
#endif 
 return ( *curPos != -1 ) ? true : false;
}
size_t OsFile_Read( IN FILE * file, IN int numOfBytes, OUT unsigned char * buffer )
{
 return fread( buffer, sizeof( unsigned char ), numOfBytes, file );
}
int OsFile_Eof( IN FILE * file )
{
 return feof( file );
}

Answer 1

I'd re-structure (the Win32 part of) the open function to something like this:

#ifdef WIN32
 wchar_t unicodeMode[MAX_PATH];
 wchar_t unicodeName[MAX_PATH];
 if (MultiByteToWideChar( CP_UTF8, 0, name, -1, unicodeName, MAX_PATH) && 
 MultiByteToWideChar( CP_UTF8, 0, mode, -1, unicodeMode, MAX_PATH) && 
 (handle = _wfopen( unicodeName, unicodeMode)) != NULL) 
 {
 *file = handle;
 return true;
 }
 return false;
#else

You don't need to call MultiByteToWideChar twice for each conversion -- the first call not only returns the length, but does the conversion as well. As it stands, your code has potential buffer overruns as well -- it used the first call to MultiByteToWideChar to measure the length that would be needed for the result, but never checked that this was less than or equal to the buffer space you provided.

It can be legitimate to do the two consecutive calls, especially if (for example) you want to ensure you can handle any possible input by using the first call the measure the necessary length, then allocating the necessary space dynamically, and then doing the second call using the dynamically allocated space.

Several places, you have things like:

return ( fclose( file ) == 0 ) ? true : false;

These should be written as:

return fclose(file) == 0;

or just:

return !fclose(file);

There's no need to select and return a Boolean literal -- the Boolean values yielded by == and ! will work nicely as-is.

Edit: You also have a couple of things like:

#ifdef WIN32
 *curPos = _ftelli64( file );
#else
 *curPos = ftello64( file );
#endif 

spread throughout the code. Where you're just dealing with different function names like this, I think I'd have one block up-front to deal with the name differences, and then use a single name through the rest of the code:

#ifdef WIN32
 #define ftello64(x) _ftelli64(x)
 #define fseeko64(x) _fseeki64(x)
#endif
/* ... */
Bool_T OsFile_Seek( IN FILE * file, IN Int64_T offset, IN int origin )
{
 return !fseeko64(file, offset, origin);
}

You might want to read #ifdef considered harmful for more about what to do and what to avoid.

Answer 2

Consider replacing a code structure like this:

if (predicate)
{
 // many lines of code here
 if (predicate2)
 {
 // even more lines of code here
 }
 else
 {
 return false;
 }
}
else
{
 return false;
}

to the much simpler:

if (!predicate)
 return false;
// long code here
if (!predicate2)
 return false
// second long paragraph 

• akmal: Prior to reading Code Complete, I structured my code as you showed. When you structured code like you showed, it hard to get the main idea of code, when there is a lot of 'if' conditions. In my opinion it is more important to make focus in the operation which must be executed when program executed normally, instead of focusing on exceptional situations.
• Boris: It is definitely not clear that your 'if' is an exit condition. The simple 'return false' is so far below, to the user it is not evident what the two outcomes of the ifs are. while in my version you do.
• akmal: In this case I agree with you, but in situations, when prior to exit, you must do some operations(deallocate memory, ...), it becomes complicated, because when you're reading the code you concentrated on things, which are not important as the main idea of the code, IMHO.