寫好代碼的十個(gè)秘訣.ppt

1、1,10 Things You Can Do To Write Better Code寫好代碼的十個(gè)秘訣,林斌 Development Manager Microsoft Research, China,2,一流代碼的特性,魯棒 - Solid and Robust Code 簡潔 - Maintainable and Simple Code 高效 - Fast Code 簡短 - Small Code 共享 - Re-usable Code 可測試 - Testable Code 可移植 - Portable Code,3,Why is this code bad?,void MyGirlF

2、riendFunc(CORP_DATA InputRec, int CrntQtr, EMP_DATA EmpRec, float EstimRevenue, float YTDRevenue, int ScreenX, int ScreenY, COLOR_TYPE newColor, COLOR_TYPE PrevColor, STATUS_TYPE status, int ExpenseType) int i; for (i=1; i100; i+) InputRec.revenuei = 0; for (i=1; i100; i+) InputRec.expensei= CorpExp

3、ensei; UpdateCorpDatabase(EmpRec); EstimRevenue =YTDRevenue*4.0/(float)CrntQtr; NewColor = PreColor; Status = Success; if (ExpenseType= 1) for (i=1;i12;i+) Profiti = Revenuei-Expense.Type1i; else if (ExpenseType = 2) Profiti = Revenuei - Expense.Type2i; else if (ExpenseType=3) Profiti =Revenuei -Exp

4、ense.Type3i; ,4,Why is this code bad?,void MyGirlFriendFunc(CORP_DATA InputRec, int CrntQtr, EMP_DATA EmpRec, float EstimRevenue, float YTDRevenue, int ScreenX, int ScreenY, COLOR_TYPE newColor, COLOR_TYPE PrevColor, STATUS_TYPE status, int ExpenseType) int i; for (i=1; i100; i+) InputRec.revenuei =

5、 0; for (i=1; i100; i+) InputRec.expensei= CorpExpensei; UpdateCorpDatabase(EmpRec); EstimRevenue =YTDRevenue*4.0/(float)CrntQtr; NewColor = PreColor; Status = Success; if (ExpenseType= 1) for (i=1;i12;i+) Profiti = Revenuei-Expense.Type1i; else if (ExpenseType = 2) Profiti = Revenuei - Expense.Type

6、2i; else if (ExpenseType=3) Profiti =Revenuei -Expense.Type3i; ,5,Because,Bad function name Maintainability Crash if CrntQtr equals 0 Robustness No comments Maintainability Unnecessary for loop Performance The function has no single purpose Reusability Bad layout Simplicity Limit the length of a sin

7、gle function,10,Spot the defect,DWORD Status=NO_ERROR; LPWSTR ptr; Status = f( ., ,11,Spot the defect,DWORD Status=NO_ERROR; LPWSTR ptr; Status = f( ., is? isnot? or? new C/C+ keywords? No. overloaded operators? No. just some confusing macros ,12,A better version,DWORD Status=NO_ERROR; LPWSTR ptr =

8、NULL; Status = f( ., Make your intent explicit Use existing language constructs Everybody understands them Avoid future problems: Initialize parenthesize,13,Whats the maximum length of a function?,Look at this example Watch out for functions 200 lines! Study in 1986 on IBMs OS/360: the most error-pr

9、one routines = longer than 500 lines. Study in 1991 on 148,000-line of code: functions 2.4 times less expensive to fix than longer functions.,Over 1400 lines!,14,Look at these codes,Lets look at these codes from our own projects: Small functions: Larger functions: Then look at those from Windows 200

10、0 source tree: NT Kernel Mode Code: NT User Mode CPP File: NT User Mode Header File:,15,2.取個(gè)好名字 - Use Naming Conventions,Hungarian Notation Prefix-BaseTag-Name Standard Prefix: p A pointer. CHAR* psz; rg An array. DWORD rgType; c A count of items. DWORD cBook; h A handle. HANDLE hFile; Standard “Bas

11、eTag” void - v int - i BOOL - f UINT - ui BYTE - b CHAR - ch WCHAR - wch ULONG - ul LONG - l DWORD - dw HRESULT - hr fn - function sz - NULL str USHORT, SHORT, WORD - w C+ member variables start with: m_ Global variables start with: g_,16,Lets try these,A flag indicates whether a C+ object has been

12、initialized: BOOL m_fInitialized; A Session ID: DWORD dwSessionID; A ref count in a C+ object: LONG m_cRef; / skip the l A Pointer to BYTE buffer: BYTE* pbBuffer; A global logfile filename buffer: CHAR g_szLogFileMAX_PATH; A pointer to a global logfile: CHAR* g_pszLogFile;,17,3.凌波微步, 未必摔跤 - Evil got

13、os? Maybe Not,Why goto is bad? Creates unreadable code Causes compiler to generate non-optimal code Why goto is good? Cleaner code Single entry, single exit Less duplicate code,18,Spot the gotos,START_LOOP: If (fStatusOk) if (fDataAvaiable) i = 10; goto MID_LOOP; else goto END_LOOP; else for (I = 0;

14、 I 100; I+) MID_LOOP: / lots of code here goto START_LOOP; END_LOOP:,19,BAD gotos!,START_LOOP: If (fStatusOk) if (fDataAvaiable) i = 10; goto MID_LOOP; else goto END_LOOP; else for (I = 0; I 100; I+) MID_LOOP: / lots of code here goto START_LOOP; END_LOOP:,BAD!,20,How about this one?,pszHisName = (C

15、HAR*)malloc(256); if (pszHisName = NULL) free(pszMyName); free(pszHerName); return hr; free(pszMyName); free(pszHerName); free(pszHisName); return hr; ,HRESULT Init() pszMyName = (CHAR*)malloc(256); if (pszMyName = NULL) return hr; pszHerName = (CHAR*)malloc(256); if (pszHerName = NULL) free(pszMyNa

16、me); return hr; ,21,Duplicate code,pszHisName = (CHAR*)malloc(256); if (pszHisName = NULL) free(pszMyName); free(pszHerName); return hr; free(pszMyName); free(pszHerName); free(pszHisName); return hr; ,HRESULT Init() pszMyName = (CHAR*)malloc(256); if (pszMyName = NULL) return hr; pszHerName = (CHAR

17、*)malloc(256); if (pszHerName = NULL) free(pszMyName); return hr; ,22,Harder to maintain,pszHisName = (CHAR*)malloc(256); if (pszHisName = NULL) free(pszMyName); free(pszHerName); free(pszItsName); return hr; free(pszMyName); free(pszHerName); free(pszHisName); free(pszItsName); return hr; ,HRESULT

18、Init() pszMyName = (CHAR*)malloc(256); if (pszMyName = NULL) return hr; pszHerName = (CHAR*)malloc(256); if (pszHerName = NULL) free(pszMyName); return hr; pszItsName = (CHAR*)malloc(256); if (pszItsName = NULL) free(pszMyName); free(pszHerName); return hr; ,23,How about this one?, Exit: if (pszMyNa

19、me) free(pszMyName); if (pszHeName) free(pszHeName); if (pszHiName) free(pszHiName); return hr; ,HRESULT Init() pszMyName =(CHAR*)malloc(); if (pszMyName = NULL) goto Exit; pszHeName =(CHAR*)malloc(); if (pszHeName = NULL) goto Exit; pszHiName =(CHAR*)malloc(); if (pszHiName = NULL) goto Exit;,24,Si

20、ngle entry, single exit. No duplicate code, Exit: if (pszMyName) free(pszMyName); if (pszHeName) free(pszHeName); if (pszHiName) free(pszHiName); return hr; ,HRESULT Init() pszMyName =(CHAR*)malloc(); if (pszMyName = NULL) goto Exit; pszHeName =(CHAR*)malloc(); if (pszHeName = NULL) goto Exit; pszHi

21、Name =(CHAR*)malloc(); if (pszHiName = NULL) goto Exit;,25,Easy to maintain, Exit: if (pszMyName) free(pszMyName); if (pszHeName) free(pszHeName); if (pszItName) free(pszItName); if (pszHiName) free(pszHiName); return hr; ,HRESULT Init() pszMyName =(CHAR*)malloc(); if (pszMyName = NULL) goto Exit; p

22、szHeName =(CHAR*)malloc(); if (pszHeName = NULL) goto Exit; pszItName =(CHAR*)malloc(); if (pszItName = NULL) goto Exit; pszHiName =(CHAR*)malloc(); if (pszHiName = NULL) goto Exit;,26,Use the following goto guidelines,Single entry, single exit? use goto Dont use more than one goto labels Use gotos

23、that go forward, not backward Make sure a goto doesnt create unreachable code,27,4. 先發(fā)制人, 后發(fā)制于人 - Practice Defensive Coding,Initialize variables Check return values Keep it simples Usually, simplicity = performance Keep it clean Multi-thread clean Minimize casts Avoid miscalculations Divide by Zero

24、Signed vs. Unsigned,28,Spot the defect,HRESULT hr; LPSTR str = (LPSTR)LocalAlloc(LPTR, size); if (str) . / process appropriately hr = S_OK; return hr;,29,Spot the defect,HRESULT hr; LPSTR str = (LPSTR)LocalAlloc(LPTR, size); if (str) . / process appropriately hr = S_OK; return hr; Returning a random

25、 status on failure,30,A better version?,HRESULT hr = S_OK; LPSTR str = (LPSTR)LocalAlloc(LPTR, size); if (str) . / process appropriately hr = S_OK; return hr;,31,Probably not,HRESULT hr = S_OK; LPSTR str = (LPSTR)LocalAlloc(LPTR, size); if (str) . / process appropriately hr = S_OK; return hr; Return

26、 success if nothing happened? Think before you fix!,32,A better version!,HRESULT hr = S_OK; LPSTR str = (LPSTR)LocalAlloc(LPTR, size); if (str) . / process appropriately hr = S_OK; else hr = E_OUTOFMEMORY; return hr;,33,Spot the detect,BOOL SomeProblem(USHORT x) while (-x = 0) return TRUE; ,34,Spot

27、the detect,BOOL SomeProblem(USHORT x) while (-x = 0) return TRUE; Infinite loop! Unsigned never negative!,35,Spot the detect,wcscpy(wcServerIpAdd, WinsAnsiToUnicode(cAddr,NULL); Note: WinsAnsiToUnicode is a private API which allocates a new UNICODE string given an ANSI string.,36,Spot the detect,wcs

28、cpy(wcServerIpAdd, WinsAnsiToUnicode(cAddr,NULL); WinsAnsiToUnicode can return NULL which will cause a program crash WinsAnsiToUnicode can return allocated memory which will cause a leak,37,A better version,LPWSTR tmp; tmp = WinsAnsiToUnicode(cAddr,NULL); if (tmp != NULL) wcscpy(wcServerIpAdd,tmp);

29、WinsFreeMemory(PVOID)tmp); else return(ERROR_NOT_ENOUGH_MEMORY); ,38,5. 見招拆招, 滴水不漏 - Handle The Error Cases: They Will Occur!,Common examples: Out of memory Exceptions being thrown Registry keys missing Networks going down This is the hardest code to test so it better be right! Dont fail in C+ objec

30、t constructor, you cant detect it!,39,Error cases (continued),In an error case, the code should Recover gracefully Get to a consistent state Clean up any resources it has allocated Let its caller know Interface should define and document the behavior (return status, throw exception) Code should adhe

31、re to that definition What not to do Ignore the error Crash Exit,40,Spot the detect,CWInfFile:CWInfFile() m_plLines = new TPtrList(); / . m_plSections = new TPtrList(); / . m_ReadContext.posLine = m_plLines-end(); . . . ,41,Spot the detect,CWInfFile:CWInfFile() m_plLines = new TPtrList(); / . m_plSe

32、ctions = new TPtrList(); / . m_ReadContext.posLine = m_plLines-end(); . . . MFCs operator new throws an exception causing a leak when out of memory,42,A better version?,CWInfFile:CWInfFile() try m_plLines = new TPtrList(); / . m_plSections = new TPtrList(); / . m_ReadContext.posLine = m_plLines-end(

33、); . . . catch ( . . . ) if (m_plLines) delete m_plLines; if (m_plSections) delete m_plSections; ,43,No!,CWInfFile:CWInfFile() try m_plLines = new TPtrList(); / . m_plSections = new TPtrList(); / . m_ReadContext.posLine = m_plLines-end(); . . . catch ( . . . ) if (m_plLines) delete m_plLines; if (m_

34、plSections) delete m_plSections; ,Values may be uninitialized Think before you fix!,44,A better version,CWInfFile:CWInfFile() : m_plLines(NULL), m_plSections(NULL) try m_plLines = new TPtrList(); / . m_plSections = new TPtrList(); / . m_ReadContext.posLine = m_plLines-end(); . . . catch ( . . . ) if

35、 (m_plLines) delete m_plLines; if (m_plSections) delete m_plSections; ,45,But wait, there is more,CWInfFile:CWInfFile() : m_plLines(NULL), m_plSections(NULL) try m_plLines = new TPtrList(); / . m_plSections = new TPtrList(); / . m_ReadContext.posLine = m_plLines-end(); . . . catch ( . . . ) if (m_pl

36、Lines) delete m_plLines; if (m_plSections) delete m_plSections; ,When not use MFC, operator new may return NULL,46,Spot the defect,Class foo private: CHAR* m_pszName; DWORD m_cbName; public: foo(CHAR* pszName); CHAR* GetName() return m_pszName; ;,foo:foo(CHAR* pszName) m_pszName = (BYTE*) malloc(NAM

37、E_LEN); if (m_pszName = NULL) return; strcpy(m_pszName, pszName); m_cbName = strlen(pszName); ,47,Spot the defect,Class foo private: CHAR* m_pszName; DWORD m_cbName; public: foo(CHAR* pszName); CHAR* GetName() return m_pszName; ;,foo:foo(CHAR* pszName) m_pszName = (BYTE*) malloc(NAME_LEN); if (m_psz

38、Name = NULL) return; strcpy(m_pszName, pszName); m_cbName = strlen(pszName); ,If malloc() fails, this code will crash: foo* pfoo = new foo(“MyName”); if (pfoo) CHAR c = *(pfoo-GetName(); ,48,A better version,Class foo private: CHAR* m_pszName; DWORD m_cbName; public: foo(); HRESULT Init(CHAR* pszNam

39、e); ; foo:foo() m_cbName = 0; m_pszName = NULL; ,HRESULT foo:Init(CHAR* pszName) HRESULT hr = S_OK; if (pszName) m_cbName = lstrlen(pszName); m_pszName = (CHAR*)malloc(m_cbName+1); if (m_pszName = NULL) hr = E_OUTOFMEMORY; return hr; strcpy(m_pszName, pszName); else hr = E_INVALIDARG; return hr; ,49

40、,6. 熟習(xí)劍法刀術(shù), 所向無敵 - Learn Win32 API Seriously,Learn Win32 APIs, from MSDN Why Win32 APIs? Well designed Well tested Easy to understand Improve performance dramatically Understand the ones you use, read the SDK doc in MSDN Pick the best one based on your need,50,Spot the defect,for( i=0; i 256; i+) re

41、i = 0; imi = 0; for( k = 0; k 128; k+) AvrSpeck = 0; #define FrameLen 200 for(k=0; k5*40*FrameLen; k+) lspk = lspk + 40*FrameLen; for(k = 0; k FrameLen; k+) audiok = vectk; ,51,Better version,for( i=0; i 256; i+) rei = 0; imi = 0; for( k = 0; k 128; k+) AvrSpeck = 0; #define FrameLen 200 for(k=0; k5

42、*40*FrameLen; k+) lspk = lspk + 40*FrameLen; for(k = 0; k FrameLen; k+) audiok = vectk; ,52,Spot the defect,TCHAR WindowsDirMAX_PATH; TCHAR DriveLetter; GetWindowsDirectory(WindowsDir, MAX_PATH); DriveLetter = WindowsDir0; .,53,Spot the defect,TCHAR WindowsDirMAX_PATH; TCHAR DriveLetter; GetWindowsD

43、irectory(WindowsDir, MAX_PATH); DriveLetter = WindowsDir0; . Missing return value check Operating on a random drive if GetWindowsDirectory() fails,54,Can GetWindowsDirectory fail?,MSDN has always said “yes” Implementation has always said “no” Until Terminal Server! Computing GetWindowsDirectory may

44、allocate memory If allocation fails, the call returns FALSE Result: potential catastrophic errors on Terminal Server,55,A better version,TCHAR WindowsDirMAX_PATH; TCHAR DriveLetter; if (!GetWindowsDirectory(WindowsDir, MAX_PATH) return E_OUTOFMEMORY; / proper recovery DriveLetter = WindowsDir0; .,56

45、,7. 雙手互搏, 無堅(jiān)不摧 - Test, but dont stop there,Step through in the debugger Especially the error code paths Test as much of your code as you can If you havent tested it, it probably doesnt work If you dont know how much youve tested, its less than you expect So find out! Code review Have somebody else r

46、ead it And read somebody elses code!,57,Spot the defect,HRESULThr=NOERROR; /Makesuretheargumentspassedarevalid if(NULL!=m_paConnections) . / do the right thing else hr=S_FALSE; if (SUCCEEDED(hr) if(NULL!=m_paConnections0.pUnk) . . .,58,Spot the defect,HRESULThr=NOERROR; /Makesuretheargumentspassedar

47、evalid if(NULL!=m_paConnections) . / do the right thing else hr=S_FALSE; if (SUCCEEDED(hr) if(NULL!=m_paConnections0.pUnk) . . . SUCCEEDED(S_FALSE) = TRUE Crash if m_paConnections = NULL Easily simulatable in the debugger,59,Better version,HRESULThr=NOERROR; /Makesuretheargumentspassedarevalid if(NU

48、LL!=m_paConnections) . / do the right thing else hr=E_INVALIDARG; if (SUCCEEDED(hr) if(NULL!=m_paConnections0.pUnk) . . .,60,Spot the defect,/ / Get file size first / DWORD dwFileSize = GetFileSize( hFile, NULL ); if ( dwFileSize = -1 ) / what can we do ? keep silent ErrorTrace(0, GetFileSize failed

49、 with %d, GetLastError(); return; Note: GetFileSize returns 1 if it fails.,61,Spot the defect,/ / Get file size first / DWORD dwFileSize = GetFileSize( hFile, NULL ); if ( dwFileSize = -1 ) / what can we do ? keep silent ErrorTrace(0, GetFileSize failed with %d, GetLastError(); return; Note: GetFile

50、Size returns 1 if it fails.,Always return as errors occur without proceeding,62,Better version,/ / Get file size first / DWORD dwFileSize = GetFileSize( hFile, NULL ); if ( -1 = dwFileSize ) / what can we do ? keep silent ErrorTrace(0, GetFileSize failed with %d, GetLastError(); return; Note: GetFil

51、eSize returns 1 if it fails.,Compiler will catch the error if you miss “=“,63,8. 活用段言 - Use, dont abuse, Assertions,Use Assertions because Useful for documentation Describe assumptions Describe “impossible” situations Useful for debugging Help diagnose problems May detect problems earlier Dont abuse

52、 Assertions because Assertions are usually no-ops in a free/release build Dont use Assertions in situations that can occur in practice,64,Spot the defect,CHAR * pch; pch = GlobalAlloc(10); ASSERT(pch != NULL); pch0 = 0;,65,Spot the defect,CHAR * pch; pch = GlobalAlloc(10); ASSERT(pch != NULL); pch0

53、= 0; ASSERT is (usually) a no-op in a free build And asserts should not be used for things that will happen in practice Program crash when out of memory,66,A better version!,CHAR * pch; pch = GlobalAlloc(10); If (NULL = pch) return E_OUTOFMEMORY; pch0 = 0;,67,9. 草木皆兵, 不可大意 - Avoid Assumptions,Dont a

54、ssume good behavior There will be badly-written apps Dont assume you can trust data Validate data you cant trust Public interfaces User input File contents Test your validation code! Dont assume you know all your users An interface may have unexpected clients and not all of them are friendly Documen

55、t the assumptions you make, use ASSERT,68,Spot the defect,CIRRealExtractor:CIRRealExtractor( HRESULT* hr, const BITMAPINFOHEADER *lpbmi, const void *lpvBits, const RECT *prectBlock) *hr = ContainDIB(lpbmi, lpvBits, prectBlock); ;,69,Spot the defect,CIRRealExtractor:CIRRealExtractor( HRESULT* hr, con

56、st BITMAPINFOHEADER *lpbmi, const void *lpvBits, const RECT *prectBlock) *hr = ContainDIB(lpbmi, lpvBits, prectBlock); ; Crash if “hr” is passed in as NULL! Crash in a C+ constructor is the last thing you want to do,70,A better version,CIRRealExtractor:CIRRealExtractor( HRESULT* hr, const BITMAPINFO

57、HEADER *lpbmi, const void *lpvBits, const RECT *prectBlock) if (hr) *hr = ContainDIB(lpbmi, lpvBits, prectBlock); ;,71,But wait, a even better version,CIRRealExtractor:CIRRealExtractor() HRESULT CIRRealExtractor:Init( const BITMAPINFOHEADER *lpbmi, const void *lpvBits, const RECT *prectBlock) / lpbm

58、i, lpvBits, and prectBlock can never be NULL ASSERT( lpbmi != NULL); ASSERT( lpvBits != NULL); ASSERT( prectBlock != NULL); return ContainDIB(lpbmi, lpvBits, prectBlock); ;,72,Spot the defect,Random reboots in low memory situations Immediate cause: services.exe exits. Why would services.exe exit? Would you believe that services often call wscicmp (and other string functions)? that calling these functions can cause your program to exit?,73,Code