10 Things You Can Do To Write Better Code 写好代码的十个秘诀

1. 10 Things You Can Do To Write Better Code写好代码的十个秘诀 林斌 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 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; i<100; i++) InputRec.revenue[i] = 0; for (i=1; i<100; i++) InputRec.expense[i]= CorpExpense[i]; UpdateCorpDatabase(EmpRec); EstimRevenue =YTDRevenue*4.0/(float)CrntQtr; NewColor = PreColor; Status = Success; if (ExpenseType== 1) for (i=1;i<12;i++) Profit[i] = Revenue[i]-Expense.Type1[i]; else if (ExpenseType == 2) Profit[i] = Revenue[i] - Expense.Type2[i]; else if (ExpenseType==3) { Profit[i] =Revenue[i] -Expense.Type3[i]; }

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; i<100; i++) InputRec.revenue[i] = 0; for (i=1; i<100; i++) InputRec.expense[i]= CorpExpense[i]; UpdateCorpDatabase(EmpRec); EstimRevenue =YTDRevenue*4.0/(float)CrntQtr; NewColor = PreColor; Status = Success; if (ExpenseType== 1) for (i=1;i<12;i++) Profit[i] = Revenue[i]-Expense.Type1[i]; else if (ExpenseType == 2) Profit[i] = Revenue[i] - Expense.Type2[i]; else if (ExpenseType==3) { Profit[i] =Revenue[i] -Expense.Type3[i]; }

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 & Maintainability • None testable if ExpenseType is 1 – Testability • Many more: too many arguments, abuse usage of 100, 4.0, etc, un-use parameters, none-documented parameters…

6. 代码高手十大秘诀 • 集百家之长, 归我所用 - Follow Basic Coding Style • 取个好名字 - Use Naming Conventions • 凌波微步, 未必摔跤 - Evil goto’s? Maybe Not… • 先发制人, 后发制于人- Practice Defensive Coding • 见招拆招, 滴水不漏 - Handle The Error Cases: They Will Occur!

7. 代码高手十大秘诀 (Cont.) • 熟习剑法刀术, 所向无敌 - Learn Win32 API Seriously • 双手互搏, 无坚不摧 - Test, but don’t stop there • 活用段言 - Use, don’t abuse, assertions • 草木皆兵, 不可大意 - Avoid Assumptions • 最高境界, 无招胜有招 - Stop writing so much code

8. 1. 集百家之长, 归我所用 - Follow Basic Coding Style • Your code may outlive you or your memory! • Think about the maintainer • Comment your code • File Header • Function Header • Inline Commenting • Pick a good name for your functions and variables • Set Tab/Indent to 4 characters • Align your code! • Less arguments

9. Coding Style (Cont.) • Use all uppercase with underscores for macro. #define MY_MACRO_NUMBER 100 • Declare all your variables at the start of function. • Avoid hard coding constant. Use enum or macro. #define MY_MAX_LENGTH 1024 • User braces for one line code If (m_fInitialized) { hr = S_OK; } • Limit the length of a single function

10. Spot the defect DWORD    Status = NO_ERROR; LPWSTR ptr; Status = f( ..., &ptr ); if( Status isnot NO_ERROR or ptr is NULL ) goto cleanup;

11. Spot the defect DWORD    Status = NO_ERROR; LPWSTR ptr; Status = f( ..., &ptr ); if( Status isnot NO_ERROR or ptr is NULL ) goto cleanup; • 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 = NULL; Status = f( ..., &ptr ); if( (Status != NO_ERROR) || (ptr == NULL) ) goto cleanup; • Make your intent explicit • Use existing language constructs • Everybody understands them • Avoid future problems: • Initialize • parenthesize

13. What’s the maximum length of a function? Over 1400 lines!!! • Look at this example • Watch out for functions > 200 lines! • Study in 1986 on IBM’s OS/360: the most error-prone routines => longer than 500 lines. • Study in 1991 on 148,000-line of code: functions < 143 lines => 2.4 times less expensive to fix than longer functions.

14. Look at these codes • Let’s 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:

15. 2.取个好名字 - 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 “BaseTag” 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. Let’s 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_szLogFile[MAX_PATH]; • A pointer to a global logfile: • CHAR* g_pszLogFile;

17. 3.凌波微步, 未必摔跤 - Evil goto’s? 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; 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 = (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*)malloc(256); if (pszHerName == NULL) { free(pszMyName); 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*)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 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? 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; … … Exit: if (pszMyName) free(pszMyName); if (pszHeName) free(pszHeName); if (pszHiName) free(pszHiName); return hr; }

24. Single entry, single exit. No duplicate code… 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; … … Exit: if (pszMyName) free(pszMyName); if (pszHeName) free(pszHeName); if (pszHiName) free(pszHiName); return hr; }

25. Easy to maintain… HRESULT Init() { pszMyName =(CHAR*)malloc(…); if (pszMyName == NULL) goto Exit; pszHeName =(CHAR*)malloc(…); if (pszHeName == NULL) goto Exit; pszItName =(CHAR*)malloc(…); if (pszItName == NULL) goto Exit; pszHiName =(CHAR*)malloc(…); if (pszHiName == NULL) goto Exit; … … Exit: if (pszMyName) free(pszMyName); if (pszHeName) free(pszHeName); if (pszItName) free(pszItName); if (pszHiName) free(pszHiName); return hr; }

26. Use the following goto guidelines • Single entry, single exit? – use goto • Don’t use more than one goto labels • Use goto’s that go forward, not backward • Make sure a goto doesn’t create unreachable code

27. 4. 先发制人, 后发制于人 - 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 • 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 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 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 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 wcscpy(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); 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! • Don’t fail in C++ object constructor, you can’t 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 adhere 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_plSections = new TPtrList(); // ... m_ReadContext.posLine = m_plLines->end(); . . . } • MFC’s 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(); . . . }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_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 (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_plLines) 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(NAME_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_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) { • CHAR c = *(pfoo->GetName()); • }

48. A better version Class 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 == NULL) { hr = E_OUTOFMEMORY; return hr; } strcpy(m_pszName, pszName); } else { hr = E_INVALIDARG; } return hr; }

49. 6. 熟习剑法刀术, 所向无敌 - 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[i] = 0; im[i] = 0; } for( k = 0; k < 128; k++) AvrSpec[k] = 0; … … #define FrameLen 200 for(k=0; k<5*40*FrameLen; k++) lsp[k] = lsp[k + 40*FrameLen]; … … for(k = 0; k < FrameLen; k++) { audio[k] = vect[k]; } … …