Re: return string
- From: Joseph M. Newcomer <newcomer@xxxxxxxxxxxx>
- Date: Sun, 20 Apr 2008 17:57:53 -0400
As several people have pointed out, there are many problems here.
See below...
On Wed, 16 Apr 2008 17:18:07 -0700, Eric Kaplan <tobycraftse@xxxxxxxxx> wrote:
I have a function like this****
=============
bool Table::Get(char* FieldName, char* FieldValue)
The only correct way to write this in modern programming style is to make it Unicode-aware
and independent of buffer length
bool Tabe::Get(const CString & FieldName, CString & FieldValue)
the const tells you that the field name is not going to be modified, the CString &
combined with const maximizes the efficiency; you could also use LPCTSTR, but NEVER
'char*'. If the FieldName is not modifiable, modern best practice is to make sure the
const attribute is added.
You could consider writing it as
bool Table::Get(LPCTSTR FieldName, LPTSTR FieldValue, SIZE_T len)
and make sure that under no conditions can you EVER copy more than len-1 characters into
the FieldValue, but anything less than that is now considered Unacceptable Practice. It
is your responsibility to make sure you do not exceed the buffer size. It is NEVER
acceptable to fail to pass a buffer size under such conditions.
*****
****
and I call the function like the follow
=============
while(!tbl.ISEOF())
{
char id2[300];
When you write code like this, you need to *instantly* say "This is a design error". There
are very few contexts in which you would ever write an array of fixed bounds (MAX_PATH for
file paths is one of the few places, and it can break for long Unicode file names but we
rarely have a choice in the matter given some of the APIs, which will not give us the
actual length required). There is no reason I can imagine that under these circumstances
to
(a) use the obsolete data type 'char' for any purpose
(b) use a fixed size array
(c) think that 300 made sense and not provide an explicit #define or static const int to
define it
****
if(tbl.Get("program",id2))****
First parameter is bad: it uses 8-bit character data. Where is the Unicode-awareness?
_T("program")
Second parameter is bad: you are passing a pointer to a string of unknown length
Third parameter is bad: you failed to pass the length of the buffer! (Yes, I know there
isn't a third parameter, and that's where this code is absolutely worst-case design!)
NEVER pass a pointer to a buffer without passing the size!!!!! (This kind of thing can
get you fired from some companies these days)
****
cout<<"program:"<<id2<<endl;****
What is cout doing in an MFC program? Where is this message going?
****
else****
{
tbl.GetErrorErrStr(ErrStr);
cout<<"\n"<<ErrStr<<"\n";
break;
}
}
above I passed in a char pointer (char*) - FieldValue and use it just
like a return value.
No, you are not using it as a return value; you are using it as a buffer to be filled in.
****
****
but it seem not efficient as each time i initialize a char array of
[300] characters.
You do? Where? I see no array being initialized! I don't even see one being allocated
'each time' (it is allocated ONCE when the function containing the declaration is called,
no matter how deep the declaration is nested!)
****
****
but some string (FieldValue) is only like [5] char.
If you are concerned about 'efficiency', you need to worry when you are actually spending
time doing something. This means that, if you are reading data from a database or file,
that it is likely that the transport time to get the data to your program is four to six
orders of magnitude slower than, say, having a full-fledged CString variable allocated and
initialized. A sense of proportion is appropriate here. Far too many people make code
"efficient" by getting rid of all those annoying features, like code safety, and
eventually these programs make headlines, such as "Virus takes over 300,000 machines in
the first hour by exploiting a buffer overflow in <your program name here>" You do not
want to have your program name there. Forget "efficiency" when it compromises safety; if
efficiency really does matter, you need to CAREFULLY rethink your algorithm, but in this
case, it is a completely pointless concern
****
****
how to I do better for returning string? any example code?
=================
should I just return the string in regular return type like -
std::string Table::Get(char* FieldName)
Why use std::string in MFC code? It serves no particularly useful purpose, and is not
nearly as good as CString. CString Plays Well With Others, whereas std::string plays well
with no one. If I were doing this, I might consider
CString Table::Get(const CString & FieldName)
or
CString Table::Get(LPCTSTR FieldName)
but as soon as you write 'char', you've already lost. 'char' is a dead data type except
in very rare and exotic circumstances, and this does not appear to be such a case.
It is essential to develop good programming habits, and Unicode is really the Only Way To
Go. At various points, such as talking to specialized devices that send 8-bit characters,
you might use 8-bit characters, and at the "borders" of the code, such as in reading and
writing files, you might use 8-bit UTF-8 encoding, but think of 'char' as obsolete, think
of fixed char or TCHAR arrays as almost completely obsolete, think of uncounted buffer
points as Just Plain Wrong. The above code is almost a showpiece of bad programming
style.
joe
*****
Joseph M. Newcomer [MVP]
email: newcomer@xxxxxxxxxxxx
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
.
- Prev by Date: I received my Corum Bubble Dive Laquered Dial With Devil Illustration I Chronograph Automatic - http://www.salereplicawatch.com CR001 watch and I’m blown away at the beauty of it. Watch is exactly as described, and so very impressed with customer service. Sharp, professional and honest from the start through the end of the transaction. I was sceptical about buying a replica watch and having it sent to the UK, however I can assure any other potential buyer that you will not be let down. I was delighted with my Corum Bubble Dive Laquered Dial With Devil Illustration I Chronograph Automatic - CR001 that came straight to my workplace in less than 5 days. Communications were excellent too…
- Next by Date: Re: Can't get OLE interface
- Previous by thread: Re: return string
- Next by thread: CFileFind
- Index(es):
Relevant Pages
|
|
