Improve company productivity with a Business Account.Sign Up

x
  • Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 239
  • Last Modified:

Experts-please review my function and see if it is "good" enough..!

hi guys,

I have code a function that reads lines from a file into a buffer based on this file format:

H,1,2,3,4,5,6,7,8,9,0
D,1,2,3,4,5
T,3

H,1,2,3,4,5,6,7,8,9,0
D,1,2,3,4,5
T,3

...more records...
where it can also be:
H,1,2,3,4,
5,6,7,8,9,0
D,1,2
,3,4,5
T,3

where the number of fields is FIXED..(ie. H has 10 data fields, D has 5, T has 1)

My function reads one "record" into a buffer:

iErr = DoGetBuffer(szFile,SZ_NOFIELD1, szBuffer1); /* for H */
iErr = DoGetBuffer(szFile,SZ_NOFIELD2, szBuffer2); /* for D */
iErr = DoGetBuffer(szFile,SZ_NOFIELD3, szBuffer3); /* for T */

Note that I use the no of commas as an indicator. This function is very important and there cannot be any bugs in it... can u guys please go through it and comment/improve upon it so that it is "perfect" for production use?

Thanzs!

--------------------------------------
int CMyClass::DoGetBuffer(CFile * myFile, int NoFields, char *buffer)
{
      int iComma;
      char szElement;
      DWORD dwRead;
      int iLine;
      // initialize
      iLine = 0;
      iComma = 0;
      szElement = NULL;

    do
    {
        dwRead =  myFile->Read(&szElement, 1);
            
            // check if end of record
            if ( \
                  ((szElement == '\r')&&(iComma == (NoFields - 1)))|| \
                  ((dwRead  <= 0)&&(iComma == (NoFields - 1))) \
               )      
            {      // should check for record_type in the next line
                        buffer[iLine] = '\0';
                        return 0;
            }

            // ignore newline and cr
            if ((szElement != '\n')&&(szElement != '\r'))
            {
            buffer[iLine] = szElement;
            if (buffer[iLine] == ',')  // one comma add
                  iComma++;
            iLine++;
            };
            

    }
    while (dwRead > 0);
      // dwRead returned <= 0
      return -1;

}
----------------------------------
0
Haho
Asked:
Haho
1 Solution
 
ShaunWildeCommented:
you may want to add some exception handling as CFile::Read(...) can throw a CFileException .

You may also wish to put in some tests on buffer i.e. make sure it is not NULL and that you do not exceed its bounds (you may have to pass in the size of the buffer)
0
 
wasanCommented:
Hi,

Make sure that you have only one return
statement in your function.

You can make the following change to do it.

int iRc = -1;


    do
    {
        dwRead =  myFile->Read(&szElement, 1);

// check if end of record
if ( \
((szElement == '\r')&&(iComma == (NoFields - 1)))|| \
((dwRead  <= 0)&&(iComma == (NoFields - 1))) \
   )
{ // should check for record_type in the next line
buffer[iLine] = '\0';
iRc = 0;  // changed by wasan
}
//follwing two lines added by wasan
if ( iRc == 0 )
break;
// ignore newline and cr
if ((szElement != '\n')&&(szElement != '\r'))
{
buffer[iLine] = szElement;
if (buffer[iLine] == ',')  // one comma add
iComma++;
iLine++;
};

    }
    while (dwRead > 0);
// dwRead returned <= 0
return iRc; //changed by wasan

bye,
wasan.

   
0
 
DKostovCommented:
Why you don't make "char* buffer" input
parameter "std::string &buffer" or with MFC-"CString &buffer"? In this
case you'll never exceed the buffer's memory limits.
0
Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.

Join & Write a Comment

Featured Post

Upgrade your Question Security!

Your question, your audience. Choose who sees your identity—and your question—with question security.

Tackle projects and never again get stuck behind a technical roadblock.
Join Now