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