what is a better style?

[Nobody]

Lets say I have a class that is only available if a specific DLL (a.dll) is
present. I can't link to that DLL through lib files or my app will fail on
any machine that doesn't have a.dll.

So I do LoadLibrary()'s and keep function pointers in my wrapper class...
Which is a better style?

DWORD CClass::SomeFunc()
{
if (m_pfn != NULL)
return (*m_pfn)(parm1, parm2);

return (DWORD) -1;
}

or

DWORD CClass::SomeFunc()
{
ASSERT(m_pfn != NULL);
return (*m_pfn)(parm1, parm2);
}

I guess the first version fails "more graciously" if the developer calls it
incorrectly, but the 2nd version isn't returning "made up" return codes and
is more "in your face" during development.

 

[Jonathan Mcdougall]

Lets say I have a class that is only available if a specific DLL (a.dll) is
present. I can't link to that DLL through lib files or my app will fail on
any machine that doesn't have a.dll.

So I do LoadLibrary()'s and keep function pointers in my wrapper class...
Which is a better style?

DWORD CClass::SomeFunc()
{
if (m_pfn != NULL)
return (*m_pfn)(parm1, parm2);

return (DWORD) -1;
}

or

DWORD CClass::SomeFunc()
{
ASSERT(m_pfn != NULL);
return (*m_pfn)(parm1, parm2);
}

I guess the first version fails "more graciously" if the developer calls it
incorrectly, but the 2nd version isn't returning "made up" return codes and
is more "in your face" during development.

Assertions are used during development to catch "that's not supposed to
happen" errors, such as bad arguments to a function. They disappear
when the program is compiled in "release" mode, so they are of no use
to convey information to users.

Returning error codes (or sometimes throwing exceptions) are common
ways to signal a "valid" error in a program. It is then possible to
display a user-friendly message, recover or fail.

As you can see, both ways are not necessarily exclusives. Actually, it
is common practice to use both assertions and error codes in the same
function. If it is normal (or predictable) for an error to happen, you
should not use (only) assertions to catch them, since you don't want a
user to see them.

Remember to be both user-friendly (try to recover or display an
informative message and fail graciously) and developper-friendly
(convey as much informations you can, such as with compile-time or
run-time assertions).


Jonathan

 

[Ivan Vecerina]

: Lets say I have a class that is only available if a specific DLL
(a.dll) is
: present. I can't link to that DLL through lib files or my app will
fail on
: any machine that doesn't have a.dll.
:
: So I do LoadLibrary()'s and keep function pointers in my wrapper
class...
: Which is a better style?
:
: DWORD CClass::SomeFunc()
: {
: if (m_pfn != NULL)
: return (*m_pfn)(parm1, parm2);
:
: return (DWORD) -1;
: }
This function is ok if a value such as (DWORD)-1 makes sense
and can be interpreted as an error condition by the caller.
But as you mentioned, such an "invalid" result value may not
always exist -- so a separate way to report errors may be needed.

And this is exactly what exceptions are for.
I would actually write this function as:
DWORD CClass::SomeFunc()
{
if( m_pfn == NULL )
throwLibraryNotLoadedError(); // calls throw(..smthg..)
return (*m_pfn)(parm1, parm2);
}

: or
:
: DWORD CClass::SomeFunc()
: {
: ASSERT(m_pfn != NULL);
: return (*m_pfn)(parm1, parm2);
: }

So what happens in release mode, assuming that ASSERT checks
are then disabled, if that function is called when m_pfn==0 ?
You will be getting undefined behavior (hopefully just a
crash of your program).
Can this ever be a good idea ?

: I guess the first version fails "more graciously" if the developer
calls it
: incorrectly, but the 2nd version isn't returning "made up" return
codes and
: is more "in your face" during development.

My approach is to throw an exception if one of the preconditions
of the function is violated (for instance, if no DLL was loaded).
To help with debugging, I may also want to trigger an assertion
failure, breakpoint or warning at debug time (i.e. within the function
throwLibraryNotLoadedError() ).

Ivan

 

[Greg]

Lets say I have a class that is only available if a specific DLL (a.dll) is
present. I can't link to that DLL through lib files or my app will fail on
any machine that doesn't have a.dll.

So I do LoadLibrary()'s and keep function pointers in my wrapper class...
Which is a better style?

DWORD CClass::SomeFunc()
{
if (m_pfn != NULL)
return (*m_pfn)(parm1, parm2);

return (DWORD) -1;
}

or

DWORD CClass::SomeFunc()
{
ASSERT(m_pfn != NULL);
return (*m_pfn)(parm1, parm2);
}

I guess the first version fails "more graciously" if the developer calls it
incorrectly, but the 2nd version isn't returning "made up" return codes and
is more "in your face" during development.

The difference between these two versions is more than an issue of
style. Each is making a very different statement about the program.
Either one may be correct, but not both.

The first version states that m_pfn could be NULL while this program is
executing; so it is necessary for the program to check for that
possibility and report it to the caller.

The second version states that the program is certain that m_pfn will
never be NULL while it is executing. The program's behavior is
therefore defined only when m_pfn is not NULL. Should that assumption
ever turn out not to be true, then the program's behavior will be
undefined. Ideally the program would crash quickly without causing any
harm.

Greg

 

[Mirek Fidler]

Lets say I have a class that is only available if a specific DLL (a.dll) is
present. I can't link to that DLL through lib files or my app will fail on
any machine that doesn't have a.dll.

So I do LoadLibrary()'s and keep function pointers in my wrapper class...
Which is a better style?

DWORD CClass::SomeFunc()
{
if (m_pfn != NULL)
return (*m_pfn)(parm1, parm2);

return (DWORD) -1;
}

or

DWORD CClass::SomeFunc()
{
ASSERT(m_pfn != NULL);
return (*m_pfn)(parm1, parm2);
}

I guess the first version fails "more graciously" if the developer calls it
incorrectly, but the 2nd version isn't returning "made up" return codes and
is more "in your face" during development.

I guess later is better - because not every function returns DWORD that
can be interpreted. Your code has to verify the presence of .dll abefore
calling methods anyway, so you are just duplicating this there.

BTW, in our system we have introduced nice related tool to automatize
generating such wrapper objects. To create such object, we provide
".dli" file with content like (actual example is for Lotus Notes client
..dll)

FN(WORD, OSLoadString, (HMODULE hModule, STATUS StringCode, char
*retBuffer, WORD BufferLength))
FN(WORD, OSTranslate, (WORD TranslateMode, char far *In, WORD
InLength, char far *Out, WORD OutLength))
FN(STATUS, NotesInitExtended, (int argc, char **argv))
FN(STATUS, OSPathNetConstruct, (char *PortName, char *ServerName, char
far *FileName, char *retPathName))
FN(STATUS, NSFDbOpen, (char far *PathName, DBHANDLE far *rethDB))
FN(STATUS, NSFDbClose, (DBHANDLE hDB))
............

- basically, this is somewhat "reparsed" header file for .dll. Then place

#define DLLFILENAME "nnotes.dll"
#define DLIMODULE NOTES
#define DLIHEADER <notes/notes.dli>
#define DLLCALL LNPUBLIC
#include <Core/dli_header.h>

to common header file and

#define DLLFILENAME "nnotes.dll"
#define DLIMODULE NOTES
#define DLIHEADER <notes/notes.dli>
#define DLLCALL LNPUBLIC
#include <Core/dli_header.h>

to some .cpp file.

This creates global function NOTES() returning object instance that has
all .dll functions described in .dli file defined as its methods.
Moreover, it has operator bool that can be used to test whether .dll is
present:

char h[256];
if(NOTES())
NOTES().OSLoadString(GetModuleHandle(NULL), ERR(nError), h, 255);


(http://upp.sf.net)

Mirek