5
\$\begingroup\$

I start external process and after its completion print a message to the user. The message content depends on exit code that this process returns. I have defined this codes in enum:

public enum ExitCode {
    Success = 0,
    Warning = 1,
    Error = 2
}

I call some method only if the process exited with success.

I have 2 approaches. First is to use switch statement

switch (process.ExitCode)
{
    case ExitCode.Success:
        Console.WriteLine("Success message.");
        CallSomeMethod();
        break;
    case ExitCode.Warning:
        Console.WriteLine("Warning message.");
        break;
    case ExitCode.Error:
        Console.WriteLine("Error message.");
        break;
    default:
        Console.WriteLine("Unknown exit code.");
        break;
}

Second way is to create dictionary with (StatusCode, messageContent) pairs

if (dictionary.TryGetValue(process.ExitCode, out msg))
{
    Console.WriteLine(msg);
    if (process.ExitCode == ExitCode.Success)
    {
        CallSomeMethod();
    }
}
else
{
    Console.WriteLine("Unknown exit code.");
}

Which version is more elegant? Feel free to reject both and propose better solution.

\$\endgroup\$
  • 1
    \$\begingroup\$ I would go for switch because it is easier to write. Some people will prefer dictionary, or take this further and propose dictionary of actions... All those approaches are fine and I don't think you should stress over it. Pick what feels natural. \$\endgroup\$ – Nikita B Jun 5 '17 at 12:15
6
\$\begingroup\$

This answer applies only when you want to have well defined messages across whole application.

I would use neither. Let me first explain why and then my proposition.

Critique

Assuming you are going to use your enum elsewhere some duplication might occur. Imagine souch example: you decided to use switch in some method in catch closure and this looks good because it is clean enough. However since you built an entire enum for simply displaying the message to the user you probably would want to use it in other method or class which is very handy.

What do I propose ?

I propose something what I like to call "descriptors", it is somewhat between dictionary and switch. What would you do is create attributes over each enum with it's description :

public enum ExitCode {
    [Description("Success message")]
    Success = 0,
    [Description("Warning message")]
    Warning = 1,
    [Description("Error message")]
    Error = 2
}

and then add an extension method to your application (Source)

public static class EnumHelper
{ 
    public static string GetDesc(this ExitCode enumVal)
    {
        var type = typeof(ExitCode);
        var memInfo = type.GetMember(enumVal.ToString());
        var attributes = memInfo[0].GetCustomAttributes(typeof(DescriptionAttribute), false);
        return ((DescriptionAttribute)attributes[0]).Description;
    }
}

or similar. Hence you have beautiful alternative to your switch:

if(process.ExitCode == ExitCode.Success){
    CallSomeMethod();
}
Console.WriteLine(process.ExitCode.GetDesc());

Code is untested but you might get the idea. Furthermore, if you want to have more freedom with messages you can add second argument to extension method:public static string GetDesc(this ExitCode enumVal, string additionalComment) and use it as such:

Console.WriteLine(process.ExitCode.GetDesc("More comment needed");
\$\endgroup\$
  • \$\begingroup\$ @PratikGaikwad is GetEnumDescription part of .NET framework? \$\endgroup\$ – MaLiN2223 Jun 5 '17 at 18:54
  • \$\begingroup\$ It was an extension method designed earlier. Let me delete my old comment to reduce confusion. \$\endgroup\$ – Pratik Gaikwad Jun 5 '17 at 18:57
  • \$\begingroup\$ I was waiting for an answer like yours and I really appreciate to use power of attributes. \$\endgroup\$ – mankers Jun 6 '17 at 7:06
  • \$\begingroup\$ I don't like this approach - you can always extract the switch statement to a class to avoid repetition which is probably far easier and more straight-forward. \$\endgroup\$ – BKSpurgeon Jun 7 '17 at 13:16
5
\$\begingroup\$

The Problem I have with your switch example is that it isn't immediately apparent that there is extra logic being applied to the case where the exit code is Success and the problem with your dictionary approach is that is a lot of code for a something simple like showing a message.

So I would've used a switch or if() Console.WriteLine(...)

and then an extra

if (process.ExitCode == ExitCode.Success) SomeOtherMethod();

afterwards to make it clear that logic is happening on success.

\$\endgroup\$
  • \$\begingroup\$ Yes, of course you're right. Thanks for the advice. \$\endgroup\$ – mankers Jun 6 '17 at 6:43
1
\$\begingroup\$

The way I see it the dictionary solution only make sense if you intend to make the evaluation dynamic. For a fixed (after compilation) amount of cases I'd stick with a switch statement, you don't need to populate the dictionary in some other part; thus generating additional code that you don't actually need.

\$\endgroup\$
  • \$\begingroup\$ It's definitely right - the dictionary solution is more memory inefficient. \$\endgroup\$ – mankers Jun 6 '17 at 7:05
1
\$\begingroup\$

preface

as @eurotrash points out the method name should be more appropriate. That aside I think there are some good points you can take from this post.

the suggestions:

If you want to elegantly impress your colleagues you could go for OOP overkill and try a polymorphic approach - it was a useful and fun exercise implementing this actually. But in reality, I would almost invariable use the switch statement that you had above because its simple, quick and dirty and you have no reason to complicate it. but for academic purposes you might like the below:

The calling class

public class CallingClass
{
    public void RunExternalProcess()
    {
        // The external proceess finishes and returns an exit code
        ExitCode codeNo = ExternalProcessReturnsCode();

        ExitMessage msg = ExitMessage.Factory(codeNo);
        msg.Response(); // returns the appropriate message
    }

    private ExitCode ExternalProcessReturnsCode()
    {
        Random random = new Random();
        return (ExitCode)random.Next(0, 3);
    }
}

the other classes which do the work

public enum ExitCode
{
    Success = 0,
    Warning = 1,
    Error = 2,
    Unknown = 3
}

public abstract class ExitMessage
{
    public static ExitMessage Factory(ExitCode no)
    {
        switch (no)
        {
            case ExitCode.Success:
                return new Success();
                break;

            case ExitCode.Warning:
                return new Warning();
                break;

            case ExitCode.Error:
                return new Error();
                break;

            default:
                return new Unknown();
                break;
        }
    }

    public abstract void Response();
}

public class Success : ExitMessage
{
    public void Response()
    {
        Console.WriteLine("\n Success message!");
    }
}

public class Warning : ExitMessage
{
    public void Response()
    {
        Console.WriteLine("\n Warning message!");
    }
}

public class Error : ExitMessage
{
    public void Response()
    {
        Console.WriteLine("\n Error message!");
    }
}

public class Unknown : ExitMessage
{
    public void Response()
    {
        Console.WriteLine("\n Unknown exit code!");
    }
}

payrise here we come!

tell your boss you know how to do polymorphism and a strategy pattern. demand a payrise.

I hope this helps :P

\$\endgroup\$
  • \$\begingroup\$ An exit message that prints itself to the console, uses a non-command method name to do so, and starts itself with a newline? Payrise postponed :P \$\endgroup\$ – 404 Jun 7 '17 at 14:25
  • \$\begingroup\$ hahah chrs @eurotrash but what do you mean by non-command method name? \$\endgroup\$ – BKSpurgeon Jun 7 '17 at 21:45
  • \$\begingroup\$ The name Response is a noun which befits a property, not a method. Methods should generally be (and named as) queries (e.g. IsValid) or commands (e.g. PrintResponse) \$\endgroup\$ – 404 Jun 8 '17 at 9:11
  • \$\begingroup\$ @eurotrash chrs mate have added a preface and will keep it in mind for the next code i write \$\endgroup\$ – BKSpurgeon Jun 8 '17 at 9:19
  • \$\begingroup\$ Strategy pattern? There is none. You still have a hardcoded switch. Polymorphism, maybe a little but the usage is wrong. The factory doesn't need the switch. It can be generic and create an exit-message from T. \$\endgroup\$ – t3chb0t Jun 8 '17 at 11:01

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.