By Reg Charney
I recently needed to extract a code sample from my MSDN subscription. It is shown here with only minor formatting change, e.g., removing some spaces and changing names.
Assuming that all the constants were defined correctly and ignoring formatting, can you detected a dozen common errors? Do you see any others?
| BOOL
ProcessTheFile(HWND hWnd) { HANDLE hFile; DWORD dwBytesRead, dwFileSize; OpenFileName Ofn; TCHAR szFile[MAX_PATH] = "\0"; char *lpBuf; if (GetOpenFileName(&Ofn)) { if ((hFile=CreateFile(Ofn.lpstrFile))==(HANDLE)-1) { MessageBox(hWnd, "File open failed." ); return FALSE; } dwFileSize = GetFileSize(hFile); if (dwFileSize == 0xFFFFFFFF) // invalid size { MessageBox( NULL, "GetFileSize failed!"); return FALSE; } lpBuf=(char*)GlobalAlloc(GMEM_FIXED,dwFileSize); if (lpBuf == NULL) { MessageBox(NULL, "GlobalAlloc failed!"); CloseHandle( hFile ); return FALSE; } ReadFile(hFile,lpBuf, dwFileSize, &dwBytesRead); if (dwBytesRead == 0) { MessageBox(hWnd, "Zero bytes read." ); return FALSE; } DoSomething(lpBuf) CloseHandle(hFile); return TRUE; } else return FALSE; } Listing #1 - Problem Code |
The errors that I detected were:
- If an invalid file size is detected, the file is not closed.
- If zero bytes are read, the file is not closed.
- If the size of the file is zero bytes, an attempt is made to allocate zero bytes.
- If zero bytes are read, GlobalAlloc memory is not freed.
- At the end of the routine, GlobalAlloc memory is not freed.
- No check is made for an error after reading the file. Only the file size is checked .
- Inconsistent use of first argument in MessageBox.
- The function has multiple return points.
- The outer most else is not needed.
- Reversing the sense of the first if would let me eliminate one level of nesting.
- While the file name could be in Unicode, no allowance was made for the contents of ReadFile being Unicode. The type of lpBuf should have been TCHAR*.
- The size of szFile did not allow for file names of exactly MAX_PATH length. It should have been MAX_PATH+1.
I rewrote this example to correct for these errors. Can you see any other errors in this modified example.
| BOOL
ProcessTheFile(void) { HANDLE hFile = (HANDLE)-1; DWORD dwBytesRead, dwFileSize; OpenFileName Ofn; TCHAR szFile[MAX_PATH+1] = "\0"; TCHAR *lpBuf = NULL; BOOL bRet = FALSE; if (! GetOpenFileName(&Ofn)) goto Exit; if ((hFile=CreateFile(Ofn.lpstrFile)) == (HANDLE)-1) { MessageBox(NULL, "File open failed."); goto Exit; } dwFileSize = GetFileSize(hFile); if (dwFileSize == 0xFFFFFFFF) // invalid size { MessageBox(NULL, "GetFileSize failed!"); goto Exit; } if (dwFileSize == 0) goto Exit; // exit if nothing to read lpBuf = (TCHAR *)GlobalAlloc(GMEM_FIXED,dwFileSize); if (lpBuf == NULL) { MessageBox(NULL, "GlobalAlloc failed!"); goto Exit; } if (! ReadFile(hFile,lpBuf,dwFileSize,&dwBytesRead)) { MessageBox(NULL, "Error reading file."); goto Exit; } if (dwBytesRead == 0) { MessageBox(NULL, "Zero bytes read."); goto Exit; } if (DoSomething(lpBuf)) bRet = TRUE; Exit: if (lpBuf) GlobalFree(lpBuf); if (hFile != (HANDLE)-1) CloseHandle(hFile); return bRet; } Listing #2 - Corrected Code |
If I were writing this code from scratch, using OO techniques would simplify the code and reduce the chances of errors.
By Reg. Charney
European Observations
I just came back from the U.K. and found a number of surprises in the IT arena.
They are much more prevalent there than here. They are also usually smaller and lighter. While the total cost for phone service in Europe is generally more expensive than here in the U.S., mobile phone payment plans are much more aggressive there. For example, most plans do not charge for incoming calls. Costs of some major plans for international calls are 7.5¢ to the U.S. from the U.K. Calling anywhere in Europe from the U.K. costs 15¢. There is a least one plan that offers calls on a flat 15¢ per minute basis, with no monthly or yearly contract needed. Another plan offered a free mountain bike when you signed up for a year.
There are also a number of dual mode phones available that work both in Europe and the U.S. Prices quoted ranged from about $450 to $900 depending on plan.
The quality of European TV pictures have always been better than here because of their greater screen line density. Now, they have gone the next step and have been selling keystone TVs for over a year which match the keystone pictures you sometimes see on the better TV channels when they show panoramic films.
While the Internet is ubiquitous in the U.K. getting access to it for a traveler is still a major undertaking. At the two four star hotels we stayed at, there was no telephone in the suite and only one for all the hotel guests. Thus, getting on the Net was out of the question.
In 1899, London traffic traveled at 11 mph. In 1999, London traffic traveled at 11 mph.
By Reg. Charney
Good housekeeping is a sign of a good program. Unfortunately, in the rush to produce results in an ever more complex application, cleaning up after yourself gets more and more difficult and more and more essential. This is where the simple concept of constructors and destructors can be a life saver. In the Windows systems, there are many error codes with corresponding error messages. There is a complex function, FormatMessage(…), that returns the text that corresponds to a given error code. In part, it does this by passing back a pointer to an internal buffer containing the text of the message. You must remember to free this buffer after you have done with it. If you are like me, you sometimes forget. This little class, EM, uses its constructor and destructor to do all that while simplifying the error message interface. It also defines some intuitive conversion operators to extract the needed code and text. I expect that there may be some comments on this latter technique, but it does reduce namespace pollution.
If there is no corresponding error message text for a given error code, the text pointer is set to NULL. This can be tested and acted on accordingly.
The first part of the test program loops through the first 10 possible error codes and supplies them to the EM instance. The last part of the test program tries to create an invalid directory and lets the system set the error code which is then captured and reported on.
This class is not thread-safe because FormatMessage is not thread safe. Any ideas on how to make this thread-safe?
|
#include "stdafx.h" #include <afx.h> #include <iostream.h> // +++ class definition class ElapsedTimer { DWORD dwStarted; // type is unsigned long public: inline void Start() { dwStarted = GetTickCount(); } inline DWORD End() { return ( GetTickCount() - dwStarted ); } inline operator DWORD() { return End(); } ElapsedTimer() { Start(); } }; // +++ output program +++ // +++ test program +++ int main() // === time copying a file. === // === time copying file back again === return 0; Listing #3 - Elapsed Timer |
By Reg. Charney
As you are probably aware, growth in the number of job openings has slowed. Here in the Valley, recent reports tell of layoffs ranging from 5,000+ to 15,000+. The layoffs are a logical consequence of the changing demand. First, there is a drop off in the number of jobs posted, then there is a hiring freeze and then there are layoffs. It has taken five months, from a high of job openings in April of this year to now, for layoffs to have occurred.
Not all is gloomy. As Chart #1 shows, there has been an upturn in Software Engineering job openings both here and nationwide. Also, the rate of shrinkage of job openings generally is slowing.
Chart #1 is normalized for the number of jobs in January of this year. That is, the number of openings in January 2000 has been taken as 1.00.
For the first time since we started tracking technology trends, demand for non-internet related technologies has outstripped demand for internet-related skills. Chart #2 shows a general decline in demand for Internet related skills. This is consistent with the slow down in the growth of the dot.coms.
While not shown,
the only other noteworthy positive growth has been for Linux
platform skills. In fact, over the last sixteen months, demand for
Linux skills has outstripped all other platforms.