Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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?

share|improve this question
1  
Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers. – Vogel612 Nov 9 '15 at 22:16
1  
Thank you @Vogel612, didn't know there are such rules, I will try making another question after I finish from this question :) – Walid Nawfal Sabihi Nov 9 '15 at 22:20

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.

share|improve this answer
    
Thank you for the feedback, will put it to good use! – Walid Nawfal Sabihi Nov 9 '15 at 22:27

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 
share|improve this answer
    
Thank you for the info, I am a beginner-intermediate programmer so that benefits me alot :) – Walid Nawfal Sabihi Nov 10 '15 at 12:55

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.