return value style question

Question

Lets say I have the following code

public static string GetXMLValue()
{
    XDocument settingsFile = XDocument.Load("Settings.xml");
    return settingsFile.Element("Settings").Element("GenericValue").Value;
}

It simply reads an XML Settings file and returns the GenericValue value. It can't be any simpler than that. Now my question is, would it provide any benifit (readability, performace, syntactically, maintainablitiy, etc.) to first place the return value in a string variable then return? Or is it best left the way it is?

Answer 1

From a readability perspective, assigning the values to a variable and returning it would definitely help.

Answer 2

There are a couple situations in which I see value in creating an auxiliary variable:

• I want to assert something about it as a precondition (e.g. not empty string; a minimum/maximum length; etc.)
• I am having trouble and I want to debug the value more easily.

Even in the absence of these, for such a nontrivial expression, I would create a local variable, to make the function more readable.

Answer 3

To be honest, the simplicity of the methods makes it readable even in "one" line:

public static string GetXMLValue() 
{ 
    return XDocument
             .Load("Settings.xml") 
             .Element("Settings")
             .Element("GenericValue")
             .Value; 
} 

Answer 4

would it provide any benifit [...] to first place the return value in a string variable then return? Or is it best left the way it is?

The function is so simple it just does not matter, so don't lose sleep about it. Once the function becomes more complex, you can always rethink this.

If for example you later need to run checks on the value before returning it, or want to log it for auditing reasons, a separate variable will make sense. Until then, leave it as it is.

As an aside:

What I find much more questionable is that you are reading an external resource (file) in a getter method. Invoking operations that can have side effects (such as reading a file) in a getter is bad style IMHO. That way for example every caller of the getter will have to handle IOExceptions from reading the file.

Consider changing this, for example by passing in the information via the constructor (either read the file from the constructor, or pass in an object that takes care of supplying the information). This will decouple your design, and simplify e.g. reuse and unit testing.