Basic XML Writer in C++

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 
#include 
#include 
#include 

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 tempOpenTag;
    std::vector 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
//== ---------- ---------

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 tempOpenTag;
std::vector 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 \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 \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.

StackExchange Code Review Q#67334, answer score: 6