XML Writer in C++ - Updated

Question

I recently posted about a basic XML writer in C++ and got a lot of great feedback. Well, I'm back with an updated version of the XML writer that is a bit less basic, but hope it's better than the previous. I've implemented a stack, better handling of the string writing and attributes, and default and custom constructors. I have yet to implement better error checking, but I am aware and am looking into it.

StackADT.h

#ifndef StackADT_H
#define StackADT_H

template<class Type>
class StackADT {

public:
    virtual void initializeStack() = 0;
    virtual bool isEmptyStack() const = 0;
    virtual bool isFullStack() const = 0;
    virtual void push(const Type& newItem) = 0;
    virtual Type top() const = 0;
    virtual void pop() = 0;

};

#endif

LinkedStack.h

#ifndef LinkedStack_H
#define LinkedStack_H

#include <iostream>
#include <cassert>

#include "StackADT.h"

template <class Type>
struct nodeType {
    Type info;
    nodeType<Type> *link;
};

template <class Type>
class LinkedStack : public StackADT < Type > {

public:
    const LinkedStack <Type>& operator=(const LinkedStack<Type>&);
    bool isEmptyStack() const;
    bool isFullStack() const;
    void initializeStack();
    void push(const Type& newItem);
    Type top() const;
    void pop();
    LinkedStack();
    LinkedStack(const LinkedStack<Type>& otherStack);
    ~LinkedStack();

private:
    nodeType<Type> *stackTop;
    void copyStack(const LinkedStack<Type>& otherStack);

};

#include "LinkedStack.tpp"

#endif

LinkedStack.tpp

#include <iostream>

template <class Type>
const LinkedStack<Type>& LinkedStack<Type>::operator=(const LinkedStack<Type>& otherStack) {
    if (this != &otherStack)
        copyStack(otherStack);

    return *this;
}

template <class Type>
LinkedStack<Type>::LinkedStack() {
    stackTop = NULL;
}

template <class Type>
LinkedStack<Type>::LinkedStack(const LinkedStack<Type>& otherStack) {
    stackTop = NULL;
    copyStack(otherStack);
}

template <class Type>
LinkedStack<Type>::~LinkedStack() {
    initializeStack();
}

template <class Type>
void LinkedStack<Type>::initializeStack() {
    nodeType<Type> *temp;

    while (stackTop != NULL) {

        temp = stackTop;
        stackTop = stackTop->link;
        delete temp;

    }
}

template <class Type>
bool LinkedStack<Type>::isEmptyStack() const {
    return (stackTop == NULL);
}

template <class Type>
bool LinkedStack<Type>::isFullStack() const {
    return false;
}

template <class Type>
void LinkedStack<Type>::push(const Type& newElement) {
    nodeType<Type> *newNode;

    newNode = new nodeType<Type>;
    newNode->info = newElement;
    newNode->link = stackTop;
    stackTop = newNode;
}

template <class Type>
Type LinkedStack<Type>::top() const {
    assert(stackTop != NULL);
    return stackTop->info;
}

template <class Type>
void LinkedStack<Type>::pop() {
    nodeType<Type> *temp;

    if (stackTop != NULL) {
        temp = stackTop;
        stackTop = stackTop->link;
        delete temp;
    }
    else {
        std::cout << "Cannot remove from an empty stack.\n";
    }
}

template <class Type>
void LinkedStack<Type>::copyStack(const LinkedStack<Type>& otherStack) {
    nodeType<Type> *newNode, *current, *last;

    if (stackTop != NULL)
        initializeStack();

    if (otherStack.stackTop == NULL)
        stackTop = NULL;
    else {
        current = otherStack.stackTop;
        stackTop = new nodeType<Type>;
        stackTop->info = current->info;
        stackTop->link = NULL;
        last = stackTop;
        current = current->link;

        while (current != NULL) {
            newNode = new nodeType<Type>;
            newNode->info = current->info;
            newNode->link = NULL;
            last->link = newNode;
            current = current->link;
        }
    }
}

XmlWriter.h

#ifndef XmlWriter_H
#define XmlWriter_H

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

#include "LinkedStack.h"

class XmlWriter {

public:
    XmlWriter();
    XmlWriter(std::string fileName);
    XmlWriter(std::string fileName, std::string xmlEncoding);
    void close();
    bool exists(std::string);
    bool isOpen();
    void writeStartDocument();
    void writeEndDocument();
    void writeStartElement(std::string);
    void writeEndElement();
    void writeAttribute(std::string);
    void writeString(std::string);

private:
    LinkedStack<std::string> tagStack;
    std::ofstream outFile;
    std::string xmlEncode;
    int current_indent;
    bool startDocument;
    bool docWrite;
    bool firstStartElement;
    bool elementOpen;
    bool stringWritten;

};

#endif

XmlWriter.cpp

#include <Windows.h>
#include <lmcons.h>

#include "XmlWriter.h"

XmlWriter::XmlWriter() {
    char username[UNLEN + 1];
    DWORD username_len = UNLEN + 1;
    GetUserName(username, &username_len);

    std::string strUserName(username);

    std::string fileName = "C:\\Users\\" + strUserName + "\\My Documents\\DefaultXml.xml";
    std::string xmlEncoding = "utf-8";

    if (!(exists(fileName))) {
        outFile.open(fileName);
        if (outFile.is_open()) {
            std::cout << "File created successfully.\n";
            current_indent      = 0;
            startDocument       = false;
            docWrite            = false;
            firstStartElement   = true;
            elementOpen         = false;
            stringWritten       = false;
            xmlEncode           = xmlEncoding;
        }
    } else {
        std::cerr << "Default File Exists.";
    }

}


XmlWriter::XmlWriter(std::string fileName) {
    std::string xmlEncoding = "utf-8";

    if (!(exists(fileName))) {
        outFile.open(fileName);
        if (outFile.is_open()) {
            std::cout << "File created successfully.\n";
            current_indent = 0;
            startDocument = false;
            docWrite = false;
            firstStartElement = true;
            elementOpen = false;
            stringWritten = false;
            xmlEncode = xmlEncoding;
        }
    }
    else {
        std::cerr << "File already exists.";
    }
}


XmlWriter::XmlWriter(std::string fileName, std::string xmlEncoding) {
    if (!(exists(fileName))) {
        outFile.open(fileName);
        if (outFile.is_open()) {
            std::cout << "File created successfully.\n";
            current_indent      = 0;
            startDocument       = false;
            docWrite            = false;
            firstStartElement   = true;
            elementOpen         = false;
            stringWritten       = false;
            xmlEncode           = xmlEncoding;
        }
    } else {
        std::cerr << "File already exists.";
    }
}


void XmlWriter::writeStartDocument() {
    if (!startDocument) {
        startDocument = true;
        docWrite = true;
    }
}


void XmlWriter::writeEndDocument() {
    if (startDocument) {
        startDocument   = false;
        docWrite        = false;
    }
}


bool XmlWriter::exists(std::string fileName){
    char *outFile = (char*)fileName.c_str();

    std::ifstream checkFile(outFile);
    return !!checkFile;
}


bool XmlWriter::isOpen() {
    if (outFile.is_open()) {
        return true;
    }
    return false;
}


void XmlWriter::close() {
    if (!startDocument) {
        outFile.close();
    }
}


void XmlWriter::writeStartElement(std::string elementTag) {
    if (startDocument) {
        outFile << "<!--XML Document-->\n";
        outFile << "<?xml version='1.0' encoding='" << xmlEncode << "'?>";
        startDocument = false;
    }
    if (docWrite) {
        if (firstStartElement) {
            firstStartElement = false;
        }
        outFile << "\n";
        outFile << std::string(current_indent, '\t');
        current_indent++;
        tagStack.push(elementTag);
        outFile << "<" << tagStack.top() << ">";

        elementOpen     = true;
        stringWritten   = false;
    }
}


void XmlWriter::writeEndElement() {
    if (docWrite) {
        if (!(tagStack.isEmptyStack())) {
            if (!firstStartElement) {
                outFile << "</" << tagStack.top() << ">";
                current_indent--;
                firstStartElement = true;
            } else {
                current_indent--;
                outFile << "\n";
                outFile << std::string(current_indent, '\t');
                outFile << "</" << tagStack.top() << ">";
            }
            tagStack.pop();

            elementOpen     = false;
            stringWritten   = false;
        } else {
            std::cerr << "No tags to close.";
        }
    }    
}


void XmlWriter::writeAttribute(std::string outAttribute) {
    if (docWrite && elementOpen && !stringWritten) {
        long pos = outFile.tellp();
        outFile.seekp(pos - 1);
        outFile << " " << outAttribute << ">";
    }
}


void XmlWriter::writeString(std::string outString) {
    if (docWrite && elementOpen) {
        outFile << outString;
        stringWritten = true;
    }
}

Answer 1

StackADT:

At a first glance, it seems to me that you have over-engineered the stack concept. Do you really need different stack implementations? That would be the only justification for the virtual interface. std::stack would certainly be enough for your needs, and it is most likely more efficient too, as it doesn't utilize virtual dispatch, like yours.

NULL in C++:

No, no, and no! Use nullptr.

const methods:

Methods that don't mutate member state should be marked with a const at the end, this is called Const Correctness. isOpen() is an example:

bool isOpen() const;

Don't use C-style casts and don't cast away const:

bool XmlWriter::exists(std::string fileName){
    char *outFile = (char*)fileName.c_str();

    std::ifstream checkFile(outFile);
    return !!checkFile;
}

Always prefer C++ casts over C-style casts. Use the correct operators: static_cast, reinterpret_cast, etc. This will prevent you from accidently casting away things you don't mean to without being explicit.

NEVER cast away const from a pointer. In this case you don't have to. This is the most dangerous thing you can do and leads to potentially crashing code. Your could have declared the pointer as const char *.

Don't store the content of string::c_str() in a variable:

If the std::string is modified in any way, the pointer that was returned is invalidated. If you try to use that pointer, you have undefined behavior. So storing the pointer is dangerous. The best place to use the value is to pass it directly as a parameter to the function:

 std::ifstream  checkFile(fileName.c_str()); // Perfectly fine.
                                             // There is no chance of altering
                                             // filename and invalidating the
                                             // result of the call to c_str()

Note: With C++11, std::ifstream now accepts an std::string as filename, so you can pass fileName directly if this is the case:

 std::ifstream checkFile(fileName);

Miscellaneous:

xmlEncoding is referenced a couple times. Make it a class level static constant:

class XmlWriter {
...
private:

    static const std::string xmlEncoding;

And in the .cpp you define it:

const std::string XmlWriter::xmlEncoding("utf-8");

---

Align these assignments to match the rest:

XmlWriter::XmlWriter(std::string fileName) {
    if (!(exists(fileName))) {
        outFile.open(fileName);
        if (outFile.is_open()) {
            std::cout << "File created successfully.\n";
            current_indent    = 0;
            startDocument     = false;
            docWrite          = false;
            firstStartElement = true;
            elementOpen       = false;
            stringWritten     = false;
            xmlEncode         = xmlEncoding;
        }
    }
    ...
}

Those (in the XmlWriter(std::string) ctor) were not aligned like in other methods.

---

Avoid nesting. It is usually better to return early. For example, instead of:

if (!(exists(fileName))) {
    ... stuff ...
}
else {
    std::cerr << "File already exists.";
}

Prefer:

if (exists(fileName)) {
    std::cerr << "File already exists.";
    return;
}
... stuff ...

Answer 2

I started with your code to make it clean and correct, but I had to throw away so much code that not much is left anymore. Here are my changes, together with some remarks:

CMakeList.txt

cmake_minimum_required(VERSION 3.9)
project(XmlWriter)

set(CMAKE_CXX_STANDARD 11)

SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Werror -Os")

add_executable(
        XmlWriter
        XmlWriter.cpp
        main.cpp)

My IDE added this file to my project, and I just added the compiler options and the source files. It builds and runs fine. Providing your code reviewers with your original environment is crucial because they can then easily see how you set up your project. And they can just copy the files and be ready to test and improve your code.

I threw away the stack and the linked stack since they have nothing to do with generating XML. A simple std::vector has all the features this code needs.

XmlWriter.hpp

#ifndef XmlWriter_H
#define XmlWriter_H

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

class XmlWriter {

public:
    XmlWriter(const std::string &fileName);
    void close();
    bool isOpen();
    void writeStartElement(const std::string &elementName);
    void writeEndElement();
    void writeAttribute(const std::string &name, const std::string &value);
    void writeString(const std::string &text);

private:
    std::ofstream outFile;
    std::vector<std::string> openTags;
    bool open;
};

#endif

I removed the exists function and converted all std::string arguments to be const std::string &, since passing a reference (the &) is more efficient than copying the string.

XmlWriter.cpp

#include <exception>
#include <stdexcept>

#include "XmlWriter.hpp"

Here, I removed the no-args constructor since a general-purpose class like XmlWriter has nothing to do with reading files on someone's desktop.

XmlWriter::XmlWriter(const std::string &fileName)
        : open(false) {
    outFile.open(fileName);
}

I also removed the constructor that takes an encoding. Nowadays, all XML files should always be written in the UTF-8 encoding. And this encoding is what your program hopefully uses internally. I cannot guarantee this because std::string is not a string of characters, but a string of bytes. And these have nothing to do with each other.

bool XmlWriter::isOpen() {
    return outFile.is_open();
}

The above code does the same as your code, it just takes less space.

void XmlWriter::close() {
    if (!openTags.empty()) {
        throw std::logic_error("Unclosed tags");
    }
    outFile.close();
}

I decided to throw exceptions whenever a programming mistake is detected because generating malformed XML files is something that no program should ever do.

I removed the startDocument and endDocument functions since they are not necessary. The default encoding for XML files is UTF-8, therefore it doesn't need to be specified in the <?xml ... ?> preamble.

void XmlWriter::writeStartElement(const std::string &name) {
    if (open) {
        outFile << ">";
        open = false;
    }
    outFile << "<" << name;
    openTags.push_back(name);
    open = true;
}

I removed the automatic newline and indentation since some XML files need them. For most practical files, automatic indentation and linebreaks can be added as long as there is an additional function writeTextElement that does writeStartElement, writeString, writeEndElement.

void XmlWriter::writeEndElement() {
    if (openTags.empty()) {
        throw std::logic_error("No open elements");
    }
    if (open) {
        outFile << " />";
    } else {
        outFile << "</" << openTags.back() << ">";
    }
    openTags.pop_back();
    open = false;
}

namespace {
    void writeEscaped(std::ofstream &out, const std::string &text) {
        for (auto &ch : text) {
            switch (ch) {
                case '<':
                    out << "&lt;";
                    break;
                case '>':
                    out << "&gt;";
                    break;
                case '"':
                    out << "&quot;";
                    break;
                case '\'':
                    out << "&#39;";
                    break;
                case '&':
                    out << "&amp;";
                    break;
                default:
                    if (static_cast<unsigned char>(ch) >= ' ') {
                        out << ch;
                    } else {
                        out << "&#" << static_cast<unsigned int>(static_cast<unsigned char>(ch)) << ";";
                    }
            }
        }
    }
}

The above helper function is the most important detail of the whole XmlWriter class. It ensures that however broken the strings from your application are, the chances are very high that the resulting XML document is well-formed. The above code doesn't catch all cases (some Unicode code points are not allowed in XML files, and since the strings are hopefully encoded as UTF-8, these code points would span multiple bytes and are therefore difficult to check. Especially when you consider that writeString may be called twice in a row, once with the first half of a code point, and the next time with the second half. But still, these code points are not as dangerous as having literal <>&" in the XML.

void XmlWriter::writeAttribute(const std::string &name, const std::string &value) {
    if (!open) {
        throw std::logic_error("Attributes can only written in an open element");
    }
    outFile << " " << name << "=\"";
    writeEscaped(outFile, value);
    outFile << "\"";
}

void XmlWriter::writeString(const std::string &text) {
    if (open) {
        outFile << ">";
        open = false;
    }
    writeEscaped(outFile, text);
}

Summary

• Your code can be written a lot simpler.
• Your code doesn't take into account special characters, which it must.
• Your code defined a superfluous data type even though the C++ standard library provides a suitable data type. That is error-prone.