Reading Values From Enviromental Monitor XML Feed

Question

I have written the below code to handle accessing and parsing the data presented in XML format from our server room environmental monitor.

The plan is to call this class from a controller/view handling the "dashboard" on our Intranet page.

Is there any way I could optimize the class? should the methods really be static or would it be better to create an instance of the class and call the methods that way?

public class EnviromentialMonitorInterface
{        
    private static XmlDocument LoadInXML()
    {
        string environmentalMonitorUrl = "http://192.168.6.95/data.xml";
        //string environmentalMonitorUrl = ConfigurationManager.AppSettings["WhiteListedDomainNames"];

        XmlDocument xmlDoc = new XmlDocument();
        xmlDoc.Load(environmentalMonitorUrl);

        return xmlDoc;
    }

    public static double GetTemperature()
    {            
        var extraction = LoadInXML().SelectSingleNode("//field[@key='TempC']").Attributes.GetNamedItem("value").Value;

        double value = Convert.ToDouble(extraction);

        return value;
    }

    public static int GetHumidity()
    {
        var extraction = LoadInXML().SelectSingleNode("//field[@key='Humidity']").Attributes.GetNamedItem("value").Value;

        int value = Convert.ToInt32(extraction);

        return value;
    }
}

Abbas · Accepted Answer · 2015-01-21 16:04:56Z

Naming/spacing:

Don't use Interface in a class name, it might be confusing. Just make it EnviromentialMonitor. A name like extraction doesn't say much, try xmlTemperature and xmlHumidity for example.
Be consistent when using var:

Use implicit typing for local variables when the type of the variable is obvious from the right side of the assignment, or when the precise type is not important.

With extraction you correctly used var, but you should also have done it for value in both methods since you know what the type is from the right side of that assignment.

I don't like the linebreaks you put everywhere, it makes the code harder to read. What I usually do is block a section withy declarations and then block a section with methods, for example:
```
var a = 1;
var b = 2;
var items = new List<int>();

items.Add(a);
items.Add(b);
```

Double code:

Both the GetTemperature and GetHumidity method have a very similar structure. You could refactor the XML-handling in a separate method which you call from the other two. Both would return a double then.

public static double GetTemperature()
{            
    return GetValueFromXml("//field[@key='TempC']");
}

public static double GetHumidity()
{
    return GetValueFromXml("//field[@key='Humidity']")
}

private static double GetValueFromXml(string field)
{
    var xmlValue = LoadInXML().SelectSingleNode(field).Attributes.GetNamedItem("value").Value;
    retrun Convert.ToDouble(xmlValue);
}

IP address:

Perhaps the IP won't change that fast, or at all for that matter but when you want to write good code, you make it independent. Pass the IP address as a parameter to the method or get it from a config/resource file like the line that is commented out in your question.

Static vs. Instance:

Since this is kind of a utility class and you're nowhere creating instances and/or modyfying properties/fields in the class, you can leave it static.

Also, static methods can be a little bit faster than instance methods. More reading on this from MSDN: CA1822: Mark members as static. Basically:

Members that do not access instance data or call instance methods can be marked as static (Shared in Visual Basic). After you mark the methods as static, the compiler will emit nonvirtual call sites to these members. Emitting nonvirtual call sites will prevent a check at runtime for each call that makes sure that the current object pointer is non-null. This can achieve a measurable performance gain for performance-sensitive code. In some cases, the failure to access the current object instance represents a correctness issue.

Two comments on your comments: (1) the value of extraction is really not clear from the RHS, I would explicitly specify the type in this case. (2) The static/instance performance difference is so amazingly negligible in this case - these methods are calling LoadXml, the potential nanosecond difference between instance and static methods means nothing compared to readability. I would also leave it as static, but not at all for reasons of performance. It's true that you do not state that as the primary reason, just saying that OP should not expect any noticeable difference. — Rotem
– Rotem, Commented Jan 21, 2015 at 16:10
@Rotem About the first comment: the definition on the usage of var states that you should use it when it is obvious from the RHS or when the type is not important. That last is the case in this case, but I understand your point. About the second comment, you're absolutely right, the difference is hardly noticable but I thought it might be best to inform the OP fully. Thanks for the comment though! :) — Abbas
– Abbas, Commented Jan 21, 2015 at 20:15

Stack Exchange Network

Reading Values From Enviromental Monitor XML Feed

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Reading Values From Enviromental Monitor XML Feed

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions