Is this good C# style?

Question

Consider the following method signature:

public static bool TryGetPolls(out List<Poll> polls, out string errorMessage)

This method performs the following:

• accesses the database to generate a list of Poll objects.
• returns true if it was success and errorMessage will be an empty string
• returns false if it was not successful and errorMessage will contain an exception message.

Is this good style?

Update: Lets say i do use the following method signature:

public static List<Poll> GetPolls()

and in that method, it doesn't catch any exceptions (so i depend the caller to catch exceptions). How do i dispose and close all the objects that is in the scope of that method? As soon as an exception is thrown, the code that closes and disposes objects in the method is no longer reachable.

Answer 1

That method is trying to do three different things:

1. Retrieve and return a list of polls
2. Return a boolean value indicating success
3. Return an error message

That's pretty messy from a design standpoint.

A better approach would be to declare simply:

public static List<Poll> GetPolls()

Then let this method throw an Exception if anything goes wrong.

Answer 2

I believe

public static bool TryGetPolls(out List<Poll> polls)

would be more appropriate. If the method is a TryGet then my initial assumption would be there is reason to expect it to fail, and onus is on the caller to determine what to do next. If they caller is not handling the error, or wants error information, I would expect them to call a corresponding Get method.

Answer 3

As a general rule, I would say no.

The reason I say no is actually not because you're performing a TryGetX and returning a bool with an out parameter. I think it's bad style because you're also returning an error string.

The Try should only ignore one specific, commonly-encountered error. Other problems may still throw an exception with the appropriate exception message. Remember that the goal of a Try method like this is to avoid the overhead of a thrown exception when you expect a particular, single sort of failure to happen more frequently than not.

Instead, what you're looking for is a pair of methods:

public static bool TryGetPolls( out List<Poll> polls );
public static List<Poll> GetPolls();

This way the user can do what's appropriate and GetPolls can be implemented in terms of TryGetPolls. I'm assuming that your staticness makes sense in context.

Answer 4

Consider returning:

• an empty collection
• null

Multiple out parameters, to me, is a code smell. The method should do ONE THING only.

Consider raising and handling error messages with:

throw new Exception("Something bad happened");
//OR
throw new SomethingBadHappenedException();

Answer 5

It depends on what the error message is. For instance, if processing couldn't continue because the database connection wasn't available, etc., then you should throw an exception as other people have mentioned.

However, it may be that you just want to return "meta" information about the attempt, in which case you just need a way to return more than one piece of information from a single method call. In that case, I suggest making a PollResponse class that contains two properties: List < Poll > Polls, and string ErrorMessage. Then have your method return a PollResponse object:

class PollResponse
{
    public List<Poll> Polls { get; }
    public string MetaInformation { get; }
}

Answer 6

Depends on if an error is a common occurance or if it us truly an exception.

If errors are gunuinely rare and bad then you might want to consider having the method just return the list of polls and throw an exception if an error occurs.

If an error is something that is realtively common part of normal operations, as like an error coverting a string to an integer in the int.TryParse method, the method you created would be more appropriate.

I'm guessing the former is probably the best case for you.

Answer 7

This is definitely not an idiomatic way of writing C#, which would also mean that it probably isn't a good style either.

When you have a TryGetPolls method then it means you want the results if the operation succeeds, and if it doesn't then you don't care why it doesn't succeed.

When you have simply a GetPolls method then it means you always want the results, and if it doesn't succeed then you want to know why in the form of an Exception.

Mixing the two is somewhere in between, which will be unusual for most people. So I would say either don't return the error message, or throw an Exception on failure, but don't use this odd hybrid approach.

So your method signatures should probably be either:

IList<Poll> GetPolls();

or

bool TryGetPolls(out IList<Poll> polls);

(Note that I'm returning an IList<Poll> rather than a List<Poll> in either case too, as it's also good practice to program to an abstraction rather than an implementation.)

Answer 8

No, from my point of view this is very bad style. I would write it like this:

public static List<Poll> GetPolls();

If the call fails, throw an exception and put the error message in the exception. That's what exceptions are for and your code will become much cleaner, more readable and easier to maintain.

Answer 9

It depends on how frequently the method will fail. In general, errors in .Net should be communicated with an Exception. The case where that rule doesn't hold is when the error condidition is frequent, and the performance impact of throwing and exception is too high.

For Database type work I think an Exception is best.

Answer 10

I'd restate it like this.

public static List<Poll> GetPolls()
{
    ...
}

It should probably be throwing an exception (the errorMessage) if it fails to retrieve the polls, plus this allows for method chaining which is less cumbersome than dealing with out parameters.

If you run FxCop, you'll want to change List to IList to keep it happy.

Answer 11

Not really - I can see a number of problems with this.

First of all, the method sounds like you'd normally expect it to succeed; errors (cannot connect to database, cannot access the polls table etc) would be rare. In this case, it is much more reasonable to use exceptions to report errors. The Try... pattern is for cases where you often expect the call to "fail" - e.g. when parsing a string to an integer, chances are good that the string is user input that may be invalid, so you need to have a fast way to handle this - hence TryParse. This isn't the case here.

Second, you report errors as a bool value indicating presence or absence of error, and a string message. How would the caller distinguish between various errors then? He certainly can't match on error message text - that is an implementation detail that is subject to change, and can be localized. And there might be a world of difference between something like "Cannot connect to database" (maybe just open the database connection settings dialog in this case and let the user edit it?) and "Connected to database, but it says 'Access Denied'". Your API gives no good way to distinguish between those.

To sum it up: use exceptions rather than bool + out string to report messages. Once you do it, you can just use List<Poll> as a return value, with no need for out argument. And, of course, rename the method to GetPolls, since Try... is reserved for bool+out pattern.

Answer 12

I think its fine. I would prefer though:

enum FailureReasons {}
public static IEnumerable<Poll> TryGetPolls(out FailureReasons reason)

So the error strings don't live in the data-access code...

Answer 13

C# Methods should really only do one thing. You're trying to do three things with that method. I would do as others have suggested and throw an exception if there is an error. Another option would be to create extension methods for your List object.

e.g. in a public static class:

public static List<Poll> Fill( this List<Poll> polls) {
   // code to retrieve polls
}

Then, to call this, you would do something like:

List<Poll> polls = new List<Poll>().Fill();
if(polls != null)
{
  // no errors occur
}

edit: i just made this up. you may or may not need the new operator in List<Poll>().Fill()

Answer 14

Please state your assumptions, constraints, desires/goals, and reasoning; we're having to guess and/or read your mind to know what your intentions are.

assuming that you want your function to

• create the polls list object
• suppress all exceptions
• indicate success with a boolean
• and provide an optional error message on failure

then the above signature is fine (though swallowing all possible exceptions is not a good practice).

As a general coding style, it has some potential problems, as others have mentioned.

Answer 15

The guidelines say to try to avoid ref and out parameters if they are not absolutely required, because they make the API harder to use (no more chaining of methods, the developer has to declare all the variables before calling the method)

Also returning error codes or messages is not a best practice, the best practice is to use exceptions and exception handling for error reporting, else errors become to easy to ignore and there's more work passing the error info around, while at the same time losing valuable information like stacktrace or inner exceptions.

A better way to declare the method is like this.

public static List<Poll> GetPolls() ...

and for error reporting use exception handling

try 
{
   var pols = GetPols();
   ...
} catch (DbException ex) {
   ... // handle exception providing info to the user or logging it.
}

Answer 16

There is also this pattern, as seen in many Win32 functions.

public static bool GetPolls(out List<Poll> polls)


if(!PollStuff.GetPolls(out myPolls))
  string errorMessage = PollStuff.GetLastError();

But IMO it's horrible. I would go for something exception based unless this method has to run 65times per second in a 3d game physics engine or someting.

Answer 17

Did I miss something here? The question asker seems to want to know how to clean up resources if the method fails.

public static IList<Poll> GetPolls()
{
    try
    {
    }
    finally
    {
        // check that the connection happened before exception was thrown
        // dispose if necessary
        // the exception will still be presented to the caller
        // and the program has been set back into a stable state
    }
}

On a design side note, I'd consider pushing this method into a repository class so you have some sort of context with which to understand the method. The entire application, presumably, is not responsible for storing and getting Polls: that should be the responsibility of a data store.