Refactor code for usability

Question

I need help to refactor my code. I usually had a hard time figuring out how to make my code reusable.

I have an XML file that hold the data for each Tag element. Tag element should have child nodes LastClocked, and TotalClocked. I first thought of creating Tag object and do serialization. But, I found Linq to XML is much easier. I would really appreciate if you guys can tell me what to improve for my code. Thank you.

namespace StopWatch.Models
{
    public class TagCollection
    {
        private XElement doc;
        private IEnumerable<XElement> tagElements;

        public TagCollection()
        {
            if(File.Exists("TagsData.xml"))
            {
               doc = XElement.Load("TagsData.xml");
            }
            else
            {
                //TODO: Create XML
            }
        }

        public void Save(TimeSpan clocked, string tags)
        {
            tagElements = from t in doc.Elements("Tag")
                          where (string)t.Attribute("Name") == tags
                          select t;

            TimeSpan lastClocked = TimeSpan.Parse((string)
                                   (from lc in tagElements.Descendants("LastClocked")
                                    select lc).First());

            lastClocked = lastClocked.Add(clocked);

            if (!tagElements.Any())
            {
                Insert(clocked, tags);
            }
            else
            {
                Update(clocked, lastClocked);
            }

            doc.Save("TagsData.xml");
        }

        private void Update(TimeSpan clocked, TimeSpan lastClocked) 
        {
            foreach(XElement tagElement in tagElements)
            {
                tagElement.SetElementValue("LastClocked", clocked.ToString());
                tagElement.SetElementValue("TotalClocked", lastClocked.ToString());
            }
        }

        private void Insert(TimeSpan clocked, string tags) 
        {
            XElement newTag = new XElement("Tag",
                                    new XAttribute("Name", tags),
                                    new XElement("LastClocked", clocked.ToString()),
                                    new XElement("TotalClocked", clocked.ToString()));

            doc.Add(newTag);
        }
    }
}

Answer 1

Your code seems very procedural - you're worrying about implementing how things will be done before you think about what those things are and how you'd like to use them. You don't have any properties, just methods, which I don't particularly like.

From what I understand you want to write a reusable wrapper from some XML, that's going to fit in a consistent format. Do you have a class that represents that object?

public TagElement
{
    private XElement tagXml
    public string Name
    public DateTime LastClocked
    public DateTime TotalClocked
}

Now I'd simply make each of those properties point into the private XML object get / set their values.

public TagElement(string name, DateTime initialValue)
{        
    tagXml = new XElement("Tag", 
                 new XAttribute("Name", name),
                 new XElement("LastClocked", initialValue),
                 new XElement("TotalClocked", initialValue));
}

public DateTime LastClocked
{
    get
    {
        return tagXml.Descendents("LastClocked").Single().Value;
    }
    set
    {
        tagXml.Descendents("LastClocked").Single() = value;
    }
}

You can still have a Save method that persists the XElement to a file. You probably also want a collection class that wraps the individual TagElements.

public TagCollection
{
    public List<TagElement> TagElements { get; set; }
}

So you can then use

var collection = new TagCollection("myFile.xml");
// constructor for TagCollection needs to parse the XML, and create TagElements

var fooElement = collection.TagElements.Single(tag => tag.Name == "foo");
fooElement.LastClocked = DateTime.Now;

collection.TagElements.ForEach(tag => tag.TotalClocked = DateTime.Now);

In summary, prefer to use properties to get and set values than using methods. Abstract away the XML so that when you're using the class you do not know or care that it is being persisted to XML.