Generate URLs, Download, extract and add files

Question

So lets have a try. My first console application and I want to know if its good or bad.

The code should do this:

1. You have a bunch of zipped folders in a directory
2. You extract them.
3. Get a specific XML file where a specific URL Link is inside
4. Get the two files from the URL Link (extension .ere and .ter)
5. Put them into the extracted folder

6. Zip the new folder with the new files

   namespace MyApp
   {
       class Program
       {
           static void Main(string[] args)
           {
               try
               {
                   DirectoryAvailable(args[0]); //serverpath
               }
               catch (Exception e)
               {
                   Console.WriteLine(e.Message);
                   DisplayHelp();
               }
               Environment.Exit(0);
           }
   
           public static void DirectoryAvailable(string server)
           {
               CreateDirectory(server);
               GoThroughFolder(server);
               DeleteCreateOutputFolder(server);
   
               numberOfZip = Directory.GetFiles(server + @"\OutputZip").Length;
   
               Console.Write("Job done");
               Console.ReadLine();
           }
   
           public static void GoThroughFolder(string s)
           {
               string[] files = Directory.GetFiles(s, "BGBLA*" + ".zip", SearchOption.AllDirectories);
               foreach (var item in files)
               {
                   string outputPath = s + @"\Output\";
                   string outputPathZip = s + @"\OutputZip\";
                   string getBGBLA = Path.GetFileNameWithoutExtension(item);
                   string getXmlPath = string.Empty;
   
                   UnzipFile(item, outputPath + getBGBLA);
                   outputPath += getBGBLA;
   
                   if (string.IsNullOrEmpty(Path.GetExtension(outputPath)))
                       getXmlPath = outputPath + @"\" + getBGBLA + ".xml";
   
                   GetHtmlFile(getXmlPath, outputPath);
                   ZipFilePackaging(outputPath, outputPathZip + getBGBLA + ".zip");
                   Console.WriteLine("Info: " + outputPathZip + getBGBLA + ".zip");
               }
           }
   
           private static void UnzipFile(string zipFile, string outputPath)
           {
               try
               {
                   ZipFile.ExtractToDirectory(zipFile, outputPath);
               }
               catch (Exception e)
               {
                   Console.WriteLine(e.Message);
                   Environment.Exit(0);
               }
           }
   
           private static void ZipFilePackaging(string zipFile, string outputPath)
           {
               try
               {
                   ZipFile.CreateFromDirectory(zipFile, outputPath);
               }
               catch (Exception e)
               {
                   Console.WriteLine(e.Message);
                   Environment.Exit(0);
               }
           }
   
           private static void GetHtmlFile(string s, string outputPath)
           {
               //XML variablen
               string selectedNodeToElement = @"//ns:OgdDocumentReference/ns:Data/ns:Dokumentliste/ns:ContentReference/ns:Urls/ns:ContentUrl/ns:Url";
   
               string dokumentName = string.Empty;
               string hauptDokumentXml = "Hauptdokument.xml";
   
               //File-Exts
               string htmlExtension = ".html";
               string erechtExtension = ".ere";
               string xmldsigExtension = ".ter";
               string cooFileName = "COO";
   
               //vars for download files
               string eRechtDownloadLink = string.Empty;
               string eRechtFileName = string.Empty;
               string dSigDownloadLink = string.Empty;
               string dSigFileName = string.Empty;
   
               //XML laden
               XmlDocument doc = new XmlDocument();
               doc.Load(s);
   
               //set namespace
               string currentNamespace = doc.DocumentElement.FirstChild.NamespaceURI;
               XmlNamespaceManager ns = new XmlNamespaceManager(doc.NameTable);
               ns.AddNamespace("ns", currentNamespace);
   
               foreach (XmlNode htmlUrlMain in doc.DocumentElement.SelectNodes(selectedNodeToElement, ns))
               {
                   //catch main document
                   if (htmlUrlMain.InnerText == hauptDokumentXml)
                   {
                       dokumentName = Path.GetFileNameWithoutExtension(htmlUrlMain.InnerText);
                   }
   
                   //check if html exists
                   if (Path.GetExtension(htmlUrlMain.InnerText) == htmlExtension)
                   {
                       //check if ioo exists
                       if (htmlUrlMain.InnerText.Contains(cooFileName))
                       {
                           dokumentName = Path.GetFileNameWithoutExtension(htmlUrlMain.InnerText);
                       }
   
                       //*.ere
                       eRechtDownloadLink = GenerateErechtLink(htmlUrlMain.InnerText);
                       eRechtFileName = outputPath + @"\" + dokumentName + erechtExtension;
                       DownloadFile(eRechtDownloadLink, eRechtFileName);
   
                       //*ter
                       dSigDownloadLink = GenerateDsigLink(htmlUrlMain.InnerText);
                       dSigFileName = outputPath + @"\" + dokumentName + xmldsigExtension;
                       DownloadFile(dSigDownloadLink, dSigFileName);
                   }
   
               }
           }
   
           private static string GenerateErechtLink(string urlToErechtFile)
           {
               try
               {
                   var res = Path.ChangeExtension(urlToErechtFile, "erecht");
                   return res;
               }
               catch (Exception e)
               {
                   Console.WriteLine(e.Message);
                   throw;
               }
           }
   
           private static string GenerateDsigLink(string urlToXmldsigFile)
           {
               try
               {
                   var res = Path.ChangeExtension(urlToXmldsigFile, "xmldsig");
                   return res;
               }
               catch (Exception e)
               {
                   Console.WriteLine(e.Message);
                   throw;
               }
           }
   
           private static void DownloadFile(string url, string pathName)
           {
               try
               {
                   var client = new WebClient();
                   client.DownloadFile(url, pathName);
               }
               catch (Exception e)
               {
                   Console.WriteLine(e.Message);
                   throw;
               }
           }
   
           private static void DeleteCreateOutputFolder(string serverpath)
           {
               Directory.Delete(serverpath + @"\Output\", true);
           }
   
           private static void CreateDirectory(string server)
           {
               try
               {
                   Directory.CreateDirectory(server + @"\Output");
                   Directory.CreateDirectory(server + @"\OutputZip");
               }
               catch (Exception e)
               {
                   Console.WriteLine(e.Message);
                   throw;
               }
           }
       }
   }
   

So my problems I have to deal with:

1. I am mixing english and german (vars, comments, names, ..). How to deal with it when german is main?
2. I am not sure if this is the best way to do it? Maybe there is something, where I can extract, add files, zip again in one method/lib/...
3. I have problems with setting try/catch. I don't know where I have to set it exactly, in my first tries I have the try/catch phrase everywhere...
4. Mixing public/private. In this project, I wasn't sure what I should use in the project (if it grows)
5. Declaring variables: Should each string be defined? Or just when I am using it more than once. Actual I have a mix. :(

Answer 1

I'll first try to answer your questions then I'll go through the rest.

I am mixing english and german (vars, comments, names, ..). How to deal with it when german is main?

This should be a company policy, best scenario is to have everything in English (because everyone else will be able to review and extend your code). If it's not possible then you should limit the foreign language to comments.

I am not sure if this is the best way to do it? Maybe there is something, where I can extract, add files, zip again in one method/lib/...

There is not a single method to call to perform this task but see later.

I have problems with setting try/catch. I don't know where I have to set it exactly, in my first tries I have the try/catch phrase everywhere...

This is a really important question. For more complex applications you should have (and follow) a well-defined exception handling policy but in general you should catch an exception only when you can do something about it. For example:

try
{
    var res = Path.ChangeExtension(urlToXmldsigFile, "xmldsig");
    return res;
}
catch (Exception e)
{
    Console.WriteLine(e.Message);
    throw;
}

This is not the right™ way to handle an exception:

• You're catching a generic Exception. Is it catching OutOfMemoryException (for example) an appropriate action here? You exactly know what Path.ChangeExtension() may throw (only ArgumentException) then, if it makes sense, that's the only exception you may catch here. However...
• It doesn't make sense to catch an exception here. ArgumentException is generated if path contains invalid characters. In this case it's a possibility (because caller read this parameter from an external XML file which may be invalid) but this validation should be performed elsewhere (otherwise you will need to repeat the same validation/exception handling code here and there). Validate the path after it has been read and then consider it valid.
• You're logging to console. First of all errors should go to stderr then System.Console.Error stream. More than that you're tying this code to console applications (more on that later). What if it will run unattended as a service? What if you'll make a GUI? Abstract logging and user interaction into an injected interface (it'll also make testing easier).
• You're logging and then re-throwing. It will be catched again in an outer handler (in Main()) and you'll repeat the same code. You can simply remove the inner handler and nothing will change in program behavior.

Mixing public/private. In this project, I wasn't sure what I should use in the project (if it grows)

Quick rule: does it need to be called outside current scope (class/assembly)? Make it public. If not then pick the less permissive visibility rule (for example internal if it has be called/used inside the assembly but not outside and private if it's a class member which is used only inside the class).

Note that testing (unless you place your tests in the same assembly or you declare tests assembly as friend) may impose to relax class visibility rules.

In your case every class member may be private. Class itself and its Main() method may also be private (because they're not invoked directly by code) but someone thinks they're conceptually public because they're the program entry point.

Declaring variables: Should each string be defined? Or just when I am using it more than once. Actual I have a mix. :(

Not everything you declared as a variable...is a variable. You're trying to avoid magic constants in your code and it's a good thing but you should move them to private const string fields (if at class level) or const string if at method level).

Also you're declaring all the variables at the beginning of the method. It's not required in any modern language and it greatly hurts readability (especially for methods longer than few lines).

---

There are many things to say but the most important one (IMO) is that program flow and behavior are not obvious. You made a great job in this question to explain in plain English what you're doing. To read a program shouldn't be more difficult than that. Let's try to translate to code your description.

You have a bunch of zipped folders in a directory

static void AddExternalFilesToZips(IEnumerable<string> zipFiles)
{
    foreach (var zipFilePath in zipFiles)
        AddExternalFilesToZip(zipFilePath);
}

Don't be lazy to find better names (you know more about the context), use descriptive self-explaining identifiers. For example you have a parameter named s which signifies nothing.

You extract them.

Get a specific XML file where a specific URL Link is inside

Get the two files from the URL Link (extension .ere and .ter)

Put them into the extracted folder

Zip the new folder with the new files

static void AddExternalFilesToZip(string zipPath)
{
    var temporaryPath = GetTemporaryPathForExtractedFiles();

    ExtractZipToTemporaryFolder(zipPath, temporaryPath);
    CopyExternalFilesToTemporaryFolder(temporaryPath);
    RecreateZipFileFromTemporaryFolder(zipPath, temporaryPath);
}

Now we can read few lines of code and we already have the exact idea of what will happen (we may investigate how if/when we're interested).

To implement these methods should be trivial, let's consider it an exercise for the reader. Few things to keep in mind:

• Keep your methods as short as possible. Each method should do exactly one thing and if you can't find a good name then probably it's doing too much. For example GoThroughFolder() does not say what the method is doing, I have to read and understand its code (and called functions) to have a vague idea.
• Do not create paths by hand, use Path.Combine().
• Validate your inputs and declare pre-conditions that a method expects (for example using Debug.Assert()).
• Write code with testing in mind. More logic you can abstract and more tests you can write to handle corner cases.
• Declare constants as appropriate (see comments before).
• Declare variables where you use/initialize them. To assign a dummy never used value is useless.

Your Main() method might be simplified. Environment.Exit(0) is redundant and you're returning 0 even in case of errors. There is much more to write for a decent exception handling (and nested exceptions should be considered) but you may start with:

int Main(string[] args)
{
    switch (args.Length)
    {
        case 0:
            DisplayHelp();
            return ExitCodeOk;
        case 1:
            break;
        default:
            Console.Error.WriteLine("Invalid number of arguments.");
            DisplayHelp();
            return ExitCodeError;
    }

    try
    {
        // ...
    }
    catch (Exception e)
    {
        Console.Error.WriteLine(e.Message);
        return ExitCodeError;
    }

    return ExitCodeOk;
}

private const int ExitCodeOk = 0;
private const int ExitCodeError = 1;

Later in future you may want to move all this code into a separate class. It has few benefits you should seriously consider:

• You can reuse it for a GUI application, a service or whatever.
• It's easier to test than static methods (because you can easily inject dependencies).

Answer 2

IMHO the Program class of a console app should contain as little as possible; mine usually contain little more than an instantiation of another class and a call to its public method. That way you at least escape the requirement for methods etc. to be static.

You class is approx. 200 lines and is supposed to do half a dozen things: that's way to much. "Ideally" each of the functions should be a class of their own; though sometimes that could be overkill.

---

Method names should tell you what they do; yet DirectoryAvailable sounds like a boolean and calls the other methods which execute your logic. This is bad design.

---

string[] files = Directory.GetFiles(s, "BGBLA*" + ".zip", SearchOption.AllDirectories);

Why bother with storing the result in files? Just use Directory.GetFiles() in your foreach.

---

Use descriptive names for variables etc.; string s tells me absolutely nothing.

---

string outputPath = s + @"\Output\"; Do not use string concatenation when constructing paths: that is why Path.Combine() exists.

---

Your code is full of Console.WriteLine();. This means there's a strong coupling between your "UI" (a console) and your actual code. Instead consider using a logging system like Serilog to handle all that.

---

Avoid mixing German and English names. It adds a level of complexity to your code, even if you're fluent enough in both.

---

GenerateErechtLink and GenerateDsigLink are virtually the same method and could easily be reduced to a single method with two parameters (sourceUrl, newExtension). And does there really need to be a try...catch block around this? Does this functionality even need to be in a separate method? Why not just call Path.ChangeExtension()?

---

The folder name @"\Output\" is used in at least three separate methods. If you ever decide to rename this folder you'll need to change this in three places: that's a recipe for bugs. Such things should be defined as a const in a configuration class, or retrieved from a .config file.

---

This might seem like overkill, but it really benefits yourself if you apply similar coding standards to small projects as you'd do on larger ones. Because such small projects might grow, and then it's nice if you don't need to rewrite a lot just to introduce some small additional functionality.

Answer 3

1. I am mixing english and german (vars, comments, names, ..). How to deal with it when german is main?

Generally speaking, company policy takes precedence. In absence of company policy, try to stick to English.

However, as a Dutch speaker, there are valid reasons to mix Dutch and English. We decided to keep using Dutch entity names, since the end users refer to these entities by the Dutch name that they've had for years.
If we were to translate these to English, people would start getting confused between possibly close-but-distinct words, e.g. between a discount and a rebate.

The same applies to developer confusion. E.g. when a developer is talking about the application to a user/analyst/..., he provides an explanation in Dutch, using Dutch names. It's easier for the developer to find the relevant code if it uses that same Dutch name that the user/analyst uses, compared to having to translate it to English and hoping that he translates it the same way as the previous developer. Especially because words can add/lose ambiguity in a different language.

But everything else remains in English. You get method names such as GetDeletedPersoon (persoon is Dutch for person), which is a bit jarring to read, but every other alternative would end up causing more confusion. It's the lesser of the evils.

---

3. I have problems with setting try/catch. I don't know where I have to set it exactly, in my first tries I have the try/catch phrase everywhere...

Suppose you have three lines of code, all of which consist of a call to a remote resource.

var a = service.GetA();
var b = service.GetB();
var c = service.GetC();

You're trying to add a try/catch to handle exceptions when the service is unavailable. Initially, you try to wrap every individual call:

try
{
    a = service.GetA();
}
catch(ServiceIsUnavailableException ex)
{
    log.Error("Service down!");
}

try
{
    b = service.GetB();
}
catch(ServiceIsUnavailableException ex)
{
    log.Error("Service down!");
}

try
{
    c = service.GetC();
}
catch(ServiceIsUnavailableException ex)
{
    log.Error("Service down!");
}

But then you notice the behavior. When A fails, the code handles the exception, but it then tries to do B. This doesn't make sense. If A already failed, there's no point to trying B and C.

Therefore, you instead try/catch the entire block:

try
{
    a = service.GetA();
    b = service.GetB();
    c = service.GetC();
}
catch(ServiceIsUnavailableException ex)
{
    log.Error("Service down!");
}

This way, the code won't try to call the service again when an exception has already occured.

Rule of thumb:

• Where possible, shift the code so that you can group a single point of failure (like the service in the example above) in a single try block.
• The catch should occur where you want the code flow to resume after handling the exception. This can be in the middle of your method (if you can still do meaningful work despite the outage), or at the end of the method (if you can't do any meaningful work due to the outage).
• Try to catch specific exceptions (e.g. FileNotFoundException) rather than catching the base Exception class.

---

4. Mixing public/private. In this project, I wasn't sure what I should use in the project (if it grows)

Private by default. Protected/public/internal is only used on a need-to-have basis.

---

5. Declaring variables: Should each string be defined? Or just when I am using it more than once. Actual I have a mix. :(

I suggest defining all variables, and not trying to inline them. Especially when still learning to code, you should try to avoid making complex lines of codes.

Better three easy to read lines, than one hard to read line of code.

This also makes it easier to use watches during debugging, so you can check all the string values if you need to.