2
\$\begingroup\$

This is a follow up to this question. I got some very good answers for my process speed, and wanted to go ahead and share what I've done with the program. The new updates of the program consist of the following:

  • Put all the information handling into it's own class
  • Changed the way the filename is created, much quicker and just as random
  • Moved the actual zipping into it's own method, so it can be called instead of relying on the main method.

As for the speed, I created a basic benchmark using DateTime.Now and subtracting the time from when the process began to when the process completed. I then ran the program three times, and got these results:

  1. 56.5322256
  2. 1:12.0224298
  3. 58.7751815

With an average completion time of: 62.44327896666667

I know this seems slower, but this program is running through around 50 files with over 3GB worth of storage. As for doing the basic right click send to zip file, it takes well over 2 minutes to complete all the files.

What I'm looking for is some critique on what I've done, what did I do well, how can I improve this further, are there better ways to do what I've done etc..?

Source:

using System;
using System.IO;
using System.IO.Compression;

namespace ArchiveCreator
{
    /* 
     * This interface is used as a set point
     * for the information handling. All information
     * will be color coordinated in order to 
     * show the severity of what's happening with
     * the program itself. IE:
     * Red => Bad
     * Green => Good
     */

    public interface Information
    {
        string Say(string input);
        string Success(string input);
        string Warn(string input);
        string FatalErr(string input);
        string MinorErr(string input);
    }

    /* 
     * This class is where the interface is
     * inherited from. Basically this will
     * contain the color coordinating of the
     * information displayed from the interface.
     */

    public class ConsoleReport : Information
    {
        public string Success(string input)
        {
            Console.ForegroundColor = ConsoleColor.White;
            Console.WriteLine(input);
            return input;
        }

        public string Warn(string input)
        {
            Console.ForegroundColor = ConsoleColor.Yellow;
            Console.WriteLine(input);
            return input;
        }

        public string Say(string input)
        {
            Console.ForegroundColor = ConsoleColor.DarkCyan;
            Console.WriteLine(input);
            return input;
        }

        public string FatalErr(string input)
        {
            Console.ForegroundColor = ConsoleColor.DarkRed;
            Console.WriteLine(input);
            return input;
        }

        public string MinorErr(string input)
        {
            Console.ForegroundColor = ConsoleColor.DarkYellow;
            Console.WriteLine(input);
            return input;
        }

    }

    /* 
     * Main class of the program, basically this
     * class is what makes the program actually
     * run.
     */

    class Archive
    {
        static void Zip(string fromDir, string zipName)
        {
            ZipFile.CreateFromDirectory(fromDir, zipName, CompressionLevel.Fastest, true);
        }

        //Main method
        static void Main(string[] args)
        {

            /* 
             * Create the variables that will store
             * the required information. Basically
             * these are the information handling variable
             * that is derived from the class and a random
             * filename so that you won't overwrite
             * one of your zip files.
             */

            Information info = new ConsoleReport();
            string finalString = Path.GetRandomFileName();

            info.Say("Starting file extraction..");

            /* 
             * These variables are required in order to run
             * the program successfully. The day variable is
             * to add a day of archiving to the zip filename
             * this will help if you have a lot of zip files
             * and a lot of folders in that directory.
             */

            string day = DateTime.Now.ToString("MM-dd-yy ");
            string userName = Environment.UserName;
            string startDir = $"c:/users/{userName}/test_folder";
            string zipDir = $"c:/users/{userName}/archive/{day}{finalString}.zip";
            string dirName = $"c:/users/{userName}/archive";

            //Check if the directory exists
            info.Say("Attempting to create archive directory..");

            if (Directory.Exists(dirName))
            {
                info.MinorErr("Directory already exists, resuming extraction process");
            }

            else
            {
                //Create it if it doesn't
                info.Warn($"Creating archive directory here: {dirName}");
                Directory.CreateDirectory(dirName);
                info.Say("Directory created, resuming process..");
            }

            try
            {
                //Attempt to extract to zip file
                info.Say($"Attempting to extract files into: {zipDir}");
                Zip(startDir, zipDir);
                info.Success($"Extracted file successfully to: {zipDir}");
            }

            catch (Exception e)
            {

                /* 
                 * Catch any error that occurs during
                 * the archiving stage and log the error
                 * to a text file for further analysis
                 */

                var programPath = System.Reflection.Assembly.GetExecutingAssembly();
                info.FatalErr($"Something went wrong and the program cannot continue, exiting process with error code {e}..");
                info.FatalErr("Writing error to file for further analysis.");
                File.WriteAllText($"{programPath}/log/errorlog.txt", e.ToString());
            }

            info.Say("Press enter to exit..");
            Console.ReadLine();
        }
    }
}

Images of file before compression:

enter image description here enter image description here

After compression:

enter image description here

\$\endgroup\$
  • \$\begingroup\$ Out of interest, what's the compression ratio like (how much are you writing back to the disk) \$\endgroup\$ – forsvarir May 24 '16 at 18:37
  • \$\begingroup\$ @forsvarir Sorry I'm doing a test right now, I'll post some images when it's done \$\endgroup\$ – 13aal May 24 '16 at 19:14
  • \$\begingroup\$ @forsvarir Added pictures of the files before and after compression \$\endgroup\$ – 13aal May 24 '16 at 19:18
  • \$\begingroup\$ For benchmarks, you should use Diagnotics.Stopwatch or at very least use DateTime.UtcNow (faster and less problematic than DateTime.Now). But Stopwatch really is the right tool for the job. \$\endgroup\$ – Rick Davin May 24 '16 at 19:25
  • \$\begingroup\$ @RickDavin Cool, I'll look into it thank you \$\endgroup\$ – 13aal May 24 '16 at 19:28
5
\$\begingroup\$

Some stylistic comments:

  1. The standard naming conventions for interfaces are to prefix them with I. Of course, with a word that starts with I you get the silly IInformation, but that's ok, since Information isn't a good name for the interface anyway. Consider ILogger or something which actually explains what the object does.

  2. The method names of Information aren't very... well.. informative. Specifically the Say method. Is it just a generic output? If so, does it not have a severity level? There are standard, commonly accepted names for logging levels, which usually go from Trace -> Debug -> Info -> Warn -> Error -> Fatal. This might not be intrinsically clearer, but is a common pattern so will be familiar to others.

  3. You're running on Windows, I assume. If so, using unix-style forward slashes for directory separators works, but looks strange and isn't the native style. You might be doing it to avoid having to escape backslashes, but for that you can just use @ for verbatim strings (which should work fine with string interpolation)

  4. Your comments are... excessive. You don't need 3 lines of comments saying that you're now defining a variable holding a temporary filename, when the line is literally a call to "GetTempFilename". A huge amount of vertical space is wasted which could be used to seeing more of the code at once. Similarly, saying "these variables are required to run this program" seems pointless. Of course they are, or they wouldn't be there.

  5. While logging is good, does not having the output directory really warrant a Warning? It's a simple case that's easily handled by the code. It should be a Success (Info?) message at best - there's nothing for the user to take action about. Likewise, if the directory already exists, it really isn't an error, even a minor one. Errors are errors, something the admin or dev need to handle.

  6. You're concatenating the current username to "C:\Users\" to get to a user's home directory - this is error prone. What if the home directory is on a different drive? Or if it isn't called "Users" (e.g. on a Windows system in a different language)? Or if the user's directory doesn't map directly to his username (you might have both an "MyUsername" and "MyUsername.DOMAIN" folders for local and domain users).

Instead of concatenating, use the built-in method to get the user's profile directory:

var profileDir = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile);
\$\endgroup\$
  • \$\begingroup\$ These are some very valid points. This is really my first real program in c# \$\endgroup\$ – 13aal May 24 '16 at 20:21
  • 1
    \$\begingroup\$ @13aal Well, you're off to a good start, then. Most people's first programs are an uncommented jumble. Yours is very organized, so you just need some experience figuring out where comments are theoretically useful and where they actually help. \$\endgroup\$ – Avner Shahar-Kashtan May 25 '16 at 6:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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