Basic XML Parser

A while back, I posted an XML Writer. Today I have finished what I hope to be a solid XML parser that, while not professional, will get the job done.

To start, I do use a stack on one part of the parser, so you can find the code for that in the XML writer page I linked above.

Xml.h

```
#ifndef Xml_H
#define Xml_H

#include
#include
#include "LinkStack.h"
#include
#include
#include //Error reporting with message boxes.

namespace Xml {

enum class ErrorCode {

None,
StreamError,
FileExistError,
ElementCloseError,
ElementTagsNotEmpty

};

class Error {
public:
/**
Constructor
*/
Error();

/**
Throws Error Message and displays the message in messagebox
*/
void ThrowError(ErrorCode errorCode);

private:
ErrorCode err;
const char* errMsg;
};

Error::Error() :
err(ErrorCode::None),
errMsg(nullptr)
{

}

void Error::ThrowError(ErrorCode errorCode) {
err = errorCode;
if (errorCode != ErrorCode::None) {

switch (errorCode) {
case ErrorCode::StreamError:
errMsg = "Stream error has occured";
break;
case ErrorCode::FileExistError:
errMsg = "File does not exist";
break;
case ErrorCode::ElementCloseError:
errMsg = "No elements to close";
break;
case ErrorCode::ElementTagsNotEmpty:
errMsg = "Not all element tags are closed. Closing remaining tags";
break;
}
MessageBox(NULL, errMsg, "Error Has Occured", MB_ICONEXCLAMATION | MB_OK);
}
errMsg = nullptr;
}

class XmlReader : private Error {

public:
/**
Child Node
*/
struct cNode {
std::vector childNode;
std::string strInnerText;

Relying on specific type of error message deep inside your library makes it very tied to a specific platform.

void Error::ThrowError(ErrorCode errorCode) {

       // STUFF

        MessageBox(NULL, errMsg, "Error Has Occured", MB_ICONEXCLAMATION | MB_OK);

       // STUFF
}

Your library should not be handling user interactions. This is the job of the application. The library should just inform the application that an error has occurred and then let the application decide what to do (this code could be used in some background server app; you defiantly do not want a dialog box to show up on a headless server).

Two big options.

• Throw an exception.

• Allow the user to specify a callback that is called on error.

• Or both. The default action of the callback (if not provide by the user) is to throw.

Do you really need a cNode, pNode and Node? They all look identical.

Comments like this:

/**
        Checks if the file exists
    */
    bool exists(std::string strFileName);

    /**
        Checks if the file is open
    */
    bool isOpen();

are a waste of space. They are not part of the public interface and the function name clearly identifies what they do. Useless comments are worse than no comments as they need to be maintained.

The check for file existence seems overly hyperbolic:

if (!exists(strFileName)) {
        Error::ThrowError(ErrorCode::FileExistError);
        return false;
    }
    else {
        m_ifsInFile.open(strFileName, std::ios::in);
    }

The call to exist merely tries to create a file stream object and returns its state. Why not just try and open the file then check its state.

m_ifsInFile.open(strFileName, std::ios::in);

    if (!m_ifsInFile) {
        Error::ThrowError(ErrorCode::FileExistError);
        return false;
    }

Double negative check.

return !!ifsCheckFile;

That's old school not seen that in a while. But its also not even needed. A stream object used in a boolean context (like returning a bool from a function) is converted to a boolean value (or pre C++11 a value that can be used in a boolean context).

So easier to write:

return ifsCheckFile;  // And much easier to understand for those that have not
                         // seen the double negative check trick.

Don't use if to check the state of a boolen

if (!m_ifsInFile.is_open()) {
        return false;
    }
    return true;

    // much easier to write as:

    return m_ifsInFile.is_open();

StackExchange Code Review Q#74532, answer score: 6