Re: peer code review/advice needed for noob programmer
- From: Joseph M. Newcomer <newcomer@xxxxxxxxxxxx>
- Date: Tue, 22 May 2007 09:12:12 -0400
On Tue, 22 May 2007 00:49:00 -0700, Stick <Stick@xxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
Hi,****
I have the following function which works, but I want to do two things in
updating it.
1) I want to convert from sprintf() to sprintf_s() etc. to end warnings
2) I want to understand how one properly gets the size (vs. what I might
be doing)
and finally:
3) I compile on VS 2005 SP1, on 32 bit Windows XP Home SP2, and want to
know if I can compile with options to target a 64-bit platform so that this
function would run properly on Vista 64. (If possible) Actually, though,
this last is not going to be used as this program is part of a game add-on
that will run in a 32-bit game even when under Vista, so it is really a
non-issue. I just want to educate myself more.
Anyway, here is the function:
RETVALUE CMyClass::Buffer2HexString( const BYTE* pBuffer, const unsigned int
pBufferSize, char** pString )
Do not use Hungarian Notation if you are clueless about how to use it. You refer to a
const in as "pBufferSize", but "pBufferSize" means that BufferSize is a POINTER!
And what is with the bizarre use of char **? Or using an obsolete data type like 'char'?
Using char buffers in MFC is a DEAD style. The appearance of char** in an MFC program is
definitely weird-beyond-imagining in a case like this!
I wouldhave written this as
RETVALUE CMyClass::Buffer2HexString(CByteArray & Buffer, CString & String)
and not had a byte buffer, an explicit length, and certainly not something as silly as a
char** parameter!
*****
{*****
RETVALUE lReturnValue = SUCCESS;
unsigned int uOutStringSize = pBufferSize*2 + 2 + 1;
// Caller must delete[] this pointer
char* sOutString = new char[uOutStringSize];
First, forget that char exists. Consider it an obsolete data type that should be used in
rare and specialized conditions, of which this is not one. There is little if any reason
to allocate a buffer here.
Consider a style that uses fixed-size buffers as obsolete programming practice.
CString OutString;
*****
//sprintf( sOutString, "%s", "" );*****
sprintf_s( sOutString, uOutStringSize, "%s", "" );
Consid\er all forms of sprintf as being obsolete. Also, code Unicode-aware. Consider
'char' an obsolete data type.
OutString.Format(_T("%s"), _T("your string here"));
Note several things:
formatting an empty string into a buffer to initialize is simply silly.
_tcscpy(sOutString, _T(""));
would have done it for your case, and in the case of a
CString, you don't need to initialize it to the empty string
because the constructor already does that.
For that matter, even _tcscpy is considered by some to
be overkill, and they would write
sOutString[0] = _T('\0');
******
*****
for( unsigned int i = 0; i < pBufferSize / 2 ; i++ )
Why 'unsigned int'? Why not 'UINT'?
*****
{*****
for ( int j = 0; j < 2; j++ )
{
// 4 chars per group
BYTE b = pBuffer[ 2 * i + j ];
// NOTE: 2 chars in out string for each byte in input buffer
byte ch = 0x00;
char hexString[] = "00"; // NULL terminated
char pseudo[] = {"0123456789ABCDEF"};
This is truly weird code! What are you trying to do here?
For one thing, there is no need to declare the array of hex digits as shown; assuming this
horrible code was assumed to make sense at all, the correct declaration of the hex digits
would be
const static TCHAR pseudo[] = _T("0123456789ABCDEF");
The hexString would be
TCHAR hexString[3];
and would not need to be initialized. The initialization is silly. It serves no useful
purpose whatsoever.
Even in this case, the simplest approach would have been the one I show below, and even in
MFC it is a weird approach.
*****
****
ch = (byte) (b & 0xF0); // Strip off high nibble
ch = (byte) (ch >> 4); // Shift the bits down
ch = (byte) (ch & 0x0F); // In case high order bit set
hexString[0] = pseudo[ (int)ch ]; // Nibble to hex
The correct spelling is 'nybble'
****
*****
ch = (byte) (b & 0x0F); // Strip off low nibble
hexString[1] = pseudo[ (int)ch ]; // Nibble to hex
hexString[2] = NULL;
//strcat( sOutString, hexString );
strcat_s( sOutString, uOutStringSize, hexString );
This is truly one of the most bizarre ways I've ever seen to convert a value to hex.
What's wrong with
LPTSTR sep = _T("");
for(int i = 0; i < pBufferSize; i += 2)
{
TCHAR bt[6];
_stprintf_s(bt, _T("%s%02X%02X"), sizeof(bt)/sizeof(TCHAR),
sep, pBuffer[i], pBuffer[i+1]);
_tcscat_s(sOutString, uOutStringSize, bt);
sep = _T("-");
}
replacing ALL of the above baroque doubly-nested loop?
Also, how long is the byte array being passed in? For efficiency, there are better
techniques than using _tcscat_s to append to the string if there are hundreds of thousands
of bytes being formatted; for short lines, efficiency is not even worth discussing.
BUt if you pass in a CString & String then the correct loop is
CString sep;
for(int i = 0; i < pBufferSize; i += 2)
{
CString t;
t.Format(_T("%s%02X%02x"), sep, pBuffer[i], pBuffer[i+1]);
String += t;
sep = _T("-");
}
and if you use a CByteArray & Buffer then it would be
for(UINT_PTR i = 0; i < Buffer.GetSize(); i+=2)
for the loop.
If the byte array can have an odd number of bytes, a simple if-test will handle the
boundary case of the orphaned byte at the end:
CString t;
if(i+1 < pBufferSize)
t.Format(_T("%s%02X%02X"), sep, pBuffer[i], pBuffer[i+1]);
else
t.Format(_T("%s%02X"), sep, pBuffer[i]);
To read data into a CByteArray, you can do
CByteArray buffer;
...compute size required, n
buffer.SetSize(n);
f.Read(buffer.GetData(), n);
Make the assumption that if you use new to create an array of primitive types, you have
probably made a mistake. In fact, assume that if you use new to create an array of
anything, you have probably made a mistake. Using the built-in MFC CArray type,
CtypeArray for the simpler types (such as CByteArray) or std::vector<type> would be a
better choice in nearly all circumstances.
*****
}****
if ( i < ( pBufferSize / 2 - 1 ) )
{
strcat_s( sOutString, uOutStringSize, "-" );
}
Since there isn't a spec on the intention, I'm believing that the role of this test is to
put a hyphen between each pair of bytes except the last pair. This is a very odd approach
as to how to accomplish this. See my example above for the more typical way. The output
I've written would show the sequence 01 02 03 04 05 06 07 08 as
0102-0304-0506-0708
Note that this does not represent the WORD values, which would be
0201-0403-0605-0807
****
}*****
*pString = sOutString;
This should not be necessary, since you would format directly into the CString &.
*****
****
return lReturnValue;
Since there is no condition that could return OTHER than SUCCESS, I'm curious why this is
not a void method.
****
}*****
This function was adapted from one I found online, and I have not
"corrected" the var names, so if you comments on better more "Code Complete"
names please let me know what you think too. I shall be thick skinned. =)
All critisim appreciated.
I have no idea what a Code Complete name is, but the notation here is definitely bizarre.
And frankly, I consider Hungarian Notation to be a quaint and primitive relic of a piece
of the history of the C language that is no longer relevant for modern programming. It
solved a problem that no longer exists, and hasn't existed for over 15 years. It should
be ignored.
Other than a complete rewrite to make it smaller and simpler, there's nothing wrong with
it. It is exceptionally poor code for all the reasons observed above. It uses obsolete
data types, it uses explicit allocation, it uses obsolete operations like sprintf (and
sprintf_s is still essentially obsolete), it goes out of its way to use the most
complicated way possible to do the conversion, and so on. It is inconsistent with C++ and
MFC best practice. It is not Unicode-aware. I'd scrap the whole thing, including the
parameter types. Not to put too fine a point on it, if this were a homework assignment it
would get either a D or an F, depending on what I had taught as best practice up to that
point.
joe
*****
Joseph M. Newcomer [MVP]
Thanks, Patrick
email: newcomer@xxxxxxxxxxxx
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
.
- Follow-Ups:
- References:
- Prev by Date: Re: AliR, may I have your attention? Please.
- Next by Date: Re: Compilator C++ freeware for commercial use
- Previous by thread: peer code review/advice needed for noob programmer
- Next by thread: Re: peer code review/advice needed for noob programmer
- Index(es):
Relevant Pages
|
|
