Microsoft Sample Code

Here's some CeeStyleCeePlusPlus code from everyone's favorite software shop:


        BOOL CreateURLShortcut(LPWSTR pszURL, LPWSTR pszShortcutFile) {

        WCHAR pszShortcutContents[1024];


        HANDLE hf = CreateFileW(pszShortcutFile, GENERIC_READ | GENERIC_WRITE, (DWORD) 0, NULL, CREATE_ALWAYS,

        FILE_ATTRIBUTE_NORMAL, (HANDLE) NULL); 

        if(hf == INVALID_HANDLE_VALUE) return FALSE;

        int iCharCount = swprintf(pszShortcutContents, SHORTCUT_TEMPLATE, pszURL) + 1;

        DWORD dwWritten = 0;

        if(!WriteFile(hf, pszShortcutContents, sizeof(WCHAR)*iCharCount, &dwWritten, NULL)) return FALSE;

        CloseHandle(hf);


        return TRUE;

        }

That one little sample contains...

a potential array bounds overflow
- No it doesn't. I believe that swprintf is a typo for wsprintf, which has a hardcoded maximum buffer size of 1024 elements.
  - I've never heard of such a limit. But regardless, there is an swprintf(), as verified by a few seconds of Googling. It expects SHORTCUT_TEMPLATE to be a length parameter, though, which in turn suggests that our buffer size should be consistent (e.g. SHORTCUT_TEMPLATE+1). Also, we're screwed if the 'pszURL' happens to contain any % signs.
a potential leak of a file handle
HungarianNotation
the usual discarded error values
gratuitous typecasts using C-style casts, at that
- Where are all the typecasts? I see only two. Two hardly counts as gratuitous. Moreover, all casts are for numeric constants, where the compiler won't likely know how to properly cast them anyway without the casts. I think you're just a wee bit overreacting to this code.
a #define used as a constant, SHORTCUT_TEMPLATE, defined far from its only use
- The Win32 API is coded for C, not C++, so it makes sense that they utilize C-style API usage patterns. This is one of them. (SamuelFalvo)

Oh, and the program around it used VoidMain. But at least it doesn't lead us astray by writing a small wide text file robustly using the non-MS-controlled std::wofstream!!

CharlesPetzold celebrates the 20th anniversary of his cash cow, MicrosoftWindows, by ranting on this subject here:

http://charlespetzold.com/etc/DoesVisualStudioRotTheMind.html

Things could have looked like this or similar:


        bool CreateURLShortcut(const std::wstring& url, const std::wstring& shortcutFile)

        {

        using namespace std;

        wofstream of( shortcutFile, ios::out | ios::trunc );

        if( of )

        {

        return ( of << url );

        }

        return false;

        }

I suppose this body would work too, but little terse for my liking:


        ...

        {

        using namespace std;

        wofstream of( shortcutFile, ios::out | ios::trunc );

        return (of << url);

        }

Not too terse for me. My usual set of ReFactorings would converge on that anyway (first by introducing a guard clause, then by merging the two conditionals with &&, then by observing that the test is redundant). But anyway, I read the last statement as "return whether we could send url to of", so the only part of it that's too terse for me is the stream name. I might even do it all in one line with a temporary:


        return wofstream(shortcutFilename, ios::out | ios::trunc) << url;

"return whether we could send url to a wofstream constructed using shortcutFilename with out and trunc flags". Makes perfect sense to me. -- KarlKnechtel

CategoryCoding