Console application for providing detailed error messages

Question

I'm writing a console application where I need to provide detailed error messages in case anything goes wrong. As a result of this, I find myself splitting up my code into many pieces and making sure that every step is handled correctly. While this certainly makes it possible to detail what exactly went wrong, it unfortunately also leads some horrific overhead on my code. Take the following code for example:

        // Open the SISS catalog on the server
        Catalog catalog = integrationServices.Catalogs[catalogName];
        if (catalog == null)
        {
            Console.WriteLine("Unable to open the SSIS catalog : " + catalogName 
                + ", it does not exist on the server");
            return (int)ExitCode.Failure;
        }

        // Open the folder in the catalog
        CatalogFolder folder = catalog.Folders[folderName];
        if (folder == null)
        {
            Console.WriteLine("Unable to open the folder : " + folderName
                + ", it does not exist on the server");
            return (int)ExitCode.Failure;
        }

        // Open the project in the folder
        ProjectInfo projectInfo = folder.Projects[projectName];
        if (projectInfo == null)
        {
            Console.WriteLine("Unable to open the project : " + projectName 
                + ", it does not exist on the server");
            return (int)ExitCode.Failure;
        }

        // Check if the package exists
        if (!projectInfo.Packages.Contains(packageName))
        {
            Console.WriteLine("Unable to open the package : " + packageName
                + ", it does not exist on the server");
            return (int)ExitCode.Failure;
        }

Is there any way I can make this more pretty?

Exceptions containing an error message and a utility function that checks a boolean success value and throws if it is false. — Jason C, Jul 31, 2014 at 18:40
Throw an exception, perhaps define the exact error message in the exception class itself. — IanNorton, Aug 7, 2014 at 6:32

Pimgd · Accepted Answer · 2016-02-24 14:17:24Z

12

Make a function that writes an error message (String) to the console, then exits with failure error code. But I'm not sure whether this would mess up your returns and overall control flow.

Other than that, I don't know. Yes, you can golf the string to "Unable to open X : " + x + ", it does not exist on the server", but all you're doing is making the error message harder to find when debugging later.

That said, "pretty" is pretty (Hah) abstract. As it is now, the code is perfectly readable, and it doesn't contain too much duplication. It doesn't contain any performance issues either. What you're seeing here is your code reaching the lowest Kolmogorov Complexity it can.

But let's say, for the heck of it, you were to create some sort of function that takes a catalogName and returns either null or a Catalog object. In case of null, you have to call another function to get the error message. If you did that, you're just relocating the if statements. It also makes the design confusing. Basically, I don't think you can because you have to check and you have to report. In my opinion, you did that the prettiest way possible.

edited Feb 24, 2016 at 14:17

answered Jul 31, 2014 at 7:41

Pimgd

22.3k5 gold badges64 silver badges143 bronze badges

\$\begingroup\$ Probably the best advice. The logic inside the checks can be moved to a dedicated logger or replaced by exceptions but any other 'improvements' would just produce code that looks cleaner while being much less intuitive or introduces unneeded delegation. \$\endgroup\$
– Lilienthal
Jul 31, 2014 at 14:39
1

\$\begingroup\$ Probably the worst advice, saved only by the statement "I'm not sure whether this would mess up your returns ...". Having functions exit the program themselves breaks most chances of code reuse, even within the same program for new features added later - even if it works for a specific program it's not the greatest habit to get in to. You lose the ability to handle an error without exiting. You also lose the ability to handle errors in once place (and thus if you wish to change the handling you must re-analyze your code path and change all appropriate code). Exceptions exist for this. \$\endgroup\$
– Jason C
Jul 31, 2014 at 18:50
\$\begingroup\$ So which is it? I guess my suggestion about improving it was pretty bad, but that's just me trying to come up with a good answer, then concluding there wasn't any. IMO, if it was a programming language with pointers I'd stick all of it in a function, taking a output parameter string as the error message. That'd take care of things. But this is C#... so you can't do that. \$\endgroup\$
– Pimgd
Aug 1, 2014 at 7:01

Add a comment |

ChrisWue · Accepted Answer · 2014-07-31 08:03:21Z

There seems to be a pattern there so let's see if we can encapsulate it into a function:

private T Extract(string itemType, string itemName, Func<T> itemExtractor) where T : class
{
    var res = itemExtractor();
    if (res == null)
    {
         Console.WriteLine("Unable to open the {0}: {1}", itemType, itemName);
         return null;
    }
    return res;
}

and usage:

Catalog catalog = Extract("catalog", catalogName, () => integrationServices.Catalogs[catalogName]);
if (catalog == null) return (int)ExitCode.Failure;
CatalogFolder folder =  Extract("folder", folderName, () => catalog.Folders[folderName]);
if (folder == null) return (int)ExitCode.Failure;
ProjectInfo projectInfo = Extract("project", projectName, () => folder.Projects[projectName]);
if (projectInfo == null) return (int)ExitCode.Failure;
string package = Extract("package", packageName, () => projectInfo.Packages.Contains(packageName) ? packageName : null);
if (package == null) return (int)ExitCode.Failure;

You could also throw an exception in the helper method, which you catch and log. Assuming it's called an ItemNotFoundException the code would look like this:

private T Extract(string itemType, string itemName, Func<T> itemExtractor) where T : class
{
    var res = itemExtractor();
    if (res == null)
    {
         throw new ItemNotFoundException(string.Format("Unable to open the {0}: {1}", itemType, itemName));
    }
    return res;
}

and usage:

try 
{
    Catalog catalog = Extract("catalog", catalogName, () => integrationServices.Catalogs[catalogName]);
    CatalogFolder folder =  Extract("folder", folderName, () => catalog.Folders[folderName]);
    ProjectInfo projectInfo = Extract("project", projectName, () => folder.Projects[projectName]);
    string package = Extract("package", packageName, () => projectInfo.Packages.Contains(packageName) ? packageName : null);
}
catch (ItemNotFoundException ex)
{
    Console.WriteLine(ex.Message);
    return (int)ExitCode.Failure;
}

This is what I meant in part - you've gone from a set of clearly defined and alterable tasks (changing the error message would be easy) to something that requires thought to understand. Making it shorter can be done, but it doesn't get more readable. — Pimgd, Jul 31, 2014 at 8:08
Well changing the error message is easy enough - simply replace the first two parameters of the helper with the error message you want to generate — ChrisWue, Jul 31, 2014 at 8:14
This is a very interesting pattern, though I personally would not use it for this particular task... — Vojtěch Dohnal, Jul 31, 2014 at 9:52
I really dislike the duplication of the if (xxx = null) condition, that's why I am not going to upvote it. But the pattern definitely is interesting ;) — Vogel612, Jul 31, 2014 at 13:57
@Vogel612: If you use the version with the exception then the duplication disappears — ChrisWue, Jul 31, 2014 at 20:24

craftworkgames · Accepted Answer · 2014-08-01 03:44:11Z

The problem I see is the philosophy of using return codes instead of exceptions. In modern languages that support exceptions, returning an error code is almost always going to lead to difficult situations.

Let's take a look at the first if statement. Since they are all nearly the same, the same rules will apply.

    Catalog catalog = integrationServices.Catalogs[catalogName];
    if (catalog == null)
    {
        Console.WriteLine("Unable to open the SSIS catalog : " + catalogName 
            + ", it does not exist on the server");
        return (int)ExitCode.Failure;
    }

Now, the obvious return code here is the ExitCode.Failure line. But in my opinion, that's not the root of the problem.

Let's consider the implementation of integrationServices.Catalogs[catalogName] instead.

I imagine, inside the Catalogs class you have an indexer property that looks something like this:

    public Catalog this[string key] 
    {
        get 
        { 
            Catalog catalog;

            if(_dictionary.TryGetValue(key, out catalog))
                return catalog;

            return null;
        }
    }

The problem with an implementation like this is that returning null doesn't tell you why the catalog was not found. So instead of returning null, you could throw an exception with that information.

    public Catalog this[string catalogName] 
    {
        get 
        { 
            Catalog catalog;

            if(_dictionary.TryGetValue(key, out catalog))
                return catalog;

            throw new CategoryNotFoundException(
                "Unable to open the SSIS catalog : " + catalogName + 
                ", it does not exist on the server");
        }
    }

Also, while we are on the topic I don't think it's a good idea for properties to behave this way (throwing exceptions or returning errors). So I'd consider turning this into a method like so:

    public Catalog GetCatalog(string catalogName) 
    {
        Catalog catalog;

        if(_dictionary.TryGetValue(key, out catalog))
            return catalog;

        throw new CategoryNotFoundException(
            "Unable to open the SSIS catalog : " + catalogName + 
            ", it does not exist on the server");
    }

Now back to your original code, all of the nasty if statements can be removed and replaced with a single try catch block around the whole thing.

try
{
    // Open the SISS catalog on the server
    Catalog catalog = integrationServices.GetCatalog(catalogName);

    // Open the folder in the catalog
    CatalogFolder folder = catalog.GetFolder(folderName);

    // Open the project in the folder
    ProjectInfo projectInfo = folder.GetProject(projectName);

    // Check if the package exists
    var package = projectInfo.GetPackage(packageName);

    return package;  // or do whatever it is you need the package for
}
catch (Exception ex)
{
    Console.WriteLine(ex.Message);
    //return (int)ExitCode.Failure;
    throw;
}

And of course, you can keep following this pattern up the call stack logging errors along the way eventually getting a meaningful error message to the user.

EDIT: I changed the methods to throw CategoryNotFoundException's instead because as @mjolka pointed out, you shouldn't be throwing the Exception class directly according to Microsoft guidelines. However, it is okay to catch the Exception class and log it if you are re-throwing it up the call stack.

Just be aware that the code is only to illustrate a point and I haven't defined everything needed to make it work.

+1 For being the only exception-based answer here to mention throwing through to the caller. — Jason C, Jul 31, 2014 at 18:47
@JasonC Thanks. To be fair, most programming books, tutorials and even universities don't explain this stuff properly. I've been programming for 22 years and only really got this stuff down in the last 5 or so. — craftworkgames, Aug 1, 2014 at 1:06
Downvoted due to recommendation to throw System.Exception. From MSDN: "DO NOT throw System.Exception or System.SystemException." — mjolka, Aug 1, 2014 at 2:26
@mjolka Fair call, when I was writing the code I tried to think of a better exception to throw but couldn't. In a production system I would define my own exception class in this situation but I thought it might make the answer too complicated. — craftworkgames, Aug 1, 2014 at 3:27

user50222 · Accepted Answer · 2014-07-31 09:36:25Z

"Pretty" is subjective, but for me this is the most readable:

private class CatalogException : Exception
{
    public CatalogException(string catalogName) :
        base(String.Format(@"
        Unable to open the SSIS catalog : {0}, it does not exist on the server", catalogName))
    {
    }
}

private class FolderException : Exception
{
    public FolderException(string folderName) :
        base(String.Format(@"
        Unable to open the folder : {0}, it does not exist on the server", folderName))
    {
    }
}

private class ProjectException : Exception
{
    public ProjectException(string projectName) :
        base(String.Format(@"
        Unable to open the project: {0}, it does not exist on the server", projectName))
    {
    }
}

private class PackageException : Exception
{
    public PackageException(string packageName) :
        base(String.Format(@"
        Unable to open the package: {0}, it does not exist on the server", packageName))
    {
    }
}


private int doit()
{
    try
    {
        // Open the SISS catalog on the server
        Catalog catalog = integrationServices.Catalogs[catalogName];
        if (catalog == null)
            throw new CatalogException(catalogName);

        // Open the folder in the catalog
        CatalogFolder folder = catalog.Folders[folderName];
        if (folder == null)
            throw new FolderException(folderName);

        // Open the project in the folder
        ProjectInfo projectInfo = folder.Projects[projectName];
        if (projectInfo == null)
            throw new ProjectException(projectName);

        // Check if the package exists
        if (!projectInfo.Packages.Contains(packageName))
            throw new PackageException(packageName);

        return (int)ExitCode.Sucess;
    }
    catch (Exception e)
    {
        Console.WriteLine(e.Message);
        return (int)ExitCode.Failure;
    }
}

+1 for use of exceptions although I personally believe you have subdivided them unnecessarily here; but the general idea works well. Also it is a bit of an injustice to exceptions to re-catch them then return an error status - while that might be necessary if the OP is trying to stick to the current API, throwing the exceptions out of doit and letting a caller choose to handle them would lead to a clean and powerful approach overall. — Jason C, Jul 31, 2014 at 18:41
It's poor style to catch System.Exception, except for in a top-level exception handler, see MSDN. — mjolka, Aug 1, 2014 at 2:34
@JasonC Maybe I went a little overboard with a separate class for each kind of exception, but I like the clarity and readability of resulting code. As said, it's a matter of taste. And of course, I would never advocate catching exceptions to return an error status, but assumed the idea is to rewrite the existing function without changing its semantics. — user50222, Aug 2, 2014 at 13:40

Chris St John · Accepted Answer · 2014-07-31 11:28:59Z

The style of your function (with a success/failure return code) suggests you're coming from a C/C++ background. I'd recommend that you embrace the throw/catch system for handling errors - either by deriving your own exceptions as suggested by @user50222 or else simply raising a base exception:

try
{
    // Open the SISS catalog on the server
    var catalog = integrationServices.Catalogs[catalogName];
    if (catalog == null)
    {
        throw new System.Exception("Unable to open the SSIS catalog : " + catalogName + ", it does not exist on the server");
    }
    /*...*/
    return (int)ExitCode.Success;
}
catch (System.Exception x)
{
    System.Console.WriteLine(x.Message);
    return (int)ExitCode.Failure;
}

This also handles the case where something else went wrong like integrationServices is null or the database had a problem - because the runtime and libraries will raise their own exceptions.

And try not to worry about efficiency or what happens to the stack when you try/throw/catch/finally - it just works. If you don't particularly care why the error occurred you could go the hardcore approach:

try
{
    // Open the project in the catalog
    var catalog = integrationServices.Catalogs[catalogName];
    var folder = catalog.Folders[folderName];
    var projectInfo = folder.Projects[projectName];

    // Check if the package exists
    if (!projectInfo.Packages.Contains(packageName))
    {
        throw new System.Exception("Unable to open the package : " + packageName
            + ", it does not exist on the server");
    }
    /*...*/
    return (int)ExitCode.Success;
}
catch
{
    return (int)ExitCode.Failure;
}

This terrifies C programmers (me included!) but if the underlying classes are properly implemented they'll get disposed and garbage collected just fine.

+1 Although you lose a lot of the power of exceptions if you catch them then return an error status - while that might be necessary if the OP is trying to stick to the current API, throwing the exceptions out of doit and letting a caller choose to handle them would lead to a clean and powerful approach overall. — Jason C, Jul 31, 2014 at 18:44
Downvoted due to recommendation to throw System.Exception. From MSDN: "DO NOT throw System.Exception or System.SystemException." — mjolka, Aug 1, 2014 at 2:25
Thanks @mjolka - I wasn't aware that it is deprecated. Funny that System.Exception still has a public constructor rather than a protected one. — Chris St John, Nov 20, 2014 at 16:00

Vojtěch Dohnal · Accepted Answer · 2014-07-31 09:19:53Z

I basically agree with Prmgd. Mark that you can use .Contains method of collections. Also I would recommend to replace "string" + x + "string" with String.Format

Maybe you have seen this link already: http://muxtonmumbles.blogspot.cz/2012/08/programmatically-executing-packages-in.html

public int ExecuteSSISPackage()
{
    string whatFailed = null;
    if (integrationServices.Catalogs.Contains(catalogName))
    {
        var catalog = integrationServices.Catalogs[catalogName];
        if (catalog.Folders.Contains(folderName))
        {
            var folder = catalog.Folders[folderName];
            if (folder.Projects.Contains(projectName))
            {
                var project = folder.Projects[projectName];
                if (project.Packages.Contains(packageName))
                {
                    var package = project.Packages[packageName];

                    //... Execute package?

                } else whatFailed = String.Format("package: {0}", packageName);
            } else whatFailed = String.Format("project: {0}", projectName);
        } else whatFailed = String.Format("folder: {0}", folderName);            
    } else whatFailed = String.Format("SSIS catalog: {0}", catalogName);

    if (whatFailed != null)
    {
        Console.WriteLine("Unable to open the {0}, it does not exist on the server", whatFailed);
        return (int)ExitCode.Failure;
    }
    else
        return (int)ExitCode.Success;

Code like this makes adding steps an error-prone maintenance nightmare, especially when adding steps in the middle. And indentation starts to get out of control with added steps. — Jason C, Jul 31, 2014 at 18:45
This particular task is relatively simple - there is given fixed count of nested ifs, so this will not get too bad in this case. You can of course rewrite the whole thing with one single command within one try catch block, but you will not get the desired error message then. — Vojtěch Dohnal, Jul 31, 2014 at 19:16
Arrowhead anti-pattern stackoverflow.com/questions/17804005/… — Kiquenet, Mar 10, 2015 at 10:19

Community · Accepted Answer · 2017-05-23 12:41:02Z

2

In functional programming (e.g. in Haskell or Scala), you would use the Either monad, which can hold either the result of a calculation (as Right subclass) or the error reason (as Left subclass). The advantage is that you do your whole computation as if "nothing could happen", and look only at the very end if there was a problem.

Now nothing stops you from having Either in C#, see e.g. http://siliconcoding.wordpress.com/2012/10/26/either_in_csharp/

Update

See also the answer of Simon Woods to this questions.

edited May 23, 2017 at 12:41

CommunityBot

1

answered Jul 31, 2014 at 10:30

Landei

6,9822 gold badges23 silver badges34 bronze badges

Add a comment |

Ace · Accepted Answer · 2014-08-06 19:55:18Z

0

I am not sure that I could make it "prettier" as being pretty is subjective. As a personal preference, I would use a logging library (such as NLog). A possible option (which @chris-st-john covers) is throwing exceptions. If these failures may be frequent, then I prefer your approach (of not throwing errors and reporting them instead).

answered Aug 6, 2014 at 19:55

Ace

1

Add a comment |

Stack Exchange Network

Console application for providing detailed error messages

8 Answers 8

Your Answer

Not the answer you're looking for? Browse other questions tagged
c#
console
error-handling
or ask your own question.

Hot Network Questions

Console application for providing detailed error messages

8 Answers 8

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c#consoleerror-handling or ask your own question.

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
c#
console
error-handling
or ask your own question.