trjhtr

I was wondering if someone wouldn't mind reviewing this XML writer code that I've been working on. It should be fully functional, and includes an example of how to use it.

ConsoleApplication1.cpp

// ConsoleApplication1.cpp : Defines the entry point for the console application. 
// 

#include "stdafx.h" 
#include "XMLReader.h" 
#include "XMLWriter.h" 
#include "XMLErr.h" 
#include <Windows.h> 

int main() 
 
 XMLWriter doc("test.xml"); 
 doc.CreateDecl("1.0", "utf-8", "true"); 
 doc.CreateRootNode("XML", "id", "test"); 
 XMLNode* childNode = new XMLNode("ACHIEVEMENT"); 
 doc.InsertNode(childNode); 
 doc.InsertAttribute(childNode, "TestAttr", "test"); 

 XMLNode* newNode = new XMLNode("TEST", "testinnertext"); 
 childNode->AddChildNode(newNode); 

 XMLAttribute* attr = new XMLAttribute("test", "0"); 
 newNode->AddAttribute(attr); 

 doc.WriteDoc(); 
 doc.Close(); 
 

XMLWriter.cpp

#include "stdafx.h" 
#include "XMLWriter.h" 

XMLWriter::XMLWriter(const char* fileName) 
 
 file = std::ofstream(fileName); 
 

XMLWriter::~XMLWriter() 
 

 

void XMLWriter::Close() 
 
 if (file.is_open()) 
 file.close(); 

 rootNode->Close(); 
 

void XMLWriter::Save() 
 
 

void XMLWriter::CreateDecl(const char* version, const char* encoding, bool standalone) 
 
 file << "<?xml version=" << """ << version << "" "; 
 if (encoding != nullptr) 
 file << "encoding=" << """ << encoding << "" "; 
 if (standalone != false) 
 file << "standalone=" << """ << "true" << """; 

 file << "?>" << "n"; 
 

void XMLWriter::CreateRootNode(const char* rootName, const char* attrName, const char* attrValue) 
 
 rootNode = new XMLNode(); 

 rootNode->SetName(rootName); 
 if (attrName != nullptr && attrValue != nullptr) 
 
 XMLAttribute* attribute = new XMLAttribute(attrName,attrValue); 
 attribute->SetName(attrName); 
 attribute->SetValue(attrValue); 
 rootNode->AddAttribute(attribute); 

 //file << "<" << rootName << " " << attrName << "=" << """ << attrValue << """ << ">" << "n"; 

 
 else 
 //file << "<" << rootName << ">"; //<< "n" << "</" << rootName << ">"; 
 
 

void XMLWriter::InsertNode(XMLNode* child) 
 
 rootNode->AddChildNode(child); 
 

void XMLWriter::InsertNode(const char* nodeName, const char* nodeValue) 
 
 XMLNode* node = new XMLNode(nodeName, nodeValue); 

 rootNode->AddChildNode(node); 
 

void XMLWriter::InsertAttribute(XMLNode* target, const char* attrName, const char* attrValue) 
 
 target->AddAttribute(attrName, attrValue); 
 

void XMLWriter::WriteDoc() 
 
 //TODO: get this gross ass shit functional 
 file << "<" << rootNode->GetName(); 
 for (auto it : rootNode->GetAttributes()) 
 
 file << " " << it->GetName() << "=" << """ << it->GetValue() << """; 
 
 file << ">"; 

 WriteNodesRecursively(rootNode); 

 file << "</" << rootNode->GetName() << ">"; 
 

void XMLWriter::WriteNodesRecursively(XMLNode* currNode) 
 
 for (auto itor : currNode->GetChildNodes()) 
 
 file << "n"; 

 file << "t" << "<" << itor->GetName(); 
 for (auto it : itor->GetAttributes()) 
 
 file << " " << it->GetName() << "=" << """ << it->GetValue() << """; 
 

 if (itor->GetInnerText().length() > 0 && itor->GetChildNodes().size() == 0) 
 
 file << ">"; 
 file << itor->GetInnerText(); 
 file << "</" << itor->GetName() << ">" << "n"; 
 
 else if (itor->GetChildNodes().size() > 0) 
 
 file << ">"; 
 WriteNodesRecursively(itor); 
 file << "t" << "</" << itor->GetName() << ">" << "n"; 
 

 
 

XMLWriter.h

#pragma once 
#include "XMLNode.h" 
#include "XMLAttribute.h" 

class XMLWriter 
 
private: 
 std::ofstream file; 
 XMLNode* rootNode; 
public: 
 XMLWriter() = default; 
 XMLWriter(const char* fileName); 
 ~XMLWriter(); 
 void Close(); 
 void Save(); 
 void WriteDoc(); 
 void WriteNodesRecursively(XMLNode* node); 
public: 

 XMLNode * GetRootNode() return rootNode; 
 void CreateDecl(const char* version, const char* encoding = 0, bool standalone = 0); 
 void CreateRootNode(const char* nodeName, const char* attrName = 0, const char* attrValue = 0); 

 ///All 3 of these are used only to add a childnode to rootnode, otherwise use XMLNodes AddChildNode function 
 ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// 
 void InsertNode(XMLNode* child); // 
 void InsertNode(const char* nodeName, const char* attrValue = 0); // 
 void InsertAttribute(XMLNode* target, const char* attrName, const char* attrValue); // 
 ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// 
; 

XMLNode.cpp

#include "stdafx.h" 
#include "XMLNode.h" 

XMLNode::XMLNode(const char* nodeName, const char* nodeInnerText) 
 
 name = nodeName; 
 if (nodeInnerText != nullptr) 
 innerText = nodeInnerText; 
 

XMLNode::~XMLNode() 
 

 

///this is called from XMLReader, delete child node and any child nodes that exists within the instance 
void XMLNode::Close() 
 
 for (auto& attribute : attributes) 
 delete attribute; 

 for (auto& childNode : childNodes) 
 childNode->Close(); 

 attributes.clear(); 
 childNodes.clear(); 

 delete this; 
 

void XMLNode::AddAttribute(XMLAttribute* attribute) 
 
 attributes.push_back(attribute); 
 


void XMLNode::AddAttribute(const char* attrName, const char* attrValue) 
 
 XMLAttribute* attr = new XMLAttribute(attrName, attrValue); 

 attributes.push_back(attr); 
 


void XMLNode::AddChildNode(XMLNode* childNode) 
 
 childNodes.push_back(childNode); 
 

XMLAttribute* XMLNode::GetAttribute(const std::string attrName) 
 
 for (auto attr : attributes) 
 
 if (_stricmp(attr->GetName().c_str(), attrName.c_str()) == 0) 
 return attr; 
 
 return nullptr; 
 

XMLNode* XMLNode::GetChildNode(int index) 
 
 if (childNodes.empty()) 
 return nullptr; 

 return childNodes.at(index); 
 

bool XMLNode::FindChild(XMLNode* node) 
 
 for (auto itor : childNodes) 
 
 if (itor == node) 
 return true; 
 
 return false; 
 

XMLNode.h

#pragma once 
#include "XMLAttribute.h" 

class XMLNode 
 

private: 
 std::string name; 
 std::string innerText; 
 std::vector<XMLAttribute*> attributes; 
 std::vector<XMLNode*> childNodes; 
public: 
 XMLNode() = default; 
 XMLNode(const char* nodeName, const char* nodeInnerText = 0); 
 ~XMLNode(); 

 void Close(); 

 void SetName(const std::string value) name = value; 
 const std::string GetName() const return name; 

 void SetInnerText(const std::string value) innerText = value; 
 const std::string GetInnerText() const return innerText; 

 //give users a choice of either adding a pointer to the function, or creating the pointer in the function, then adding the pointer to the node 
 void AddAttribute(XMLAttribute* attribute); 
 void AddAttribute(const char* attrName, const char* attrValue); 
 void AddChildNode(XMLNode* childNode); 
 // 

 XMLAttribute* GetAttribute(const std::string attrName); 
 XMLNode* GetChildNode(int index); 

 const std::vector<XMLAttribute*>& GetAttributes() return attributes; 

 bool FindChild(XMLNode* node); 

 const std::vector<XMLNode*>& GetChildNodes() const return childNodes; 
; 

XMLAttribute.cpp

#include "stdafx.h" 
#include "XMLAttribute.h" 

XMLAttribute::XMLAttribute(const char* attrName, const char* attrValue) 
 
 name = attrName; 
 value = attrValue; 
 

bool XMLAttribute::CheckValue() 
 
 for (auto c : value) 
 
 if (!isdigit(c)) 
 
 throw(std::exception("Error, value is not an integer, or contains a non-numeric value")); 
 
 
 return true; 
 

const int XMLAttribute::AsInt() 
 
 if (!CheckValue()) 
 throw std::exception("Error, attributes value contains a non-numerical character"); 

 if (std::stoi(value) > INT_MAX 

const unsigned int XMLAttribute::AsUInt() 
 

const float XMLAttribute::AsFloat() 
 std::stof(value) < FLT_MIN) 
 throw(std::out_of_range::exception("Error, value is outside the range of a float")); 

 return std::stof(value); 
 

const double XMLAttribute::AsDouble() 
 

XMLAttribute.h

#pragma once 


class XMLAttribute 
 
private: 
 std::string value; 
 std::string name; 
public: 
 XMLAttribute(const char* attrName, const char* attrValue); 
 void SetValue(const std::string inValue) value = inValue; 
 void SetName(const std::string inName) name = inName; 
 const std::string GetValue() const return value; 
 const std::string GetName() const return name; 

 ///verifies that the string contains only numeric characters 
 bool CheckValue(); 

 ///conversion functions for the attributes value 
 /////////////////////////////////////////////////////////////////////////////////////// 
 const int AsInt(); // 
 const unsigned int AsUInt(); // 
 const float AsFloat(); // 
 const double AsDouble(); // 
 /////////////////////////////////////////////////////////////////////////////////////// 

; 

It's good you have some working code, and good that you've asked for a review. There are number of things that may help you improve your code.

Use the required #includes

The code uses std::string, std::vector, etc. which means that it should have

#include <string>
#include <vector>

and others. Since you have followed the usual separation the implementation (.cpp file) from the interface (the .h file), put only those things in the header file that are required to understand the header. Any implementation details should go into the .cpp file if they don't already show up in the header. As an example, in XMLAttribute.cpp we should have:

#include <stdexcept>
#include <climits>
#include <cfloat>

while XMLAttribute.h should have this:

#include <string>

Eliminate unused headers

Nothing from the <Windows.h> or XMLReader.h or XMLErr.h appear to actually have been used within this code, so it would be best to eliminate those from the ConsoleApplication1 code. Also, I can't help but think that we can come up with a more descriptive file name for this code.

Use standard exceptions

In a few places in the code, there are non-standard exceptions like this:

throw(std::exception("Error, value is not an integer, or contains a non-numeric value")); 

There is no std::exception constructor that takes a std::string or const char * as an argument. Instead, we need to use one of the actual standard types:

throw std::domain_error("Error, value is not an integer, or contains a non-numeric value"); 

Also note that throw is a keyword and not a function call so the enclosing parentheses are not needed or wanted.

General portability

This code could be made portable if, in addition to the changes in the previous points, you omit the Windows-only include files #include "stdafx.h" and #include <Windows.h>

Use exceptions only for exceptional things

There use of exceptions is inconsistent. For instance, we have this:

bool XMLAttribute::CheckValue() const

 for (auto c : value)
 
 if (!isdigit(c))
 
 throw std::domain_error("Error, value is not an integer, or contains a non-numeric value");
 
 
 return true;

This code returns true or throws an exception. That's more than a bit strange, especially because it's only used internally. I'd suggest instead having it return a bool either way and leave it up to the caller to throw if needed, since the calling functions are written that way already anyway.

Use standard library functions

Instead of the code above for CheckValue, first, I'd rename it to something that makes it easy to interpret the result such as containsOnlyDigits. That way it's much easier to understand what it's checking and much more obvious to tell what a true return value means.

Second, use a standard function for this. Most convenient here would be to use a C++11 lambda function:

bool XMLAttribute::containsOnlyDigits() const 
 return std::all_of(value.begin(), value.end(), 
 (char c) return std::isdigit(c); );

Don't specify const for return values

The XMLAttribute member functions AsInt(), AsUInt(), etc. return a value rather than a pointer or reference, but they don't alter the underlying XMLAttribute object. For that reason, instead of this:

const int AsInt(); // returns a const?

write this:

int AsInt() const; // does not alter object

Prefer modern initializers for constructors

The constructors could use the more modern initializer style rather than the old style you're currently using. Instead of this:

XMLAttribute::XMLAttribute(const char* attrName, const char* attrValue) 
 
 name = attrName; 
 value = attrValue; 
 

you could instead write this:

XMLAttribute::XMLAttribute(const char* attrName, const char* attrValue) :
 valueattrValue,
 nameattrName
 

Note also that the member items are initialized in declaration order rather than the order they're listed in the constructor, so to avoid any misunderstanding, it's best to write the constructor such that it uses the same order as the declaration.

Let the compiler create default destructor

The compiler will create a destructor by default which is essentially identical to what you've got, so you can simply omit both the declaraton and implementation from your code. If you want to make sure the user of the code understands that it's the default-constructed destructor, you can make it very clear by writing this:

~XMLNode() = default;

Use C++ idioms

This isn't Java, so having getters and setters for everything (e.g. GetValue(), SetValue()) is not the norm and is not necessarily a good interface. For instance, the XMLNode::getAttribute() function returns a non-const pointer to its copy of the attribute. That's a broken design for at least two reasons. First, it would allow the caller to arbitrarily alter the internals of the class. Second, if the XMLNode object goes out of scope and is destroyed before the returned pointer is dereferenced, you have undefined behavior. Better would probably be to return a copy.

Reconsider the interface

There are a number of peculiarities that make this interface less than ideal. One thing is that the nodes in this scheme are allowed to have idential attribute names. According to the W3C,

For interoperability, an XML processor MAY at user option issue a warning when more than one attribute-list declaration is provided for a given element type, or more than one attribute definition is provided for a given attribute, but this is not an error.

Also the XMLWriter is tied to a std::ofstream. What if I want to write to a std::stringstream instead? Better would be to have the Writer manage serialization of data to any std::ostream and allow the user to choose at runtime, independently of how/when the instance data is populated.

Avoid raw pointers

The code includes this function:

bool XMLNode::FindChild(XMLNode* node) 
 
 for (auto itor : childNodes) 
 
 if (itor == node) 
 return true; 
 
 return false; 
 

Is it really intended to be able to use this code while passing nullptr into the function? If so, I'd suggest a check for nullptr. If not, use a const XMLNode &node argument and overload an XMLNode::operator==() function.

Understand the XML specification

The XMLNode::GetAttribute function inclues these lines

if (_stricmp(attr->GetName().c_str(), attrName.c_str()) == 0) 
 return attr; 

In addition to the already mentioned problem about returning a pointer to internal object data, this seems to imply that XML attribute names are case insensitive. In fact, they are not, so this appears to be an error in understanding or applying the XML 1.0 specification. Also _stricmp is non-standard code.