Opening an XML File and Returning the XmlDocument

Question

I'm working on my first Windows Forms tool for a project; this is my first time using C# extensively. Right now, my only objective is to be able to open an XML file and have the resulting XML in hand. I know this works, but I want to know if there's a better, or proper, way to do it.

There is also the issue that when I was first trying to load it, it didn't like the fact that I was using if statements, so the return class was giving me an error that said that not all of the branches returned a value. So, I had to throw in a messy else statement that I don't like.

One thing to note; if the empty XML file is returned, I'll have a statement for handling the returned XML document to do nothing with it.

class LoadXML
{
 public XmlDocument LoadXML_Window(object sender, System.EventArgs e)
 {
 // The stream will hold the results of opening the XML
 Stream myStream = null;
 OpenFileDialog openFileDialogXML = new OpenFileDialog();
 openFileDialogXML.InitialDirectory = "C:\\";
 openFileDialogXML.Filter = "XML Files|*.xml";
 openFileDialogXML.RestoreDirectory = true;
 if(openFileDialogXML.ShowDialog() == true)
 {
 myStream = openFileDialogXML.OpenFile();
 using (myStream)
 {
 try
 {
 // Successfully return the XML
 XmlDocument parsedMyStream = new XmlDocument();
 parsedMyStream.Load(myStream);
 return parsedMyStream;
 }
 catch (XmlException ex)
 {
 MessageBox.Show("The XML could not be read. " + ex);
 XmlDocument emptyMyStream = new XmlDocument();
 return emptyMyStream;
 }
 }
 }
 else
 {
 // Return an empty XmlDocument if the open file window was closed
 XmlDocument emptyMyStream = new XmlDocument();
 return emptyMyStream;
 }
 }
}

Answer 1

Here's how I would refactor your code:

• An event handler sholdn't return any values, it's better to save the result in a property or a field in this case.

√ DO use a return type of void for event handlers. An event handler can invoke multiple event handling methods, possibly on multiple objects. If event handling methods were allowed to return a value, there would be multiple return values for each event invocation.

Event Design

• You shouldn't do too much work inside an event handler, it's better to call a specialized method that can also be reused elsewhere later.
• OpenFileDialog needs to be disposed: see Component.Dispose Method which is one of the base class of this dialog.
• If you check the if(!openFileDialogXML.ShowDialog()) for a false value and immediately return a default value you can reduce nesting.
• If possible it's better to name a class with a noun (e.g. XmlLoader) rather then like a method LoadXml.
• Now that the unnecessary nesting is removed you can remove the Stream myStream = null; and put it inside the using.
• The last step would be to replace the repeating creation of an empty XmlDocument by a method and call it where necessary. You might want to add some default elements later so now you can do it in only one place. If the XmlDocument is no longer empty you might rename the method to CreateDefaultXmlDocument but it's important that you do it only once. You could forget to adjust the other lines if your code repeates.

---

class XmlLoader
{
 public XmlDocument XmlDocument { get; private set; }
 private XmlDocument CreateEmptyXmlDocument()
 {
 // Return an empty XmlDocument if the open file window was closed
 XmlDocument emptyMyStream = new XmlDocument();
 return emptyMyStream;
 }
 public XmlDocument OpenXml()
 {
 using(var openFileDialogXML = new OpenFileDialog())
 { 
 openFileDialogXML.InitialDirectory = "C:\\";
 openFileDialogXML.Filter = "XML Files|*.xml";
 openFileDialogXML.RestoreDirectory = true;
 if(!openFileDialogXML.ShowDialog())
 {
 return CreateEmptyXmlDocument();
 }
 // The stream will hold the results of opening the XML
 using (var myStream = openFileDialogXML.OpenFile())
 {
 try
 {
 // Successfully return the XML
 XmlDocument parsedMyStream = new XmlDocument();
 parsedMyStream.Load(myStream);
 return parsedMyStream;
 }
 catch (XmlException ex)
 {
 MessageBox.Show("The XML could not be read. " + ex);
 return CreateEmptyXmlDocument();
 }
 }
 }
 }
 public void LoadXML_Window(object sender, System.EventArgs e)
 {
 XmlDocument = OpenXml();
 }
}