Re: peer code review/advice needed for noob programmer

See below...
On Tue, 22 May 2007 07:34:02 -0700, Stick <Stick@xxxxxxxxxxxxxxxxxxxxxxxxx> wrote:

Joe,

First, thanks for your obviously complete response. I see you spent a lot
of time on it, and I shall spend a lot of time learning from it.

"Joseph M. Newcomer" wrote:

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!

Yes, as I mentioned, I had not yet corrected the notation. It is really
bad, I agree. Actually, I'm familiar with it, and an avid follower of Code
Complete. I am just in such a rush to get this product out, that I am not
fixing stuff that is not broken just yet, but it bugs me as much as you, I
assure you!


And what is with the bizarre use of char **? Or using an obsolete data type
like 'char'?

Well, I am only using c-strings as I am getting them from the API of the
game I am forced to use, and am not experience enough yet to use the class
easily. The char** is basically I'm thinking a bad idea, as a way to
communicate back to the caller.
****
There are lots of easy ways to transition from char to CString; if you were using VS.NET
then if you are forced to use 8-bit characters in a Unicode app you could use explicit
CStringA. See my essay on CStrings on my MVP Tips site.
****


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!

Yes, except I am having to program to Microsoft FS's API, and the string is
coming from code that I have no control of as a null terminated UNICODE
string. But, I'm betting that I can take that string and use a CString as
you are suggesting, so I shall read up on this more.
*****
Which API is that? I don't recognize the term.
*****


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!
*****

Understood. I will work on this as I do understand what you are saying. I
am learning by example, and unfortunately that code was a poor example and
outdated.


{
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.

Ok, will do. For the moment I'm stuck with it, but I will eventuallly do so.


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'?
*****

No real reason other than I wasn't sure that UINT would be defined unless I
included Windows.h, which in this little class, I didn't want to bother if I
didn't need to do so.
****
Since windows.h is included in stdafx.h, and you would normally include stdafx.h in all
your modules, there is zero cost to having these symbols available.
****

{
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?

Basically, I'm formating a string into this: 0000-0000-0000 (it is a key
code for a product).
*****
Then you don't need performance. In fact, you don;t even need a loop.

CString s;
s.Format(_T("%02X%02X-%02x%02X-%02X%02X"), key[0], key[1], key[2], key[3], key[4],
key[5]);

is sufficient. If you have a known, fixed number of bytes and it is small, complicated
code is unnecessary.
*****

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.

Got it. I did it only to visually remind me that I had two hex chars that
were null terminated, but I see your point.


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'
****

An interesting, if pendantic, fact. I shall google this for details. =)



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.

Haha. Just goes to show what happens when you are a psychologist tries to
program a computer without formal training. ;)


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?

Well, besides that it is currently over my head, absolutely nothing. I
shall study this code. I understand the _T macro, and the basics of TCHAR,
but have never found a great reference to take me from the old way to the new
UNICODE aware ways.
****
Check out my essay on CStrings.
****


Also, how long is the byte array being passed in?

6
****
See the previous example. For 6, no loop is required.
****

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.
*****

Understood. I remember reading about this idea.


}
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.

You are correct.

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
****

Ok.

}
*pString = sOutString;
*****
This should not be necessary, since you would format directly into the CString &.
*****

K.


return lReturnValue;
****
Since there is no condition that could return OTHER than SUCCESS, I'm curious why this is
not a void method.
****

I just didn't change it from the original code yet. I wondered that myself.


}

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.

I am refering to the book "Code Complete".
****
I gathered that. Unfortunately, HN is a religion, not a techical concept, and in modern
programming it serves no useful purpose. In "Code Complete", it is thought to be
meaningful, but it never was once C had function prototypes.
****

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.

Yes, as I become more comfortable with C++, I understand and agree.
Frankly, I didn't understand the choice of names used in that code either.


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.

Haha. =)

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.

Thanks Joe for your detailed review. I didn't think this approach was
correct at all, but since it did the job I needed done with a couple of quick
patches and I had no time to do more at the moment, I left it.

Now, you have given me ideas about how I should later correct it, and I
appreciate your time.

Patrick
http://www.benchmark-avionics.com
Joseph M. Newcomer [MVP]
email: newcomer@xxxxxxxxxxxx
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
.

Relevant Pages

  • Re: Cannot return values of char variable
    ... - buffer = ... Since you seem to be trying to return a char pointer ... int id = random; ... content is interpreted as a string. ...
    (comp.lang.c)
  • Re: why I can not write to the file after initialize the MFC in a service program
    ... you don't use char, an obsolete data type ... Why do you need an intermedate buffer to write literal strings anyway? ... For example, if AfxWinInit fails, you copy a 45-character string into a ... So you are going to try to initialize MFC EACH TIME THROUGH THE LOOP? ...
    (microsoft.public.vc.mfc)
  • Re: why I can not write to the file after initialize the MFC in a service program
    ... you don't use char, an obsolete data type ... Why do you need an intermedate buffer to write literal strings anyway? ... For example, if AfxWinInit fails, you copy a 45-character string into a ... So you are going to try to initialize MFC EACH TIME THROUGH THE LOOP? ...
    (microsoft.public.vc.mfc)
  • Re: peer code review/advice needed for noob programmer
    ... And what is with the bizarre use of char **? ... Or using an obsolete data type like 'char'? ... to allocate a buffer here. ... BUt if you pass in a CString & String then the correct loop is ...
    (microsoft.public.vc.mfc)
  • Re: return string
    ... the const tells you that the field name is not going to be modified, the CString & ... is your responsibility to make sure you do not exceed the buffer size. ... use the obsolete data type 'char' for any purpose ... NEVER pass a pointer to a buffer without passing the size!!!!! ...
    (microsoft.public.vc.mfc)