Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Also any inputs on how to improve program design/structure would be very much appreciated. Kindly excuse me if the program is long . This is my first post here. less of code comments as i am hoping the code is readable as is. I can probably just print the xml using streams but i have other reasons for using the library.

The header file

#ifndef GEN_XML
#define GEN_XML

#include <iostream>
#include <fstream>
#include <string>
#include <algorithm>
#include <vector>
#include <sstream>

typedef std::string stringType;
typedef std::vector<stringType> stringVector;
typedef std::vector<int> integerVector;

class gen_xml
{
private:
    int previousTime;
    int tempTime;
public:
    stringVector text_file;
    integerVector time_ms;
    stringVector text_info;
public:
    gen_xml():previousTime(1),tempTime(2) {};

    virtual int validate(int argc,char* argv[]);

    virtual void showInput(int argc,char* argv[]);

    virtual bool validateFileExtention(stringType fname,stringType ext);

    virtual int getAbsoluteTime(stringType value);

    virtual void getData(char* argv[]);

    virtual stringType toString(int num);

    virtual void generateXML(stringVector text_file,integerVector time_ms,char* FileName);

};

#endif  // GEN_XML

The CPP file

#include "tinyxml.h"
#include "tinystr.h"
#include "gen_xml.h"
using namespace std;


int main(int argc,char* argv[])
{
    gen_xml req;
    if(req.validate(argc,argv))
    {
        req.getData(argv);
        req.generateXML(req.text_info,req.time_ms,argv[2]);
    }
    cout<<"Done"<<endl;
    return 0;
}
void gen_xml::generateXML(stringVector text_file,integerVector time_ms,char* FileName)
{
    TiXmlDeclaration* declaration = new TiXmlDeclaration("1.0", "UTF-8", "no");//Create DTD
    TiXmlDocument* doc = new TiXmlDocument;
    doc->LinkEndChild(declaration);

    TiXmlElement* msg;

    TiXmlElement * root = new TiXmlElement( "tags" );
    doc->LinkEndChild( root );
    for(int i=0; i<(int)text_file.size(); i++)
    {
        TiXmlElement * msgs = new TiXmlElement( "metatag" );
        msgs->SetAttribute("event", "onCuePoint");
        msgs->SetAttribute("overwrite", "true");
        root->LinkEndChild( msgs );

        msg = new TiXmlElement( "name" );

        msg->LinkEndChild( new TiXmlText("CuePoint"+toString(i) ));
        msgs->LinkEndChild( msg );

        msg = new TiXmlElement( "timestamp" );
        msg->LinkEndChild( new TiXmlText(toString((int)time_ms.at(i))));
        msgs->LinkEndChild( msg );

        msg= new TiXmlElement( "parameters" );
        msgs->LinkEndChild(  msg );
        TiXmlElement * _params = new TiXmlElement( "textinfo" );
        _params->LinkEndChild(new TiXmlText( text_info.at(i)));
        msg->LinkEndChild( _params );

        msg= new TiXmlElement( "type" );
        msg->LinkEndChild( new TiXmlText("navigation"));
        msgs->LinkEndChild(  msg );
    }


    doc->SaveFile( FileName );
}
string gen_xml::toString(int num)
{
    stringstream abc;
    string value;
    abc<<num;
    value=abc.str();
    return value;
}

int gen_xml::validate(int argc,char* argv[])
{
    fstream filestr;
    string temp;
    bool result;
    if(argc>3)
    {
        cerr<<"Input Arguments Exceeded"<<endl;
        return 0;
    }

    if(validateFileExtention(argv[1],".txt")&&validateFileExtention(argv[2],".xml"))
    {
        filestr.open(argv[1]);

        if (filestr.is_open())
        {
            filestr.close();
            result=true;
        }
        else
        {
            cout << "Error opening file";
            result=false;
        }
    }
    return result;
}
void gen_xml::getData(char* argv[])
{
    fstream filestr;
    string temp;
    filestr.open(argv[1]);
    while( getline( filestr, temp ))
    {
        text_file.push_back( temp );
    }
    //cout<<(int)text_file.at(0).size()<<endl;
    //getAbsoluteTime(text_file.at(0).substr(0,12));

    for(int i=0; i<(int)text_file.size(); i++)
    {
        time_ms.push_back(getAbsoluteTime(text_file.at(i).substr(0,12)));
        temp=text_file.at(i).substr(13,(int)text_file.at(i).size()-14);
        text_info.push_back(temp);
    }
    filestr.close();
}
int gen_xml::getAbsoluteTime(string value)
{
    int hours,minutes,milliseconds;
    int absTime;

    (stringstream)value.substr(3,2)>>hours;
    (stringstream)value.substr(6,2)>>minutes;
    (stringstream)value.substr(9,3)>>milliseconds;
    absTime=60*1000*(hours*60+minutes)+  milliseconds;

    //-- stupid fix for a tool... dont ask why
    tempTime=absTime/1000;
    if(previousTime==tempTime)
    {
        absTime=(tempTime+1)*1000;
    }
    previousTime=tempTime;
    //--
    return absTime;
}
bool gen_xml::validateFileExtention(string name,string ext)
{
    string str (name);
    string str2 (ext);

    size_t found;
    bool res;
    int num_periods = count(name.begin(), name.end(), '.');
    found=str.find(str2);
    if ((found!=string::npos)&&(num_periods==1))
    {
        //cout << "file extention"<<str2<<"found at: " <<int(found) << endl;
        res=true;
    }
    else
    {
        // cout<<"file name incorrect"<<endl;
        res=false;
    }
    return res;
}
void gen_xml::showInput(int argc,char* argv[])
{
    cout<<"argc = "<<argc<<"\t";
    for (int i = 0; i<argc; i++)
    {
        cout<<"argv["<<i<<"] = "<< argv[i]<<"\n";
    }
}
share|improve this question
add comment

1 Answer

I would remove the validate method and do some modifications:

About validate method: - Check argc and argv outside the class, just provide the filename to the getData method. - You also need to check when argc is less than 2, the user may forget to supply the filename. - Openning the file in the validate method and closing it again does not guarantee that it will be still available when you call getData, so it is better to open the file only there and handle any errors there.

About getData:

  • I would rename it to loadData
  • for each line loaded you insert a value in time_ms and text_info vectors, call reserve before the loop to pre-allocate the needed memory.
  • no need to call close on the stream, it will close when its scopes end

validateFileExtention

  • use rfind instead of find and just grab the last "." from the string.
  • I would instead create a function to copy the fileExtension and check both extensions at once, instead of finding the extension each time.
  • also I would not even bother about file extension at all, I would just let the parser validate it.

About generateXML:

  • memory leak: you never delete doc, also, if you need it just inside the method, why you use dynamic memory? Just create it on stack
  • also, check if tinyxml deletes everything that you insert on the document, if not, you will have to handle this too.

Thats all that I could figure out.

Cheers

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.