MicrosoftSampleCode

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