dcsimg
CodeGuru Home VC++ / MFC / C++ .NET / C# Visual Basic VB Forums Developer.com
Results 1 to 6 of 6

Thread: Can someone look over my code ... ?

  1. #1
    Join Date
    May 2000
    Location
    England
    Posts
    574

    Question Can someone look over my code ... ?

    Hi
    I am writing a parser and i feel that my code could do with a look over by someone to advise me if any of it could be done better ..
    more profesional ?

    if someone can look over this and advise me how it can be done better that would be great .

    this is the relevent functionality

    all variables have been declared globally in the .h file

    void COTFParserDlg::OnParseData()
    {
    // Functionality to parse the data file //


    file.Open(m_strDefaultDir,CFile::modeReadWrite);

    int nBytesRead = file.GetLength();

    if (file == FALSE && nBytesRead == 0)
    {
    AfxMessageBox("Could not read data from file!");
    }

    while (file.ReadString(strLine) && nBytesRead > 0)
    {
    nBytesEaten = strLine.GetLength();

    nEquals = strLine.Find("=");
    nRemainder = nBytesEaten - nEquals;
    nHeader = nRemainder -1;

    CString strRemainder;
    CString strRemLeft;
    CString strRemData;
    strRemainder = strLine.Right(nHeader);
    strRemLeft = strLine.Left(nEquals);
    strRemData = strLine.Left(2);

    // Test the Header of the data //

    if (strRemLeft == "*MAJOR")
    {
    // MAJOR data //
    CString strMajor = "The Main version number is:";
    strMajor += strRemainder;
    strDataString = strMajor;
    WriteFile();
    }
    if (strRemLeft == "*MINOR")
    {
    // MINOR data //
    CString strMinor = "The Sub version number is:";
    strMinor += strRemainder;
    strDataString = strMinor;
    WriteFile();
    }
    if (strRemLeft == "*FORMAT")
    {
    // FORMAT data
    CString strHeaderFormat = "The user Data Format is:";
    strHeaderFormat += strRemainder;
    strDataString = strHeaderFormat;
    WriteFile();
    }
    if (strRemLeft == "*RQID")
    {
    // RQID data //
    CString strRQID = "The Number of Spool requests is:";
    strRQID += strRemainder;
    strDataString = strRQID;
    WriteFile();
    }
    if (strRemLeft == "*RQOWNER")
    {
    // RQOWNER data //
    CString strRQOWNER = "The Creator of the Spool request is:";
    strRQOWNER += strRemainder;
    strDataString = strRQOWNER;
    WriteFile();
    }
    if (strRemLeft == "*RQCLIENT")
    {
    // RQCLIENT data //
    CString strRQCLIENT = "The Client in which the Spool request was created is:";
    strRQCLIENT += strRemainder;
    strDataString = strRQCLIENT;
    WriteFile();
    }
    if (strRemLeft == "*RQCREATIME")
    {
    // RQCREATIME data //
    CString strRQCREATIME = "The Creation time of the Spool request is:";
    strRQCREATIME += strRemainder;
    strDataString = strRQCREATIME;
    WriteFile();
    }
    if (strRemLeft == "*RQNAME")
    {
    // RQNAME data //
    CString strRQNAME = "The First part of the Three part Spool name Request is:";
    strRQNAME += strRemainder;
    strDataString = strRQNAME;
    WriteFile();
    }
    if (strRemLeft == "*RQSUFFIX1")
    {
    // RQSUFFIX1 data //
    CString strRQSUFFIX1 = "The Second part of the Three part Spool name Request is:";
    strRQSUFFIX1 += strRemainder;
    strDataString = strRQSUFFIX1;
    WriteFile();
    }
    if (strRemLeft == "*RQSUFFIX2")
    {
    // RQSUFFIX2 data //
    CString strRQSUFFIX2 = "The Third part of the Three part Spool name Request is:";
    strRQSUFFIX2 += strRemainder;
    strDataString = strRQSUFFIX2;
    WriteFile();
    }
    if (strRemLeft == "*RQORIGDEST")
    {
    // RQORIGDEST data //
    CString strRQORIGDEST = "The Original output device for the Spool request is:";
    strRQORIGDEST += strRemainder;
    strDataString = strRQORIGDEST;
    WriteFile();
    }
    if (strRemLeft == "*DVCODEPAGE")
    {
    // DVCODEPAGE data //
    CString strDVCODEPAGE = "The Character set number of the Output device in the Print request is:";
    strDVCODEPAGE += strRemainder;
    strDataString = strDVCODEPAGE;
    WriteFile();
    }
    if (strRemLeft == "*DVDEVTYPE")
    {
    // DVDEVTYPE data //
    CString strDVDEVTYPE = "The Device Type of Output device PJPRINTER is:";
    strDVDEVTYPE += strRemainder;
    strDataString = strDVDEVTYPE;
    WriteFile();
    }
    if (strRemLeft == "*DVORIGDEVTYPE")
    {
    // DVORIGDEVTYPE data //
    CString strDVORIGDEVTYPE = "The Device type of the Original output device RQORIGDEST is:";
    strDVORIGDEVTYPE += strRemainder;
    strDataString = strDVORIGDEVTYPE;
    WriteFile();
    }
    if (strRemLeft == "*PJAMOUNT")
    {
    // PJAMOUNT data //
    CString strPJAMOUNT = "The Number of Copies is:";
    strPJAMOUNT += strRemainder;
    strDataString = strPJAMOUNT;
    WriteFile();
    }
    if (strRemLeft == "*PJCLIENT")
    {
    // PJCLIENT data //
    CString strPJCLIENT = "The Client in which the print Request was created is:";
    strPJCLIENT += strRemainder;
    strDataString = strPJCLIENT;
    WriteFile();
    }
    if (strRemLeft == "*PJDEPARTMENT")
    {
    // PJDEPARTMENT data //
    CString strPJDEPARTMENT = "The Department of Print request recipient is:";
    strPJDEPARTMENT += strRemainder;
    strDataString = strPJDEPARTMENT;
    WriteFile();
    }
    if (strRemLeft == "*PJFORM")
    {
    // PJFORM data //
    CString strPJFORM = "The Formatting type for the print request is:";
    strPJFORM += strRemainder;
    strDataString = strPJFORM;
    WriteFile();
    }

    // Tests the first 2 Characters in the string //

    if (strRemData == "//")
    {
    // Begin or End of SAP OTF Data flow
    Data();
    CString strDataHeading = "This is the Heading of the Data: ";
    strDataHeading += strDataString;
    strDataString = strDataHeading;
    WriteFile();
    }
    if (strRemData == "/P")
    {
    // Set current position
    Data();
    CString strCurPosition = "This sets the Current position of the Data: ";
    strCurPosition += strDataString;
    strDataString = strCurPosition;
    WriteFile();
    }



    nBytesRead -= nBytesEaten;
    }
    file.Close();
    strLine.Empty();

    }


    void COTFParserDlg:ata()
    {
    // This function Grabs the data from the string provided by SAP OTF //

    int nLength = strLine.GetLength();
    nNewLength = nLength -1;
    strDataString = strLine.Mid(2,nNewLength);

    }

    void COTFParserDlg::WriteFile()
    {
    // This function takes all the strings and create a meaningfull file //
    // Showing what is required and where it should go //

    char* pFileName = "ParsedData.txt";
    CString strOutput;
    strOutput += strDataString;

    Writefile.Open( pFileName, CFile::modeCreate | CFile::modeWrite | CFile::modeNoTruncate | CFile::typeText);
    Writefile.SeekToEnd();
    Writefile.WriteString(strOutput +"\n");
    Writefile.Flush();
    Writefile.Close();

    }

    Thanks

    P

  2. #2
    Join Date
    Apr 1999
    Posts
    3,585
    Take a look at AfxExtractSubString() on MSDN. It's an undocumented function that should make your program much easier.

  3. #3
    Join Date
    May 2000
    Location
    England
    Posts
    574

    Thanks for that .....

    But i was really after some input about how good/bad
    my logic/functionality is
    and anything that i need to look at and Improve


    Thanks again

    P

  4. #4
    Join Date
    Sep 2002
    Location
    DC Metro Area, USA
    Posts
    1,509
    The first thing that comes to mind is that you could do something like:

    char typeStr;

    typeStr = strRemLeft[0];

    // if the string can start with other chars than * and / use a switch

    if (typeStr == '*')
    {
    ProcessAsterickFileCode(strRemLeft); // method contains all the if's for strings starting with '*'
    }
    else
    {
    if (typeStr == '/')
    {
    ProcessSlashFileCode(strRemLeft); // method contains all the if's for strings starting with '/'
    }
    else
    {
    // Error handling?????
    }
    }

    You could apply the same thing to the second char, if you to speed up some of the checking (this also incorporates one of the suggestions below)

    char typeLetter;
    typeLetter = = strRemLeft[1];

    switch (typeLetter)
    {
    case 'M':
    if (strRemLeft == "*MAJOR")
    {
    // MAJOR data //
    strDataString = "The Main version number is:";
    }
    else if (strRemLeft == "*MINOR")
    {
    // MINOR data //
    strDataString = "The Sub version number is:";
    }
    break;

    case 'R':
    // Same sort of thing
    ....

    It also seems that all of the then clauses are pretty much the same i.e.
    str = "some unique stirng";
    str += strRemainder;
    strData = str
    WriteFile()

    you could factor out the last 3 lines to the bottom of the loop and just use the same var to assign the unique string to:


    if (strRemLeft == "*MAJOR")
    {
    // MAJOR data //
    strDataString = "The Main version number is:";
    }
    if (strRemLeft == "*MINOR")
    {
    // MINOR data //
    strDataString = "The Sub version number is:";
    }
    ...
    strDataString += strRemainder;
    WriteFile();

    Minor preformance enhancement would be to do this:

    if (strRemLeft == "*MAJOR")
    {
    // MAJOR data //
    strDataString = "The Main version number is:";
    goto WriteGoodData;
    }
    if (strRemLeft == "*MINOR")
    {
    // MINOR data //
    strDataString = "The Sub version number is:";
    goto WriteGoodData;
    }
    ...(other if's, etc...)

    WriteGoodData:
    strDataString += strRemainder;
    WriteFile();

    --OR--

    if (strRemLeft == "*MAJOR")
    {
    // MAJOR data //
    strDataString = "The Main version number is:";
    }
    else if (strRemLeft == "*MINOR")
    {
    // MINOR data //
    strDataString = "The Sub version number is:";
    }
    else
    ...(other if's, etc...)

    strDataString += strRemainder;
    WriteFile();

    Both will avoid checking after you've already found the right string.

    Don't foget to handle stings that don't match what you're expecting... it may not be an issue, but it certainly won't hurt
    bytz
    --This signature left intentionally blank--

  5. #5
    Join Date
    May 2000
    Location
    England
    Posts
    574

    Thanks

    I must admit i did think of breaking it up and making it more function calls ....
    but thanks for the advice !

    still got alot to learn but at least i love my job


  6. #6
    Join Date
    Sep 2002
    Posts
    1,747
    I would put your keywords in a map as the keys and the individual functions to dispatch to as function pointer values. Not only is this much easier to maintain if/when more keywords are added or functionality changes, but lookup time is only O(ln n), not O(n).
    */*/*/*/*/*/*/*/*/*/*/*/*/*/*/*/*/*/

    "It's hard to believe in something you don't understand." -- the sidhi X-files episode

    galathaea: prankster, fablist, magician, liar

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  


Windows Mobile Development Center


Click Here to Expand Forum to Full Width




On-Demand Webinars (sponsored)