Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
        }
    }
share|improve this question
1  
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 Apr 13 at 21:01
1  
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 Apr 14 at 6:29
    
have you posted a follow up yet? –  Malachi Apr 15 at 18:57
    
yes, i have: codereview.stackexchange.com/questions/47551/… –  Joel Apr 18 at 8:08

4 Answers 4

up vote 6 down vote accepted

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()
share|improve this answer

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

XKCD 1179

share|improve this answer
    
Actually I use the DateTime ToString() method, I admit that this is kind of quick n' dirty coding style. I will change this, thank you. –  Joel Apr 16 at 6:02
    
I think that he was talking about how you store it in the XML? when you add it to your XML through your code it looks like you have to refer to the date this way new DateTime(1564, 04, 26) –  Malachi Apr 16 at 12:13
    
This is how i store the Date into the XML File: new XElement(AuthorElementDayOfBirth) { Value = author.DayOfBirth.ToLongDateString() } –  Joel Apr 16 at 18:58

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);
}
share|improve this answer
1  
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 Apr 13 at 21:02
    
@Jamal o yes I did ha. Ok, I'll have another look and see if my comments still suite. Cheers. –  dreza Apr 13 at 21:06
    
your comment suits very well, I really like the wrapper class idea. –  Joel Apr 14 at 5:57
    
Serialize/Deserialize thing sounds promising but I have the requirement to keep the data file readable for humans –  Joel Apr 16 at 5:57
    
@Joel Using the XmlSerializer produces the XML exactly like if you were doing it manually. All it does is takes your classes and their structure and produces equivalent XML –  dreza Apr 16 at 6:22

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.

share|improve this answer
    
I like that you did split the professions. Small help on the syntax of German listings: item1{, }item2{, }itemx{ und }itemN. Point being: you need a third profession node ;) also "englischer" is an adjective, which in fact means: of english nationality. Also all elements of the list are modified by prepended adjectives in German. You could instead introduce a node nationality –  Vogel612 Apr 15 at 18:38
    
Thank you, I haven't thought about XML in this way. –  Joel Apr 16 at 5:59
    
you're Welcome. I look forward to seeing a follow up question. :) –  Malachi Apr 16 at 12:11

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.