current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

The configuration file of my app is similar to,

<?xml version="1.0" encoding="utf-8" ?>
<Items>
      <Item Name="Coffee"
            Cost="10"
            Image="itemCoffee.png" />
      <Item Name="Tea"
            Cost="10"
            Image="itemTea.png" />
      <Item Name="Vada"
            Cost="10"
            Image="itemVada.png" />
  </Items>

Just trying to read the above small configuration file and I wrote this method.

public static class Configuration
{
    public static T DeSerialize<T>(string filePath)
    {
        if (!System.IO.File.Exists(filePath))
        {
            throw new System.IO.FileNotFoundException(filePath);
        }

        var serializer = new System.Xml.Serialization.XmlSerializer(T);
        return (T)serializer.Deserialize(new FileStream(filePath, FileMode.Open));
    }
}

  1. Where should I use using in this code? (Because, I never ever dispose the new FileStream that I wrote in this method)

  2. Is this an overkill for reading this simple xml file?

share|improve this question
5  
If that XML is really the configuration file of your app, I think you've missed an opportunity to leverage .net application settings. –  Mat's Mug♦ Jan 31 '14 at 1:46

1 Answer 1

up vote 3 down vote accepted

  1. I would definitely wrap the FileStream object into a using clause like this:

    using (Stream reader = new FileStream(filePath, FileMode.Open))
{
  var serializer = new System.Xml.Serialization.XmlSerializer(typeof(T));
  return (T)serializer.Deserialize(reader);
}

    This way the Dispose method of the FileStream gets called immediately. Otherwise it will be delayed until the FileStream object gets garbage collected. This may not be a big problem in practice in simple applications where you read the configuration only once but is good practice to do it anyway. Also note that you have to use XmlSerializer(typeof(T)) instead of XmlSerializer(T).

  2. My opinion is that it is overkill to create a generic method if you have only one kind of configuration file in your app. This method might be hard to read for a maintainer who is not sufficiently comfortable with generics and it has no benefit in making your code DRY-er if you have only one type of configuration file.

share|improve this answer
5  
I agree on your first point, but not on your second. I do not think it's overkill to create a generic method for this. If there are any programmers in the wild that do not understand generics they shouldn't be working as programmers. And by making a generic method he will be able to reuse his method whenever he needs it - whether its today or tomorrow or months from now. –  Max Jan 30 '14 at 7:43
    
@Andris: Thank you. I prefer Answer for point 1 and go with @Max's comment for Point 2. Thanks. –  now he who must not be named. Jan 30 '14 at 8:07
1  
@nowhewhomustnotbenamed. Point 2 is a judgement call. It is quite readable but it still feels like YAGNI to me. –  Andris Jan 30 '14 at 8:11
1  
YAGNI and DRY are sometimes at odds. Particularly here where the required effort to create the generic is almost nothing, and the chances of reusing the method are high. –  Magus Jan 30 '14 at 16:19

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.