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;
        }
    }

Post rolled back. Please don't update the original code based on answers; that will invalidate them. You may instead ask a new follow-up question for further review. — Jamal♦, yesterday
Sorry, I did no know that. But I still have questions about how to handle such situations. See code review meta: meta.codereview.stackexchange.com/questions/1763/… — Joel, yesterday

janos · Answer 1 · 2014-04-13 15:51:49Z

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()

dreza · Answer 2 · 2014-04-13 20:58:44Z

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);
}

I hope you didn't just review the OP's updated code. :-( The first answer was invalidated, so I had to rollback. I apologize for the inconvenience. — Jamal♦, yesterday
@Jamal o yes I did ha. Ok, I'll have another look and see if my comments still suite. Cheers. — dreza, yesterday
your comment suits very well, I really like the wrapper class idea. — Joel, yesterday

asked	2 days ago
viewed	48 times
active	yesterday

current community

your communities

more stack exchange communities

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

2 Answers

Your Answer

Not the answer you're looking for? Browse other questions tagged c# clean-code readability xml or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

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

2 Answers

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c# clean-code readability xml or ask your own question.

Related

Hot Network Questions