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
代码高手十大秘诀
1,集百家之长,归我所用 - Follow Basic
Coding Style
2,取个好名字 - Use Naming Conventions
3,凌波微步,未必摔跤 - Evil goto’s? Maybe
Not…
4,先发制人,后发制于人 - Practice Defensive
Coding
5,见招拆招,滴水不漏 - Handle The Error
Cases,They Will Occur!
7
代码高手十大秘诀 (Cont.)
6,熟习剑法刀术,所向无敌 - Learn Win32
API Seriously
7,双手互搏,无坚不摧 - Test,but don’t stop
there
8,活用段言 - Use,don’t abuse,assertions
9,草木皆兵,不可大意 - Avoid Assumptions
10,最高境界,无招胜有招 - Stop writing so
much code
8
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
1,集百家之长,归我所用 -
Follow Basic Coding Style
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?
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.
Over 1400 lines!!!
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?
… …
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;
pszHiName =(CHAR*)malloc(…);
if (pszHiName == NULL)
goto Exit;
24
Single 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;
pszHiName =(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;
pszHeName =(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
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];
}
… …
51
Better version
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];
}
… …
ZeroMemory(re,256);
ZeroMemory(im,256);
ZeroMemory(AvrSpec,128);
… …
#define FrameLen 200
MoveMemory(lsp,
&(lsp[40*FrameLen]),
5*40*FrameLen);
… …
CopyMemory(audio,vect,
FrameLen);
… …
52
Spot the defect
TCHAR WindowsDir[MAX_PATH];
TCHAR DriveLetter;
GetWindowsDirectory(WindowsDir,MAX_PATH);
DriveLetter = WindowsDir[0];
...
53
Spot the defect
TCHAR WindowsDir[MAX_PATH];
TCHAR DriveLetter;
GetWindowsDirectory(WindowsDir,MAX_PATH);
DriveLetter = WindowsDir[0];
...
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 allocate
memory
– If allocation fails,the call returns FALSE
Result,potential catastrophic errors on Terminal
Server
55
A better version
TCHAR WindowsDir[MAX_PATH];
TCHAR DriveLetter;
if (!GetWindowsDirectory(WindowsDir,
MAX_PATH)) {
return E_OUTOFMEMORY; // proper recovery
}
DriveLetter = WindowsDir[0];
...
56
7,双手互搏,无坚不摧 - Test,
but don’t stop there
Step through in the debugger
– Especially the error code paths
Test as much of your code as you can
– If you haven’t tested it,it probably doesn’t work
– If you don’t know how much you’ve tested,it’s less than you
expect
So find out!
Code review
– Have somebody else read it
– And read somebody else’s code!
57
Spot the defect
HRESULT hr = NOERROR;
// Make sure the arguments passed are valid
if (NULL != m_paConnections) {
..,// do the right thing
}
else hr = S_FALSE;
if (SUCCEEDED(hr)) {
if (NULL != m_paConnections[0].pUnk)
.,,
58
Spot the defect
HRESULT hr = NOERROR;
// Make sure the arguments passed are valid
if (NULL != m_paConnections) {
..,// do the right thing
}
else hr = S_FALSE;
if (SUCCEEDED(hr)) {
if (NULL != m_paConnections[0].pUnk)
.,,
SUCCEEDED(S_FALSE) == TRUE
Crash if m_paConnections == NULL
– Easily simulatable in the debugger
59
Better version
HRESULT hr = NOERROR;
// Make sure the arguments passed are valid
if (NULL != m_paConnections) {
..,// do the right thing
}
else hr = E_INVALIDARG;
if (SUCCEEDED(hr)) {
if (NULL != m_paConnections[0].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 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,GetFileSize 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,GetFileSize returns –1 if it fails.
Compiler will catch the error if you miss,=“
63
8,活用段言 - Use,don’t abuse,
Assertions
Use Assertions because
– Useful for documentation
Describe assumptions
Describe,impossible” situations
– Useful for debugging
Help diagnose problems
May detect problems earlier
Don’t abuse Assertions because
– Assertions are usually no-ops in a free/release build
Don’t use Assertions in situations that can occur
in practice
64
Spot the defect
CHAR * pch;
pch = GlobalAlloc(10);
ASSERT(pch != NULL);
pch[0] = 0;
65
Spot the defect
CHAR * pch;
pch = GlobalAlloc(10);
ASSERT(pch != NULL);
pch[0] = 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;
}
pch[0] = 0;
67
9,草木皆兵,不可大意 - Avoid
Assumptions
Don’t assume good behavior
– There will be badly-written apps
Don’t assume you can trust data
– Validate data you can’t trust
Public interfaces
User input
File contents
– Test your validation code!
Don’t assume you know all your users
– An interface may have unexpected clients
– … and not all of them are friendly
Document 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,
const 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 BITMAPINFOHEADER *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)
{
… …
// lpbmi,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 inside wscicmp()
… …
if ( _locktable[locknum] == NULL ) {
if ( (pcs =
_malloc_crt(sizeof(CRITICAL_SECTION)))
== NULL )
_amsg_exit(_RT_LOCK);
… …
74
The defect
… …
if ( _locktable[locknum] == NULL ) {
if ( (pcs =
_malloc_crt(sizeof(CRITICAL_SECTION)))
== NULL )
_amsg_exit(_RT_LOCK);
… …
Wscicmp assumed the lock allocated
successfully,or it’s ok to exit!
Good or bad,I am the one with the bug!
75
10,最高境界,无招胜有招 -
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 bugs
76
Some 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
欲练神功,端正态度
It’s all about attitude
Take pride in your code
– Want it to be perfect
Correct,reliable,clean,simple,high-performance
(speed,memory,…)
– Want it to be a,role model”
Don’t get your ego wrapped up in it
– Admit that your code’s not perfect
– Actively seek out problems
– Strive to improve it
78
Spot the detect
Comment from 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,I'm just not creating it
in this situation,*/
79
Spot the detect
Comment from 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,I'm just not creating it
in this situation,*/
i.e.,“I know the right thing to do but I
couldn’t make it work so I gave up.”
80
Happy Coding
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
代码高手十大秘诀
1,集百家之长,归我所用 - Follow Basic
Coding Style
2,取个好名字 - Use Naming Conventions
3,凌波微步,未必摔跤 - Evil goto’s? Maybe
Not…
4,先发制人,后发制于人 - Practice Defensive
Coding
5,见招拆招,滴水不漏 - Handle The Error
Cases,They Will Occur!
7
代码高手十大秘诀 (Cont.)
6,熟习剑法刀术,所向无敌 - Learn Win32
API Seriously
7,双手互搏,无坚不摧 - Test,but don’t stop
there
8,活用段言 - Use,don’t abuse,assertions
9,草木皆兵,不可大意 - Avoid Assumptions
10,最高境界,无招胜有招 - Stop writing so
much code
8
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
1,集百家之长,归我所用 -
Follow Basic Coding Style
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?
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.
Over 1400 lines!!!
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?
… …
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;
pszHiName =(CHAR*)malloc(…);
if (pszHiName == NULL)
goto Exit;
24
Single 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;
pszHiName =(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;
pszHeName =(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
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];
}
… …
51
Better version
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];
}
… …
ZeroMemory(re,256);
ZeroMemory(im,256);
ZeroMemory(AvrSpec,128);
… …
#define FrameLen 200
MoveMemory(lsp,
&(lsp[40*FrameLen]),
5*40*FrameLen);
… …
CopyMemory(audio,vect,
FrameLen);
… …
52
Spot the defect
TCHAR WindowsDir[MAX_PATH];
TCHAR DriveLetter;
GetWindowsDirectory(WindowsDir,MAX_PATH);
DriveLetter = WindowsDir[0];
...
53
Spot the defect
TCHAR WindowsDir[MAX_PATH];
TCHAR DriveLetter;
GetWindowsDirectory(WindowsDir,MAX_PATH);
DriveLetter = WindowsDir[0];
...
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 allocate
memory
– If allocation fails,the call returns FALSE
Result,potential catastrophic errors on Terminal
Server
55
A better version
TCHAR WindowsDir[MAX_PATH];
TCHAR DriveLetter;
if (!GetWindowsDirectory(WindowsDir,
MAX_PATH)) {
return E_OUTOFMEMORY; // proper recovery
}
DriveLetter = WindowsDir[0];
...
56
7,双手互搏,无坚不摧 - Test,
but don’t stop there
Step through in the debugger
– Especially the error code paths
Test as much of your code as you can
– If you haven’t tested it,it probably doesn’t work
– If you don’t know how much you’ve tested,it’s less than you
expect
So find out!
Code review
– Have somebody else read it
– And read somebody else’s code!
57
Spot the defect
HRESULT hr = NOERROR;
// Make sure the arguments passed are valid
if (NULL != m_paConnections) {
..,// do the right thing
}
else hr = S_FALSE;
if (SUCCEEDED(hr)) {
if (NULL != m_paConnections[0].pUnk)
.,,
58
Spot the defect
HRESULT hr = NOERROR;
// Make sure the arguments passed are valid
if (NULL != m_paConnections) {
..,// do the right thing
}
else hr = S_FALSE;
if (SUCCEEDED(hr)) {
if (NULL != m_paConnections[0].pUnk)
.,,
SUCCEEDED(S_FALSE) == TRUE
Crash if m_paConnections == NULL
– Easily simulatable in the debugger
59
Better version
HRESULT hr = NOERROR;
// Make sure the arguments passed are valid
if (NULL != m_paConnections) {
..,// do the right thing
}
else hr = E_INVALIDARG;
if (SUCCEEDED(hr)) {
if (NULL != m_paConnections[0].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 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,GetFileSize 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,GetFileSize returns –1 if it fails.
Compiler will catch the error if you miss,=“
63
8,活用段言 - Use,don’t abuse,
Assertions
Use Assertions because
– Useful for documentation
Describe assumptions
Describe,impossible” situations
– Useful for debugging
Help diagnose problems
May detect problems earlier
Don’t abuse Assertions because
– Assertions are usually no-ops in a free/release build
Don’t use Assertions in situations that can occur
in practice
64
Spot the defect
CHAR * pch;
pch = GlobalAlloc(10);
ASSERT(pch != NULL);
pch[0] = 0;
65
Spot the defect
CHAR * pch;
pch = GlobalAlloc(10);
ASSERT(pch != NULL);
pch[0] = 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;
}
pch[0] = 0;
67
9,草木皆兵,不可大意 - Avoid
Assumptions
Don’t assume good behavior
– There will be badly-written apps
Don’t assume you can trust data
– Validate data you can’t trust
Public interfaces
User input
File contents
– Test your validation code!
Don’t assume you know all your users
– An interface may have unexpected clients
– … and not all of them are friendly
Document 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,
const 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 BITMAPINFOHEADER *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)
{
… …
// lpbmi,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 inside wscicmp()
… …
if ( _locktable[locknum] == NULL ) {
if ( (pcs =
_malloc_crt(sizeof(CRITICAL_SECTION)))
== NULL )
_amsg_exit(_RT_LOCK);
… …
74
The defect
… …
if ( _locktable[locknum] == NULL ) {
if ( (pcs =
_malloc_crt(sizeof(CRITICAL_SECTION)))
== NULL )
_amsg_exit(_RT_LOCK);
… …
Wscicmp assumed the lock allocated
successfully,or it’s ok to exit!
Good or bad,I am the one with the bug!
75
10,最高境界,无招胜有招 -
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 bugs
76
Some 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
欲练神功,端正态度
It’s all about attitude
Take pride in your code
– Want it to be perfect
Correct,reliable,clean,simple,high-performance
(speed,memory,…)
– Want it to be a,role model”
Don’t get your ego wrapped up in it
– Admit that your code’s not perfect
– Actively seek out problems
– Strive to improve it
78
Spot the detect
Comment from 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,I'm just not creating it
in this situation,*/
79
Spot the detect
Comment from 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,I'm just not creating it
in this situation,*/
i.e.,“I know the right thing to do but I
couldn’t make it work so I gave up.”
80
Happy Coding
