XML Writer in C++ - Updated

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

#include "StackADT.h"

template 
struct nodeType {
    Type info;
    nodeType *link;
};

template 
class LinkedStack : public StackADT  {

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

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

};

#include "LinkedStack.tpp"

#endif

LinkedStack.tpp

```
#include

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

return *this;
}

template
LinkedStack::LinkedStack() {
stackTop = NULL;
}

template
LinkedStack::LinkedStack(const LinkedStack& otherStack) {
stackTop = NULL;
copyStack(otherStack);
}

template
LinkedStack::~LinkedStack() {
initializeStack();
}

template
void LinkedStack::initializeStack() {
nodeType *temp;

while (stackTop != NULL) {

temp = stackTop;

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

StackExchange Code Review Q#67594, answer score: 8