Check if XmlNode with certain attribute exists and create XmlNode if not

Question

I'm not going to explain the purpose of the code provided below. If you do not understand it, i know that i have to improve it (naming, structure etc.).

I would like to know how to improve this code in terms of readability and complexity.

Data structure:

<?xml version="1.0" encoding="utf-8"?>
<QuoteApp>
  <Authors>
    <Author>
      <AuthorId>1</AuthorId>
      <Firstname>William</Firstname>
      <Lastname>Shakespeare</Lastname>
      <Profession>englischer Dramatiker, Lyriker und Schauspieler</Profession>
      <DayOfBirth>Sonntag, 26. April 1564</DayOfBirth>
      <DayOfDeath>Dienstag, 3. Mai 1616</DayOfDeath>
      <Image>/Data/Images/Author1.jpg</Image>
    </Author>
    <Author>
      <AuthorId>2</AuthorId>
      <Firstname>Friedrich</Firstname>
      <Lastname>Nietzsche</Lastname>
      <Profession>deutscher Philologe und Philosoph</Profession>
      <DayOfBirth>Dienstag, 15. Oktober 1844</DayOfBirth>
      <DayOfDeath>Samstag, 25. August 1900</DayOfDeath>
      <Image>/Data/Images/Author2.jpg</Image>
    </Author>
  </Authors>
  <Quotes>
    <Quote>
      <QuoteId>1</QuoteId>
      <Text>qwerty</Text>
    </Quote>
    <Quote>
      <QuoteId>2</QuoteId>
      <Text>qwerty</Text>
    </Quote>
  </Quotes>
</QuoteApp>

Entry Point:

private void SaveAuthor()
{
    var xmlFileHandler = new XmlFileHandler();
    //xmlFileHandler.CreateXmlFile();

    xmlFileHandler.AddAuthor(new Author(1, "William", "Shakespeare", "englischer Dramatiker, Lyriker und Schauspieler",
        new DateTime(1564, 04, 26), new DateTime(1616, 05, 3))); 

    xmlFileHandler.AddAuthor(new Author(2, "Friedrich", "Nietzsche", "deutscher Philologe und Philosoph",
        new DateTime(1844, 10, 15), new DateTime(1900, 08, 25)));
}

XmlFileHandler class

public class XmlFileHandler
    {
        private const string FileName = "quotes.xml";
        private const string AuthorsNodeName = "Authors";

        public bool AddAuthor(Author author)
        {
            var xmlQuotes = XDocument.Load(FileName);

            var authorExists = CheckIfAuthorAlreadyExists(author.AuthorId, xmlQuotes);

            if (!authorExists)
            {
                this.AddAuthorToXmlDocument(author, xmlQuotes);

                return true;
            }

            return false;
        }

        private void AddAuthorToXmlDocument(Author author, XDocument xmlQuotes)
        {
            var authorsNode = xmlQuotes.Descendants(AuthorsNodeName).FirstOrDefault();
            if (authorsNode != null) authorsNode.Add(this.CreateAuthorXmlNode(author));

            xmlQuotes.Save(FileName);
        }

        private bool CheckIfAuthorAlreadyExists(int authorId, XDocument xmlQuotes)
        {
            var xmlAuthorList = xmlQuotes.Descendants(AuthorsNodeName).Descendants("Author");
            IEnumerable<XElement> xElements = from xmlAuthor in xmlAuthorList
                                              let xElement = xmlAuthor.Element("AuthorId")
                                              where xElement != null && xElement.Value == authorId.ToString(CultureInfo.InvariantCulture)
                                              select xmlAuthor;

            if (xElements.Any())
            {
                return true;
            }

            return false;
        }

        private XElement CreateAuthorXmlNode(Author author)
        {
            var xmlAuthor = new XElement("Author");

            xmlAuthor.Add(new XElement("AuthorId") { Value = author.AuthorId.ToString(CultureInfo.InvariantCulture) });
            xmlAuthor.Add(new XElement("Firstname") { Value = author.Firstname });
            xmlAuthor.Add(new XElement("Lastname") { Value = author.Lastname });
            xmlAuthor.Add(new XElement("Profession") { Value = author.Profession });
            xmlAuthor.Add(new XElement("DayOfBirth") { Value = author.DayOfBirth.ToLongDateString() });
            xmlAuthor.Add(new XElement("DayOfDeath") { Value = author.DayOfDeath.ToLongDateString() });
            xmlAuthor.Add(new XElement("Image") { Value = author.Image.AbsolutePath });

            return xmlAuthor;
        }
    }

Answer 1

Instead of reloading the XML file every time you call AddAuthor, it would be more efficient to load the file once. If you are concerned about other threads or processes writing to the file, then you might want to lock it.

The authorExists local variable is pointless, you could drop it. Unless you think the code is more readable this way.

In AddAuthorToXmlDocument, probably you only want to save the file after successfully adding an author, so I would move the saving within the if (authorsNode != null).

In CheckIfAuthorAlreadyExists the final if is unnecessary, you can simply:

return xElements.Any()

Answer 2

If you have any choice at all in the representation of dates, choose a format based on ISO 8601. The standard represents best practices, and its advantages include

• culture neutrality
• unambiguous interpretation
• ease of parsing by computers and humans
• widespread library support

Answer 3

I think the code reads good. One minor point I have is about the hard coding of the FileName into the class. This limits that entire class to only being able to deal with a file called quotes.xml.

I think you might get more testability out of moving that to a constructor argument. If you didn't want to have to always specify the file name each time the class was constructed you could either create a default empty constructor, or provide a wrapper class.

Some options for example:

public class XmlFileHandler
{
    private readonly string _fileName;

    public XmlFileHandler() : this("quotes.xml")
    {       
    }

    public XmlFileHandler(string fileName)
    {
        _fileName = fileName;
        this.xmlQuotes = XDocument.Load(_fileName);
    }
}   

Or remove the empty constructor and create a wrapper class:

public class XmlFileHandler
{
    private readonly string _fileName;

    public XmlFileHandler(string fileName)
    {
        _fileName = fileName;
        this.xmlQuotes = XDocument.Load(_fileName);
    }
}   

public class XmlQuotesFileHandler : XmlFileHandler
{
    public XmlFileHandler() : base("quotes.xml")
    {       
    }
}

On a side note. Have you considered using the XmlSerializer classes? These might be ideal in your situation?

That might look something like:

// The classes/model that match the Xml document
public class QuoteApp
{
    public List<Author> Authors { get; set; }
    // etc
}

public class Author 
{
    public int AuthorId { get; set; }
    public string FirstName { get; set; }
    // etc
}

// The conversion of those models to the xml string equivalents
public StringWriter Serialize(QuoteApp model)
{
    // Note the actual writing of the xml document would be the responsibility of another method
    StringWriter writer = new StringWriter();
    XmlWriter xml = XmlWriter.Create(writer, new XmlWriterSettings() { Encoding = writer.Encoding });
    xs.Serialize(xml, model);

    return writer;
}

public QuoteApp Deserialize(string xml)
{
    XmlSerializer xs = new XmlSerializer(typeof(QuoteApp));
    return xs.Deserialize(sr);
}

Answer 4

this is a really good example to explain how I think about XML Documents.

<Author>
  <AuthorId>1</AuthorId>
  <Firstname>William</Firstname>
  <Lastname>Shakespeare</Lastname>
  <Profession>englischer Dramatiker, Lyriker und Schauspieler</Profession>
  <DayOfBirth>Sonntag, 26. April 1564</DayOfBirth>
  <DayOfDeath>Dienstag, 3. Mai 1616</DayOfDeath>
  <Image>/Data/Images/Author1.jpg</Image>
</Author>

first I think of the Author as the Object, it has properties(attributes) and actions(child nodes).

<author id="1" name="William Shakespeare" FirstName="William" LastName="Shakespeare" picture="/Data/Images/Author1.jpg">
    <profession>Englischer Dramatiker</profession>
    <profession>Lyriker</profession>
    <profession>Schauspieler</profession>
    <dateofbirth>Sonntag, 26. April 1564</dateofbirth>
    <dateofdeath>Dienstag, 3. Mai 1616</dateofdeath>
</author>

I am not so sure about putting in the file path into the XML either in a node/tag or as an attribute, it looks like you hold all of your images in the same folder so in your XML you only need to know which one in that folder and be more descriptive in your XML, so instead of /Data/Images/Author1.jpg you would just have Author1.jpg.

The reason I say to get rid of the specification in the file path is because if you were to use xPath to query the data and you wanted the image you would have to add a bunch of extra characters to escape the /'s and that can be annoying when writing an xPath query.

---

The Profession

if there are more than one object in a category then make more than one node/tag for that item. it's almost like SQL where you want atomic data, one thing per tag/node.