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 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);
        }
    }
}
share|improve this question

1 Answer 1

up vote 5 down vote accepted

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.

share|improve this answer
    
I have those class that represent the object for TagElement, but I deleted it since I'm not really sure how it helps. I have little experience in OOP, so your factorization code does help. I'm trying to make a wrapper for XML as db, so I could query and save data. It should be a small file, so that's why I'm using XML. Thanks a lot for your input. –  Shulhi Sapli Feb 13 '11 at 9:52
    
Hopefully this will give you some ideas; there are other ways to do it but this is just what came to mind. –  Kirk Broadhurst Feb 13 '11 at 10:04

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.