Extracting an XML config file from ZIP file and parsing it

Question

I am currently working with an app that should load various info on a very specific zip file which contains the XML config file, using the DotNetZip library on C#, here's the code:

        public static void InitializeData(string configFilePath)
    {
        if (File.Exists(configFilePath))
        {
            using (ZipFile zip1 = ZipFile.Read(configFilePath))
            {
                zipEntriesCount = zip1.Count;
                if (zip1.ContainsEntry(installConfigFileName))
                {
                    var entries = zip1.SelectEntries(installConfigFileName);
                    ZipEntry[] entriesArray = new ZipEntry[entries.Count];
                    entries.CopyTo(entriesArray, 0);
                    entriesArray[0].Extract(installConfigStream);
                    installConfigData = StreamToString(installConfigStream);
                    XmlDocument xmlDoc = new XmlDocument(); // Create an XML document object
                    xmlDoc.Load(installConfigStream); // Load the XML document from the specified file

                    // Get elements
                    XmlNodeList XmlAppTitle = xmlDoc.GetElementsByTagName("appTitle");
                    XmlNodeList XmlAppCompany = xmlDoc.GetElementsByTagName("appCompany");
                    XmlNodeList XmlAppIconPath = xmlDoc.GetElementsByTagName("appIconPath");
                    XmlNodeList XmlAppLargeIconPath = xmlDoc.GetElementsByTagName("appLargeIconPath");

                    // Store app config data globally
                    // eg: globalVar = XmlVar[0].InnerText;
                    appTitle = XmlAppTitle[0].InnerText;
                    appCompany = XmlAppCompany[0].InnerText;
                    appIconPath = XmlAppIconPath[0].InnerText;
                    appLargeIconPath = XmlAppLargeIconPath[0].InnerText;
                }
            }
        }
    }

My question is, is there any better/more compact way of reaching same functionality?

Answer 1

The repeated part of your code can be extracted into a sub-function.

string InnerTextOfFirst(XmlDocument doc, string tagName)
{
  return doc.GetElementsByTagName(tagName)[0].InnerText;
}

Then use it like this:

appTitle = InnerTextOfFirst(xmlDoc, "appTitle");
appCompany = InnerTextOfFirst(xmlDoc, "appCompany");
appIconPath = InnerTextOfFirst(xmlDoc, "appIconPath");
appLargeIconPath = InnerTextOfFirst(xmlDoc, "appLargeIconPath");

This removes about a third of the inner most block.

---

Things to look out for:

• What happens if the xml file does not have one of the tags you are expecting? It's likely that this block of code won't know the correct thing to do in this case. You should write the code so that when this happens, it is clear to the caller what the real issue is.

• The C# naming convention for local variables is camelCase. While most of the variables are named correctly, the XmlNodeList variables are not.

• There is a comment that refers to global variables. Having global mutable variables will make it harder to reason about how the code go into a certain state. It would be better to create a simple data class and have the code that parses the file return a new instance of that class with the parsed values.

Answer 2

InitializeData method is doing too much, I would split your function into three parts

1) Getting collection of zipped files from config path.

By separating the method you can use this method anywhere else too

   private ZipEntry[] GetZippedEntries()
    {
        using (var zippedFile = ZipFile.Read("configFilePath"))
        {
            if (!zippedFile.ContainsEntry(installConfigFileName))
                return null;

            var entries = zippedFile.SelectEntries(installConfigFileName);

            var entriesArray = new ZipEntry[entries.Count];
            entries.CopyTo(entriesArray, 0);

            return entries.ToArray();
        }
    }

2) Getting the path of config data

    private string GetConfigurationData()
    {
        var zippedEntries = GetZippedEntries();
        if(zippedEntries==null)
            return null;

        zippedEntries[0].Extract(installConfigStream);
        return StreamToString(installConfigStream);
    }

3) Parsing and initialize variables

 public void Initialize()
    {
        var configurationFilePath = GetConfigurationData();
        if (string.IsNullOrEmpty(configurationFilePath))
            return;

        var xmlDoc = new XmlDocument();
        xmlDoc.Load(configurationFilePath); 

        var xmlAppTitle = xmlDoc.GetElementsByTagName("appTitle");
        var xmlAppCompany = xmlDoc.GetElementsByTagName("appCompany");
        var XmlAppIconPath = xmlDoc.GetElementsByTagName("appIconPath");
        var XmlAppLargeIconPath = xmlDoc.GetElementsByTagName("appLargeIconPath");

        appTitle = xmlAppTitle[0].InnerText;
        appCompany = xmlAppCompany[0].InnerText;
        appIconPath = XmlAppIconPath[0].InnerText;
        appLargeIconPath = XmlAppLargeIconPath[0].InnerText;
    }

You can abstract this code too by using @unholysampler method for getting inner text.

Other review details:

1) Naming convention : zip1 is not appropriate way to name a variable , please use a proper name.

2) Test negative condition first to reduce nesting. so rather than checking

  if (File.Exists(configFilePath))
  { 
  //your code 
  }

can rewrriten as

  if (!File.Exists(configFilePath))
    return;

   //your code