1、110 Things You Can Do To Write Better Code写好代码的十个秘诀写好代码的十个秘诀林斌林斌Development ManagerMicrosoft Research,China2一流代码的特性 鲁棒-Solid and Robust Code 简洁-Maintainable and Simple Code 高效-Fast Code 简短-Small Code 共享-Re-usable Code 可测试-Testable Code 可移植-Portable Code一流一流代码代码3Why is this code bad?void MyGirlFriend
2、Func(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=CorpExpensei;UpdateCorpDatabase(EmpR
3、ec);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-Expense.Type3i;4Why is this code bad?void MyGirlFriendFunc(CORP
4、_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=CorpExpensei;UpdateCorpDatabase(EmpRec);Estim
5、Revenue=YTDRevenue*4.0/(float)CrntQtr;NewColor=PreColor;Status=Success;if(ExpenseType=1)for(i=1;i 200 lines!Study in 1986 on IBMs OS/360:the most error-prone 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
6、 lines!14Look at these codes Lets look at these codes from our own projects:Small functions:Larger functions:Then look at those from Windows 2000 source tree:NT Kernel Mode Code:NT User Mode CPP File:NT User Mode Header File:152.取个好名字取个好名字-Use Naming ConventionsHungarian Notation Prefix-BaseTag-Name
7、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“BaseTag”void -v int -iBOOL -f UINT -uiBYTE -b CHAR -chWCHAR -wch ULONG -ulLONG -l DWORD -dwHRESULT-hr fn -functionsz -NULL str USHORT,SHORT,WORD-wC+member variables start wit
8、h:m_Global variables start with:g_16Lets try these A flag indicates whether a C+object has been 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
9、_PATH;A pointer to a global logfile:CHAR*g_pszLogFile;173.凌波微步,未必摔跤-Evil gotos?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 code18Spot the gotosSTART_LOOP:If(fStatusOk)if(fDataAvai
10、able)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:19BAD gotos!START_LOOP:If(fStatusOk)if(fDataAvaiable)i=10;goto MID_LOOP;else goto END_LOOP;else for(I=0;I=0)return TRUE;34Spot the detectBOOL SomeProblem(USHORT x)while(-x=0)return T
11、RUE;Infinite loop!Unsigned never negative!35Spot the detectwcscpy(wcServerIpAdd,WinsAnsiToUnicode(cAddr,NULL);Note:WinsAnsiToUnicode is a private API which allocates a new UNICODE string given an ANSI string.36Spot the detectwcscpy(wcServerIpAdd,WinsAnsiToUnicode(cAddr,NULL);WinsAnsiToUnicode can re
12、turn NULL which will cause a program crashWinsAnsiToUnicode can return allocated memory which will cause a leak37A better versionLPWSTR tmp;tmp=WinsAnsiToUnicode(cAddr,NULL);if(tmp!=NULL)wcscpy(wcServerIpAdd,tmp);WinsFreeMemory(PVOID)tmp);else return(ERROR_NOT_ENOUGH_MEMORY);385.见招拆招,滴水不漏-Handle The
13、 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+object constructor,you cant detect it!39Error cases(continued)In an error case,the code should Recover gr
14、acefully 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 adhere to that definition What not to do Ignore the error Crash Exit40Spot the detectCWInfFile:CWInfFile()m_plLin
15、es =new TPtrList();/.m_plSections =new TPtrList();/.m_ReadContext.posLine=m_plLines-end();.41Spot the detectCWInfFile:CWInfFile()m_plLines =new TPtrList();/.m_plSections =new TPtrList();/.m_ReadContext.posLine=m_plLines-end();.vMFCs operator new throws an exception causing a leak when out of memory4
16、2A better version?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_plSections)delete m_plSections;43No!CWInfFile:CWInfFile()try m_plLines =new TPtrList();/.m_plSections =new TPtrList(
17、);/.m_ReadContext.posLine=m_plLines-end();.catch(.)if(m_plLines)delete m_plLines;if(m_plSections)delete m_plSections;44A better versionCWInfFile:CWInfFile():m_plLines(NULL),m_plSections(NULL)try m_plLines =new TPtrList();/.m_plSections =new TPtrList();/.m_ReadContext.posLine=m_plLines-end();.catch(.
18、)if(m_plLines)delete m_plLines;if(m_plSections)delete m_plSections;45But wait,there is moreCWInfFile: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_plLines)delete m_plLines;if(m_plSectio
19、ns)delete m_plSections;46Spot the defectClass 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_pszName=NULL)return;strcpy(m_pszName,pszName);m_cbName=strlen(pszName);47Spot the defectClass fo
20、o 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_pszName=NULL)return;strcpy(m_pszName,pszName);m_cbName=strlen(pszName);If malloc()fails,this code will crash:foo*pfoo=new foo(“MyName”);if(pfoo)
21、CHAR c=*(pfoo-GetName();48A better versionClass foo private:CHAR*m_pszName;DWORD m_cbName;public:foo();HRESULT Init(CHAR*pszName);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=NUL
22、L)hr=E_OUTOFMEMORY;return hr;strcpy(m_pszName,pszName);else hr=E_INVALIDARG;return hr;496.熟习剑法刀术,所向无敌-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
23、 Pick the best one based on your need50Spot the defectfor(i=0;i 256;i+)rei=0;imi=0;for(k=0;k 128;k+)AvrSpeck=0;#define FrameLen 200for(k=0;k5*40*FrameLen;k+)lspk=lspk+40*FrameLen;for(k=0;k FrameLen;k+)audiok=vectk;51Better versionfor(i=0;i 256;i+)rei=0;imi=0;for(k=0;k 128;k+)AvrSpeck=0;#define Frame
24、Len 200for(k=0;k5*40*FrameLen;k+)lspk=lspk+40*FrameLen;for(k=0;k FrameLen;k+)audiok=vectk;ZeroMemory(re,256);ZeroMemory(im,256);ZeroMemory(AvrSpec,128);#define FrameLen 200MoveMemory(lsp,&(lsp40*FrameLen),5*40*FrameLen);CopyMemory(audio,vect,FrameLen);52Spot the defect53Spot the defect54Can GetWindo
25、wsDirectory fail?MSDN has always said“yes”Implementation has always said“no”Until Terminal Server!Computing GetWindowsDirectory may allocate memory If allocation fails,the call returns FALSE Result:potential catastrophic errors on Terminal Server55A better version567.双手互搏,无坚不摧-Test,but dont stop the
26、re 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 read it And read somebody elses code!57Spot
27、the defectHRESULT hr=NOERROR;/Make sure the arguments passed are validif(NULL!=m_paConnections)./do the right thingelse hr=S_FALSE;if(SUCCEEDED(hr)if(NULL!=m_paConnections0.pUnk).58Spot the defectHRESULT hr=NOERROR;/Make sure the arguments passed are validif(NULL!=m_paConnections)./do the right thin
28、gelse hr=S_FALSE;if(SUCCEEDED(hr)if(NULL!=m_paConnections0.pUnk).SUCCEEDED(S_FALSE)=TRUE Crash if m_paConnections=NULL Easily simulatable in the debugger59Better versionHRESULT hr=NOERROR;/Make sure the arguments passed are validif(NULL!=m_paConnections)./do the right thingelse hr=E_INVALIDARG;if(SU
29、CCEEDED(hr)if(NULL!=m_paConnections0.pUnk).60Spot 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:GetFileSize returns 1 if it fails.61Spot the defect/Get file size f
30、irst/DWORD dwFileSize=GetFileSize(hFile,NULL);if(dwFileSize=-1)/what can we do?keep silent ErrorTrace(0,GetFileSize failed with%d,GetLastError();return;Note:GetFileSize returns 1 if it fails.62Better version/Get file size first/DWORD dwFileSize=GetFileSize(hFile,NULL);if(-1=dwFileSize)/what can we d
31、o?keep silent ErrorTrace(0,GetFileSize failed with%d,GetLastError();return;Note:GetFileSize returns 1 if it fails.638.活用段言-Use,dont abuse,Assertions Use Assertions because Useful for documentation Describe assumptions Describe“impossible”situations Useful for debugging Help diagnose problems May det
32、ect problems earlier Dont abuse Assertions because Assertions are usually no-ops in a free/release build Dont use Assertions in situations that can occur in practice64Spot the defectCHAR*pch;pch=GlobalAlloc(10);ASSERT(pch!=NULL);pch0=0;65Spot the defectCHAR*pch;pch=GlobalAlloc(10);ASSERT(pch!=NULL);
33、pch0=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 memory66A better version!CHAR*pch;pch=GlobalAlloc(10);If(NULL=pch)return E_OUTOFMEMORY;pch0=0;679.草木皆兵,不可大意-Avoid Assumptions Dont assume good behavior Th
34、ere 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 Document the assumptions you m
35、ake,use ASSERT68Spot the defectCIRRealExtractor:CIRRealExtractor(HRESULT*hr,const BITMAPINFOHEADER*lpbmi,const void*lpvBits,const RECT*prectBlock)*hr=ContainDIB(lpbmi,lpvBits,prectBlock);69Spot the defectCIRRealExtractor:CIRRealExtractor(HRESULT*hr,const BITMAPINFOHEADER*lpbmi,const void*lpvBits,con
36、st 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 do70A better versionCIRRealExtractor:CIRRealExtractor(HRESULT*hr,const BITMAPINFOHEADER*lpbmi,const void*lpvBits,const RECT*prectBlock)if(hr)*hr=Contain
37、DIB(lpbmi,lpvBits,prectBlock);71But wait,a even better versionCIRRealExtractor:CIRRealExtractor()HRESULT CIRRealExtractor:Init(const BITMAPINFOHEADER*lpbmi,const void*lpvBits,const RECT*prectBlock)/lpbmi,lpvBits,and prectBlock can never be NULL ASSERT(lpbmi!=NULL);ASSERT(lpvBits!=NULL);ASSERT(prectB
38、lock!=NULL);return ContainDIB(lpbmi,lpvBits,prectBlock);72Spot the defectRandom 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 p
39、rogram to exit?73Code inside wscicmp()if(_locktablelocknum=NULL)if(pcs=_malloc_crt(sizeof(CRITICAL_SECTION)=NULL)_amsg_exit(_RT_LOCK);74The defect if(_locktablelocknum=NULL)if(pcs=_malloc_crt(sizeof(CRITICAL_SECTION)=NULL)_amsg_exit(_RT_LOCK);Wscicmp assumed the lock allocated successfully,or its ok
40、 to exit!Good or bad,I am the one with the bug!7510.最高境界,无招胜有招-Stop writing so much code More code is not better!Do you really need your own Memory allocator?global operator new?Assertion macro?String/Handle/Smart Pointer class?Hash table?Think twice before you cut-and-paste Copied code is copied bu
41、gs76Some highlights In the Win2K code base:Over 100 different global operator news Over 80 different string implementations Hundreds(thousands?)of copied functions 9 different JPEG implementations 3 copies of atlimpl.cpp(3 different versions)Exactly duplicated ANSI/UNICODE files 77欲练神功,端正态度Its all a
42、bout attitudeTake pride in your code Want it to be perfectCorrect,reliable,clean,simple,high-performance(speed,memory,)Want it to be a“role model”Dont get your ego wrapped up in it Admit that your codes not perfect Actively seek out problems Strive to improve it78Spot the detectComment from PREfix s
43、ource code:/*In theory it might be possible to create the data with the right scope,but my attempts to do so have all created crashes of one sort or another.Since we only use this information to improve the clarity of our output,Im just not creating it in this situation.*/79Spot the detectComment fr
44、om PREfix source code:/*In theory it might be possible to create the data with the right scope,but my attempts to do so have all created crashes of one sort or another.Since we only use this information to improve the clarity of our output,Im just not creating it in this situation.*/i.e.,“I know the right thing to do but I couldnt make it work so I gave up.”80Happy Coding
