Reading Values From Enviromental Monitor XML Feed

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;
    }
}

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();

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.

StackExchange Code Review Q#78226, answer score: 7