Basic XML Writer in C++

Question

This is an XML Writer in C++. It doesn't do much but the basics, and I've tested it, so it should hold up well enough. I hope in the near future, I can bring you an XML Reader.

I kinda built the function names after the Visual Basic names for their standard XML Writer (Microsoft's anyways), so if you see the similarities, that's where I got the names from. As for the actual function internals, I don't know if they are the same or not.

XmlWriter.h:

#ifndef XmlWriter_H
#define XmlWriter_H
#include <fstream>
#include <iostream>
#include <string>
#include <vector>
class XmlWriter {
public:
 bool open(const std::string);
 void close();
 bool exists(const std::string);
 void writeOpenTag(const std::string);
 void writeCloseTag();
 void writeStartElementTag(const std::string);
 void writeEndElementTag();
 void writeAttribute(const std::string);
 void writeString(const std::string);
private:
 std::ofstream outFile;
 int indent;
 int openTags;
 int openElements;
 std::vector<std::string> tempOpenTag;
 std::vector<std::string> tempElementTag;
};
#endif

XmlWriter.cpp:

#include "XmlWriter.h"
//=============================================================================
//== Function Name : XmlWriter::exists
//==
//== Perameters
//== Name Type Description
//== ---------- ----------- --------------------
//== fileName const std::string The name of the file that is in use
//==
//== Description
//== --------------------------------------------------------------------------
//== This function is used to check if the XML file exists
//=============================================================================
bool XmlWriter::exists(const std::string fileName){
 std::fstream checkFile(fileName);
 return checkFile.is_open();
}
//=============================================================================
//== Function Name : XmlWriter::open
//==
//== Perameters
//== Name Type Description
//== ---------- ----------- --------------------
//== strFile const std::string The name of the file that the user passes
//== in the code
//==
//== Description
//== --------------------------------------------------------------------------
//== This function is used to open the XML file, first checking to see if it
//== exists first
//=============================================================================
bool XmlWriter::open(const std::string strFile) {
 if (exists(strFile)){
 std::cout << "Error: File alread exists.\n";
 return false;
 }
 outFile.open(strFile);
 if (outFile.is_open()) {
 std::cout << "File created successfully.\n";
 outFile << "<!--XML Document-->\n";
 outFile << "<?xml version='1.0' encoding='us-ascii'>\n";
 indent = 0;
 openTags = 0;
 openElements = 0;
 return true;
 }
 return false;
}
//=============================================================================
//== Function Name : XmlWriter::close
//==
//== Perameters
//== Name Type Description
//== ---------- ----------- --------------------
//== N/a N/a N/a
//==
//== Description
//== --------------------------------------------------------------------------
//== This function is used to close the XML file
//=============================================================================
void XmlWriter::close() {
 if (outFile.is_open()) {
 outFile.close();
 }
 else {
 std::cout << "File already closed.\n";
 }
}
//=============================================================================
//== Function Name : XmlWriter::writeOpenTag
//==
//== Perameters
//== Name Type Description
//== ---------- ----------- --------------------
//== openTag const std::string The name of the tag being created
//==
//== Description
//== --------------------------------------------------------------------------
//== This function creates a new tag, checking that the file is open, and saves
//== the tag name in a vector to keep track of it
//=============================================================================
void XmlWriter::writeOpenTag(const std::string openTag) {
 if (outFile.is_open()) {
 for (int i = 0; i < indent; i++) {
 outFile << "\t";
 }
 tempOpenTag.resize(openTags + 1);
 outFile << "<" << openTag << ">\n";
 tempOpenTag[openTags] = openTag;
 indent += 1;
 openTags += 1;
 }
 else {
 std::cout << "File is closed. Unable to write to file.\n";
 }
}
//=============================================================================
//== Function Name : XmlWriter::writeCloseTag
//==
//== Perameters
//== Name Type Description
//== ---------- ----------- --------------------
//== N/a N/a N/a
//==
//== Description
//== --------------------------------------------------------------------------
//== This function closes the currently open tag
//=============================================================================
void XmlWriter::writeCloseTag() {
 if (outFile.is_open()) {
 indent -= 1;
 for (int i = 0; i < indent; i++) {
 outFile << "\t";
 }
 outFile << "</" << tempOpenTag[openTags - 1] << ">\n";
 tempOpenTag.resize(openTags - 1);
 openTags -= 1;
 }
 else {
 std::cout << "File is closed. Unable to write to file.\n";
 }
}
//=============================================================================
//== Function Name : XmlWriter::writeStartElementTag
//==
//== Perameters
//== Name Type Description
//== ---------- ----------- --------------------
//== elementTag const std::string The name of the element being created
//==
//== Description
//== --------------------------------------------------------------------------
//== This function creates a new element tag and saves the name to a vector
//=============================================================================
void XmlWriter::writeStartElementTag(const std::string elementTag) {
 if (outFile.is_open()) {
 for (int i = 0; i < indent; i++) {
 outFile << "\t";
 }
 tempElementTag.resize(openElements + 1);
 tempElementTag[openElements] = elementTag;
 openElements += 1;
 outFile << "<" << elementTag;
 }
 else {
 std::cout << "File is closed. Unable to write to file.\n";
 }
}
//=============================================================================
//== Function Name : XmlWriter::writeEndElementTag
//==
//== Perameters
//== Name Type Description
//== ---------- ----------- --------------------
//== N/a N/a N/a
//==
//== Description
//== --------------------------------------------------------------------------
//== This function closed the currently opened element tag
//=============================================================================
void XmlWriter::writeEndElementTag() {
 if (outFile.is_open()) {
 outFile << "</" << tempElementTag[openElements - 1] << ">\n";
 tempElementTag.resize(openElements - 1);
 openElements -= 1;
 }
 else {
 std::cout << "File is closed. Unable to write to file.\n";
 }
}
//=============================================================================
//== Function Name : XmlWriter::writeAttribute
//==
//== Perameters
//== Name Type Description
//== ---------- ----------- --------------------
//== outAttribute const std::string The attribute being written out
//==
//== Description
//== --------------------------------------------------------------------------
//== This function writes an attribute (if any) after the element tag is first
//== opened and before the output for the element is written
//=============================================================================
void XmlWriter::writeAttribute(const std::string outAttribute) {
 if (outFile.is_open()) {
 outFile << " " << outAttribute;
 }
 else {
 std::cout << "File is closed. Unable to write to file.\n";
 }
}
//=============================================================================
//== Function Name : XmlWriter::writeString
//==
//== Perameters
//== Name Type Description
//== ---------- ----------- --------------------
//== writeString const std::string The string to be written to the element
//==
//== Description
//== --------------------------------------------------------------------------
//=============================================================================
void XmlWriter::writeString(const std::string outString) {
 if (outFile.is_open()) {
 outFile << ">" << outString;
 }
 else {
 std::cout << "File is closed. Unable to write to file.\n";
 }
}

main.cpp:

#include "XmlWriter.h"
int main() {
 XmlWriter xml;
 if (xml.open("C:\\Users\\UserNameHere\\Desktop\\test.xml")) {
 xml.writeOpenTag("testTag");
 xml.writeStartElementTag("testEle1");
 xml.writeString("This is my first tag string!");
 xml.writeEndElementTag();
 xml.writeOpenTag("testTag2");
 xml.writeStartElementTag("testEle2");
 xml.writeAttribute("testAtt=\"TestAttribute\"");
 xml.writeString("I sometimes amaze myself.");
 xml.writeEndElementTag();
 xml.writeOpenTag("testTag3");
 xml.writeStartElementTag("testEle3");
 xml.writeAttribute("testAtt2=\"TestAttrib2\"");
 xml.writeString("Though i'm sure someone can make something even better");
 xml.writeEndElementTag();
 xml.writeCloseTag();
 xml.writeCloseTag();
 xml.writeCloseTag();
 xml.close();
 std::cout << "Success!\n";
 } else {
 std::cout << "Error opening file.\n";
 }
 system("pause");
 return 0;
}

• Jamal: Updated question can be found here.

Answer 1

I'm going to second guess a bunch of decisions that you made. I don't know that you made them all incorrectly, but I think that alternatives deserve consideration.

std::ofstream outFile;

Why not make this an ostream instead? Then you could output to things other than just a file. An ofstream is a limiting choice here. My quick read is that the best choice would be to have both an XmlWriter and an XmlWriterFile which extends it.

int indent;

I'd call this current_indent for readability. Otherwise, one might guess that it reflects the overall indent rather than a value that changes over time.

int openTags;
int openElements;

If I had these variables, I'd call these something like openTagCount and openElementCount respectively. When I see a plural variable, I expect it to contain multiple things. That is, I expect plurals to indicate collections. These don't hold the open tags and elements, just the counts.

But I actually wouldn't have these variables. More in a moment.

std::vector<std::string> tempOpenTag;
std::vector<std::string> tempElementTag;

OK, these two vectors (along with the count variables) are used to implement stacks. Here's the thing though, why not just make them std::stack? You can even have the stack implementation use std::vector as the storage medium. That way, you don't have to worry about managing the stack variables as you do here.

void XmlWriter::writeCloseTag() {
 if (outFile.is_open()) {

This seems unlikely enough to not be checked. You already check that the file opens in the open method. It shouldn't close itself arbitrarily often enough that you would need to check it in each write function.

 indent -= 1;

It's more common to just say indent--; rather than to specify a decrement of 1.

 for (int i = 0; i < indent; i++) {
 outFile << "\t";
 }

Why not just

outFile << std::string(indent, '\t');

That saves you having to do indent calls to your stream. You also may want to make the indent character (\t here) a constructor option.

 outFile << "</" << tempOpenTag[openTags - 1] << ">\n";
 tempOpenTag.resize(openTags - 1);
 openTags -= 1;

You shouldn't resize a std::vector by 1. It's an expensive operation, so if you're going to do it, do it up large. A typical expansion is more like 50% or 100%. It's not evident that this program would need to make a vector smaller at all. Either the stack is going to grow again or it will end and the entire data structure can be released.

You do three things to implement a pop operation on your stack. If openTags was a stack variable, you could just say

outFile << "</" << openTags.top() << ">\n";
openTags.pop();

The pop() will trigger any resizes that may be necessary. You don't have to manage the number of open tags yourself.

 }
 else {
 std::cout << "File is closed. Unable to write to file.\n";
 }
}

This isn't necessary if you don't do the excess is_open check.

What you should be doing is to check if writeCloseTag is called when the stack is empty. If you use the stack variable, this looks something like

if ( openTags.empty() ) {
 // do something to handle this special case
 // maybe write to STDERR
 cerr << "No tag to close." << endl;
 return; // or exit or throw an exception
}

You can put that code at the beginning of the function.

You should also consider what should happen if writeCloseTag and writeEndElementTag are called in the wrong order. I don't see how your code would even notice. Perhaps open tags and elements should share the same stack rather than having two different stacks. Rather than storing them as strings, you could store them as objects of classes which extend the same class. That way you could know what is supposed to come next.

What would happen if writeString were called twice in a row? Consider adding logic to auto-close the element only if one is open rather than every time writeString is called. It should also check if it is currently in a context where a string can exist.

Check if an element tag is open before adding an attribute to it.

• CodeMonkey: Thanks for your feedback. I've implemented a few new things that you suggested and have posted an update to the code for review.

Answer 2

It's possible that I've missed things, but this all looks pretty good. The things that I'd highlight are that you have functions that can fail that are returning void. So for your

void writeOpenTag(const std::string);
void writeCloseTag();
void writeStartElementTag(const std::string);
void writeEndElementTag();
void writeAttribute(const std::string);
void writeString(const std::string); 

I'd make them either int or bool, depending on how you're feeling, and if you fail to open the file, or the file is already open, return a fail code, else return success, and have this in place of your cout << error message. If it turns out that there are enough cases where there's multiple reasons for failure, then you can return different error codes, so I'd lean towards an int return type.

• YoungJohn: The error handling could alternatively be done with exceptions, and no change would be necessary to the function signatures.
• Marco Acierno: I could also try to reduce nesting using fail fast checks ( for example the .is_open() check) and maybe instead of a Vector another collection class would fit better? Stack?

Answer 3

Coding style tidbits:

The first thing that bothers me in your code is the use of unnamed function parameters in the header declaration. I consider that to be a bad practice because the name of the parameter itself is a form of code documentation. You should provide the parameter names in the header file as well as in the source.

Your comments in the .cpp are quite verbose. Seems like you are following some documentation template. Most experienced programmers will dislike it. You should of course comment and document the code, but do it in a meaningful manner, not just in an automated fashion. Things that are too obvious don't need to be commented.

To make my point clear, take this line for example:

//== fileName const std::string The name of the file that is in use

Well, it is implicit that fileName is "The name of the file that is in use", so this should not be commented.

If there are any caveats about the filename, however, then it must be commented. For example, does the filename have to have a .xml extension at the end? What happens if it doesn't? This is the sort of things you should be commenting.

Missing constructors:

There is a thing in C++ which is called the Rule of Three (or five/zero). It is about how an object behaves regarding copy via constructors and assignment with operator =.

Also, regarding constructors, I would expect to see a constructor that takes the filename and opens the file for me without having to explicitly call open(). This would be nice:

XmlWriter xml("test.xml");
// ready to go if successful, an exception otherwise.

Error handling:

There were some other suggestions about possible alternatives to do more error handling in your writer. Error codes are somewhat OK, depending on the context. However, in a modern C++ library, as it seems to be the case with your code, exceptions are preferred for error reporting/handling.

When writing files, there is only a limited set of errors that can happen. An IO/Write error is very unlikely on modern hardware, so an exception seems like a very nice fit for such cases. Use exceptions for exceptional things.

So I'd vote against error codes and would suggest that you define a custom exception type (derived from std::exception) that your class throws on exceptional circumstances (file not open, IO error, etc). E.g.: an XmlWriterException type.

String parameters passed by const value:

When passing complex objects as function parameters, the worst possible way to do it is by const value. const disables move, so the compiler must issue a copy, even if you are just reading the param. If you then copy it to some storage inside the function, you end up doing two copies.

A good rule of thumb when passing objects as params is this:

• If you are only reading the parameter, pass it by const reference (const T &).

• If you are going to copy that object to somewhere inside the function, then pass it by value and apply std::move() on it.

Improvements to exists():

An fstream is not ideal for just checking if a file exists. But actually, XmlWriter::exists() will ALWAYS succeed, because the default open flags of fstream are ios_base::in | ios_base::out. So it will create a new file if it doesn't yet exists.

You should be using std::ifstream if your purpose is to only test if a file exists.

cout for error reporting:

This not a good choice for error logging/reporting. If you do implement exceptions, then the error message should be stored inside the exception object. Otherwise, cout is for normal program output. For error reporting, you use std::cerr (STDERR).

Answer 4

Along with what Yann4 said, you could create a typedef and macro for the errors. If you use one-hot style definitions, you can use convenient bitwise manipulation of errors, such as combining errors and later filtering for specific errors.

typedef int XmlStatus;
#define XML_STATUS_SUCCESS (XmlStatus) 0x01
#define XML_STATUS_ERROR_FILE_EXISTS (XmlStatus) 0x02
#define XML_STATUS_ERROR_FILE_CLOSED (XmlStatus) 0x04
#define XML_STATUS_ERROR_FILE_CANT_OPEN (XmlStatus) 0x08
#define XML_STATUS_ERROR_OTHER_THINGY (XmlStatus) 0x10

now you can define the functions like:

XmlStatus open(const std::string);
XmlStatus XmlWriter::open(const std::string strFile) {
 if (exists(strFile)){
 return XML_STATUS_ERROR_FILE_EXISTS;
 }
 outFile.open(strFile);
 if (outFile.is_open()) {
 outFile << "<!--XML Document-->\n";
 outFile << "<?xml version='1.0' encoding='us-ascii'>\n";
 indent = 0;
 openTags = 0;
 openElements = 0;
 return XML_STATUS_SUCCESS;
 }
 return XML_STATUS_ERROR_FILE_CANT_OPEN;
}

Reading the errors could be done like so, or handed to another function:

XmlStatus status = xml.open("C:\\Users\\UserNameHere\\Desktop\\test.xml");
if (status & XML_STATUS_SUCCESS)
{
 std::cout << "File created successfully.\n";
}
else if(status & XML_STATUS_ERROR_FILE_EXISTS)
{
 std::cout << "Error: File already exists.\n";
}
else if(status & XML_STATUS_ERROR_FILE_CANT_OPEN)
{
 std::cout << "Error: File could not be opened.\n";
}

• glampert: I think suggesting the use of macros is a very poor option for a C++ question. You should have suggested an enum or const int instead.

Answer 5

Your API has two problems:

bool exists(const std::string);

The exists function is useless and doesn't belong here. It should be replaced with a function error that checks whether an error has occurred.

void writeAttribute(const std::string);

This function must take two arguments: the name and the value of the attribute. Right now it is useless because the caller of the function must escape the strings and build the resulting string. This is exactly the thing your code should do.

Your implementation has other problems:

void XmlWriter::writeStartElementTag(const std::string elementTag) {
 if (outFile.is_open()) {
 for (int i = 0; i < indent; i++) {
 outFile << "\t";
 }
 tempElementTag.resize(openElements + 1);
 tempElementTag[openElements] = elementTag;
 openElements += 1;
 outFile << "<" << elementTag;
 }
 else {
 std::cout << "File is closed. Unable to write to file.\n";
 }
}

The problems are:

• Your class must not write to std::cout. The only allowed output channel is the file stream that has been created internally.
• tempElementTag makes openElements redundant since openElements == tempElementTag.size(), always.
• resize followed by [] can be replaced by a simple push_back(elementTag).

The writeString function is even worse. I expect that I can pass arbitrary strings to that function and that your class still generates well-formed XML.

Example strings that break everything:

<<<<<<<<<<<<<<<<<<
<script>alert(123)</script>
&unknown-entity;
&&&&&&&&&&&&&