Generating XML file from text input

Question

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.

Please offer any inputs on how to improve program design/structure.

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"; } } 

Answer 1

I would remove the validate method and do some modifications:

About validate:

• 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.
• Opening 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.

About 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.
• I would also 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.

Answer 2

Header file:

• Since your variables and functions are in camelCase, your own types, especially gen_xml, should be in PascalCase. It shouldn't be easy to mix up all of these based on their naming conventions.

• If you aren't using a particular library in a header file, don't include it. Header files should include as few things as possible because anything including those header files will also be exposed to those libraries. If any libraries are just needed in the implementation file, then keep them there.

  You're only using <string> and <vector> in the header file, so move the rest of the libraries to the implementation file.

• The typedefs in the header file don't seem too useful, and they don't really save any keystrokes. If a type is not very long and you cannot give it a shorter (and distinctive) name, then you don't need to use a typedef.

• You shouldn't have any public data members in a class. All such data members should be private. If you must allow them to be changed outside the class after object-construction), then have setters for any individual ones.

• I don't think any of these functions need to be virtual. There's no inheritance or polymorphism being used in this program.

• showInput() should be const since it's not modifying any data members.

Implementation file:

• The final output in main() doesn't look necessary. If you're executing this on the command line, you'll already receive a prompt for the next command. Plus, it's a little misleading if the file failed to open in the first place.

• validate() isn't a very descriptive name as it doesn't specify what it's validating.

  It appears to be checking two different things: the number of command line arguments and the file extensions. You should probably have main() validate the former and validate() the latter, then terminate if any of them fails. You should also rename validate() to something more descriptive, such as validateFilename().

• showInput() should be displaying data members, not command line arguments. Otherwise, it has no business being a member function.

• This cast to int is useless:

  for(int i=0; i<(int)text_file.size(); i++) 

  The function size() already returns an std::size_type, which is an integer type. You don't necessarily need to loop towards an int value; any integer type will do.

  Moreover, don't use C-style casting in C++ unless it's absolutely necessary. For such casts, use the C++-style cast static_cast<>().

  static_cast<SomeType>(someVariable); 

• getAbsoluteTime()'s argument, value, should probably be passed by const& as it isn't being modified within the function.

• If you have C++11, use std::to_string() instead of your own toString() function.