2
\$\begingroup\$

This post is a follow up question.

The purpose of this class should became clear without any further explanation. Raise your hand. If not, then I will have to improve the naming/structure.

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" Firstname="William" Lastname="Shakespeare">
      <Profession>englischer Dramatiker, Lyriker und Schauspieler</Profession>
      <DayOfBirth>Sonntag, 26. April 1564</DayOfBirth>
      <DayOfDeath>Dienstag, 3. Mai 1616</DayOfDeath>
    </Author>
    <Author AuthorId="2" Firstname="Friedrich" Lastname="Nietzsche">
      <Profession>deutscher Philologe und Philosoph</Profession>
      <DayOfBirth>1844-10-15</DayOfBirth>
      <DayOfDeath>1900-08-25</DayOfDeath>
    </Author>
  </Authors>
  <Quotes>
  </Quotes>
</QuoteApp>

XmlFileHandler class

public class XmlFileHandler
{
    #region fields

    private const string FileName = "quotes.xml";
    private const string DateFormatPattern = "yyyy-MM-dd";
    private readonly XDocument xmlQuotes;

    #endregion

    #region XML Element / Attribute names
    private const string RootNodeName = "QuoteApp";
    private const string AuthorsNodeName = "Authors";
    private const string AuthorNodeName = "Author";
    private const string QuotesNodeName = "Quotes";
    private const string QuoteNodeName = "Quote";

    private const string AuthorElementAuthorId = "AuthorId";
    private const string AuthorElementFirstname = "Firstname";
    private const string AuthorElementLastname = "Lastname";
    private const string AuthorElementProfession = "Profession";
    private const string AuthorElementDayOfBirth = "DayOfBirth";
    private const string AuthorElementDayOfDeath = "DayOfDeath";
    private const string AuthorElementImage = "Picture";

    private const string QuoteElementQuoteId = "QuoteId";
    private const string QuoteElementText = "Text";
    private const string QuoteElementAuthorId = "AuthorId";
    #endregion

    #region Singleton
    private static volatile XmlFileHandler instance;
    private static readonly object SyncRoot = new Object();

    public static XmlFileHandler Instance
    {
        get
        {
            if (instance == null)
            {
                lock (SyncRoot)
                {
                    if (instance == null)
                        instance = new XmlFileHandler();
                }
            }

            return instance;
        }
    }
    #endregion

    private XmlFileHandler()
    {
        this.xmlQuotes = XDocument.Load(FileName);
    }

    #region Create XML File

    public void CreateXmlFile()
    {
        if (File.Exists(FileName)) return;

        var settings = new XmlWriterSettings { ConformanceLevel = ConformanceLevel.Document, Indent = true };

        using (var writer = XmlWriter.Create(FileName, settings))
        {
            writer.WriteStartDocument();
            writer.WriteStartElement(RootNodeName);

            writer.WriteStartElement(AuthorsNodeName);
            writer.WriteEndElement();

            writer.WriteStartElement(QuotesNodeName);
            writer.WriteEndElement();

            writer.WriteEndElement();
            writer.WriteEndDocument();
        }
    }

    #endregion

    #region Add Author to XML File

    public bool AddAuthor(Author author)
    {
        var authorExists = CheckIfAuthorAlreadyExists(author.AuthorId);

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

            return true;
        }

        return false;
    }

    private bool CheckIfAuthorAlreadyExists(int authorId)
    {
        var xmlAuthorList = xmlQuotes.Descendants(AuthorsNodeName).Descendants(AuthorNodeName);
        var xElements = from xmlAuthor in xmlAuthorList
                        let xElement = xmlAuthor.Element(AuthorElementAuthorId)
                        where xElement != null && xElement.Value == authorId.ToString(CultureInfo.InvariantCulture)
                        select xmlAuthor;

        return xElements.Any();
    }

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

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

        xmlAuthor.Add(new XAttribute(AuthorElementAuthorId, author.AuthorId.ToString(CultureInfo.InvariantCulture)));
        xmlAuthor.Add(new XAttribute(AuthorElementFirstname, author.Firstname));
        xmlAuthor.Add(new XAttribute(AuthorElementLastname, author.Lastname));
        xmlAuthor.Add(new XElement(AuthorElementProfession) { Value = author.Profession });
        xmlAuthor.Add(new XElement(AuthorElementDayOfBirth) { Value = author.DayOfBirth.ToString(DateFormatPattern) });
        xmlAuthor.Add(new XElement(AuthorElementDayOfDeath) { Value = author.DayOfDeath.ToString(DateFormatPattern) });

        return xmlAuthor;
    }

    #endregion

    #region Retrieve Authors

    public ObservableCollection<Author> RetrieveAuthorCollection()
    {
        var authorCollection = new ObservableCollection<Author>();

        var authorList = xmlQuotes.Descendants(AuthorsNodeName).Descendants(AuthorNodeName);

        foreach (var xmlAuthor in authorList)
        {
            var tmpAuthor = new Author();

            var authorId = xmlAuthor.Attribute(AuthorElementAuthorId);
            var firstname = xmlAuthor.Attribute(AuthorElementFirstname);
            var lastname = xmlAuthor.Attribute(AuthorElementLastname);
            var profession = xmlAuthor.Element(AuthorElementProfession);
            var dayOfBirth = xmlAuthor.Element(AuthorElementDayOfBirth);
            var dayOfDeath = xmlAuthor.Element(AuthorElementDayOfDeath);

            if (authorId != null) tmpAuthor.AuthorId = Convert.ToInt32(authorId.Value);
            if (firstname != null) tmpAuthor.Firstname = firstname.Value;
            if (lastname != null) tmpAuthor.Lastname = lastname.Value;
            if (profession != null) tmpAuthor.Profession = profession.Value;
            if (dayOfBirth != null) tmpAuthor.DayOfBirth = Convert.ToDateTime(dayOfBirth.Value);
            if (dayOfDeath != null) tmpAuthor.DayOfDeath = Convert.ToDateTime(dayOfDeath.Value);

            authorCollection.Add(tmpAuthor);
        }

        return authorCollection;
    }

    #endregion

    #region Retrieve next IDs

    public int NextAuthorId()
    {
        var authorList = xmlQuotes.Descendants(AuthorsNodeName).Descendants(AuthorNodeName);

        //TODO: ReSharper warns about "Possible multiple enumeration of IEnumerable", whats this all about?
        var authorWithHighestId = authorList.Any() ? authorList.Max(author => (int)author.Attribute(AuthorElementAuthorId)) : 0;

        authorWithHighestId++;

        return authorWithHighestId;
    }

    public int NextQuoteId()
    {
        var quoteList = xmlQuotes.Descendants(QuotesNodeName).Descendants(QuoteNodeName);

        //TODO: ReSharper warns about "Possible multiple enumeration of IEnumerable", whats this all about?
        var quoteWithHighestId = quoteList.Any() ? quoteList.Max(quote => (int)quote.Attribute(QuoteElementQuoteId)) : 0;

        quoteWithHighestId++;

        return quoteWithHighestId;
    }

    #endregion
}
\$\endgroup\$
2
\$\begingroup\$

//TODO: ReSharper warns about "Possible multiple enumeration of IEnumerable", whats this all about?

Because you are working with an IEnumerable it's possible that the underlying implementation of the method will re-execute any re-loading of the data set each time you are enumerating it. Hence if that involved querying a database table you might make multiple calls into the database each time.

So Resharper suggests you do a ToList() on the Enumerable to force the result set to be fetched and returned into a list so that from then on you are only enumerating the objects in the list in memory.

If you know for certain that the IEnumerable will not do any re-fetching etc then you could ignore this warning from resharper.

So in your specific case:

// Will query a datasource
authorList.Any()

//  Might also requry the same datasource even though it shouldn't need to
authorList.Max(author => (int)author.Attribute(AuthorElementAuthorId))

My full (only slightly modified code) is below, but a summary of the mods I made include:

  1. I personally like to return early and avoid nesting if I can as I feel it helps readability. For example.

    private bool AddAuthorToXmlDocument(Author author)
    {
        var authorsNode = xmlQuotes.Descendants(AuthorsNodeName).FirstOrDefault();
    
        if (authorsNode == null)
            return false;
        // ...
    }
    
  2. I noticed a little bit of code duplication so attempted to remove that.

    public int NextQuoteId()
    {
        var quoteWithHighestId = NextElementId(
                                      QuotesNodeName,
                                      QuoteNodeName,
                                      QuoteElementQuoteId);
    
        return quoteWithHighestId++;
    }
    
  3. I'm not sure of the purpose of the CreateXmlFile() method. If we are reading the file when we first access the singleton instance why do we need this method at all to be public? could we not ensure it exists in our constructor?

  4. When using the singleton pattern I always like refer to Jon Skeet's article on it.

Here is a crack at a revised code solution:

public sealed class XmlFileHandler
{
    #region fields

    private const string FileName = "quotes.xml";
    private const string DateFormatPattern = "yyyy-MM-dd";
    private readonly XDocument xmlQuotes;

    #endregion

    #region XML Element / Attribute names
    private const string RootNodeName = "QuoteApp";
    private const string AuthorsNodeName = "Authors";
    private const string AuthorNodeName = "Author";
    private const string QuotesNodeName = "Quotes";
    private const string QuoteNodeName = "Quote";

    private const string AuthorElementAuthorId = "AuthorId";
    private const string AuthorElementFirstname = "Firstname";
    private const string AuthorElementLastname = "Lastname";
    private const string AuthorElementProfession = "Profession";
    private const string AuthorElementDayOfBirth = "DayOfBirth";
    private const string AuthorElementDayOfDeath = "DayOfDeath";
    private const string AuthorElementImage = "Picture";

    private const string QuoteElementQuoteId = "QuoteId";
    private const string QuoteElementText = "Text";
    private const string QuoteElementAuthorId = "AuthorId";
    #endregion

    #region Singleton
    private static readonly XmlFileHandler instance = new XmlFileHandler();

    // Explicit static constructor to tell C# compiler
    // not to mark type as beforefieldinit
    static XmlFileHandler()
    {
    }

    private XmlFileHandler()
    {
        this.xmlQuotes = XDocument.Load(FileName);
    }

    public static XmlFileHandler Instance
    {
        get
        {
            return instance;
        }
    }

    #endregion

    #region Create XML File

    public void CreateXmlFile()
    {
        if (File.Exists(FileName)) return;

        var settings = new XmlWriterSettings { ConformanceLevel = ConformanceLevel.Document, Indent = true };

        using (var writer = XmlWriter.Create(FileName, settings))
        {
            writer.WriteStartDocument();
            writer.WriteStartElement(RootNodeName);

            writer.WriteStartElement(AuthorsNodeName);
            writer.WriteEndElement();

            writer.WriteStartElement(QuotesNodeName);
            writer.WriteEndElement();

            writer.WriteEndElement();
            writer.WriteEndDocument();
        }
    }

    #endregion

    #region Add Author to XML File

    public bool AddAuthor(Author author)
    {
        return !AuthorAlreadyExists(author.AuthorId) &&
               AddAuthorToXmlDocument(author);
    }

    private bool AuthorAlreadyExists(int authorId)
    {
        var xmlAuthorList = Elements(AuthorsNodeName, AuthorNodeName);
        var xElements = from xmlAuthor in xmlAuthorList
                        let xElement = xmlAuthor.Element(AuthorElementAuthorId)
                        where xElement != null && xElement.Value == authorId.ToString(CultureInfo.InvariantCulture)
                        select xmlAuthor;

        return xElements.Any();
    }

    private bool AddAuthorToXmlDocument(Author author)
    {
        var authorsNode = xmlQuotes.Descendants(AuthorsNodeName).FirstOrDefault();

        if (authorsNode == null)
            return false;

        authorsNode.Add(this.CreateAuthorXmlNode(author));
        xmlQuotes.Save(FileName);   
        return true;
    }

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

        xmlAuthor.Add(new XAttribute(AuthorElementAuthorId, author.AuthorId.ToString(CultureInfo.InvariantCulture)));
        xmlAuthor.Add(new XAttribute(AuthorElementFirstname, author.Firstname));
        xmlAuthor.Add(new XAttribute(AuthorElementLastname, author.Lastname));
        xmlAuthor.Add(new XElement(AuthorElementProfession) { Value = author.Profession });
        xmlAuthor.Add(new XElement(AuthorElementDayOfBirth) { Value = author.DayOfBirth.ToString(DateFormatPattern) });
        xmlAuthor.Add(new XElement(AuthorElementDayOfDeath) { Value = author.DayOfDeath.ToString(DateFormatPattern) });

        return xmlAuthor;
    }

    #endregion

    #region Retrieve Authors

    public ObservableCollection<Author> RetrieveAuthorCollection()
    {
        var authorCollection = new ObservableCollection<Author>();

        var authorList = Elements(AuthorsNodeName, AuthorNodeName);

        foreach (var xmlAuthor in authorList)
        {
            var authorId = xmlAuthor.Attribute(AuthorElementAuthorId);
            var firstname = xmlAuthor.Attribute(AuthorElementFirstname);
            var lastname = xmlAuthor.Attribute(AuthorElementLastname);
            var profession = xmlAuthor.Element(AuthorElementProfession);
            var dayOfBirth = xmlAuthor.Element(AuthorElementDayOfBirth);
            var dayOfDeath = xmlAuthor.Element(AuthorElementDayOfDeath);

            if (authorId != null) tmpAuthor.AuthorId = Convert.ToInt32(authorId.Value);
            if (firstname != null) tmpAuthor.Firstname = firstname.Value;
            if (lastname != null) tmpAuthor.Lastname = lastname.Value;
            if (profession != null) tmpAuthor.Profession = profession.Value;
            if (dayOfBirth != null) tmpAuthor.DayOfBirth = Convert.ToDateTime(dayOfBirth.Value);
            if (dayOfDeath != null) tmpAuthor.DayOfDeath = Convert.ToDateTime(dayOfDeath.Value);

            authorCollection.Add(tmpAuthor);
        }

        return authorCollection;
    }

    #endregion

    #region Retrieve next IDs

    public int NextAuthorId()
    {        
        var authorWithHighestId = NextElementId(AuthorsNodeName,AuthorNodeName,AuthorElementAuthorId);

        // Note this isn't exactly thread safe if that is a concern in this application
        return authorWithHighestId++;
    }

    public int NextQuoteId()
    {
        var quoteWithHighestId = NextElementId(QuotesNodeName,QuoteNodeName,QuoteElementQuoteId);
        return quoteWithHighestId++;
    }

    #endregion

    private int NextElementId(string nodesName, string nodeName, string elementId)
    {
        var noteList = Elements(nodesName, nodeName);

        return noteList.Any() ? noteList.Max(quote => (int)quote.Attribute(elementId)) : 0;     
    }

    private List<Element> Elements(string nodesName, string nodeName)
    {
        return xmlQuotes.Descendants(nodesName).Descendants(nodeName).ToList();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for the great review & explanation! The NextId() method should be thread safe now, I increment the IDs using Interlocked.Increment(ref quoteWithHighestId). \$\endgroup\$ – Joel Apr 19 '14 at 17:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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