Should a connect method return a value?

Question

I was looking at some code I've inherited and I couldn't decided if I like a bit of code.

Basically, there is a method that looks like the following:

bool Connect(connection parameters){...}

It returns true if it connects successfully, false otherwise.

I've written code like that in the past, but now, when I see this method I don't like it for a number of reasons.

1. Its easy to write code that just ignores the returned value, or not realize it returns a value.

2. There is no way to return an error message.

3. Checking the return of the method doesn't really look nice:

   if (!Connect(...)){....}

I could rewrite code to throw an exception when it doesn't successfully connect, but I don't consider that an exceptional situation. Instead I'm thinking of refactoring the code as follows:

void Connect(Connection Parameters, out bool successful, out string errorMessage){...}

I like that other developers have to provide the success and error strings so they know the method has error conditions and I can know return a message

Anyone have any thoughts on the matter?

Thanks -Matt

Answer 1

I would opt for the exception versus the out parameters. If you want consumers of your class to care, make them care with the Exception, or else leave them alone. By using the out parameters, you're simply making their lives more inconvenient if they don't have to care by making them use throwaway variables. Also consider that if your function is already in the wild, you're introducing a breaking change if you change the signature (instead of providing an additional overload).

Programmers expect exceptions in error cases, particularly in languages like C#. We've been trained.

Answer 2

I see your points but to add my 2c.

I generally am not a fan of output parameters. If you need to return multiple values from a function re-evaluate what the function is doing, in most case multiple return values is a sign that a method does too much. In your case it is legitimate a complex type would be returned from your connect method (beyond a simple boolean).

I would be in favor of returning a custom type from the connect method that stores all relevant information rather than multiple output parameters. Think about future extensibility, what if you need to include more information down the road. Adding additional output parameters is a breaking change

Also, I tend to disagree with forcing a user to provide memory allocations for all state data. There can be times when I don't care about the success message (maybe I only care if it errors) in which case having to pass both output params is a pain.

I do agree though that checking the return value of the initial method posted isn't ideal.

Answer 3

I have only an opinion, so take it for what its worth.

A method named like this, "Connect", is an order. It's like giving it to a soldier, "Jump" or "Shoot". You don't expect the soldier to report back unless he's unable to complete the order, and that would be a rare happening.

As such, I have the tendency to not have a return value for such methods, but if there is a chance that under regular usage of the method, there will be failures, then I build a second method, named TryXYZ, returning a bool, and if necessary providing me with the results of whatever XYZ is as out parameters.

This follows the standard set forth by the Parse methods of various numeric types in the .NET BCL.

So in your case, I would probably have:

void Connect(connection parameters);
bool TryConnect(connection parameters, out status);

The nice thing is that if you build the TryConnect method properly, Connect becomes really easy.

Example:

public bool TryConnect(string connectionString, out ConnectionStatus status)
{
    ... try to connect
    ... set status, and return true/false
}

public void Connect(string connectionString)
{
    ConnectionStatus status;
    if (!TryConnect(connectionString, out status))
        switch (status)
        {
            case ConnectionStatus.HostNotFound:
                throw new HostNameNotFoundException();
            ...
        }
}

I don't expect my orders to not complete, but in the sense they might, I want to be explicit about it.

Answer 4

Hey Matt,

Not sure if this is any better than your refactoring code, but hopefully this could give you another idea.

For my projects, I create a method that returns the last error message.

string Error = '';
bool Connect(connection parameters)
{
// do my connection here
// if there's an error set string variable Error
// This way you can set different error message depending on what you're doing
}

string lastErrorMessage(){
// return Error string
}

This way you can do this:

if(!connect(...))
{
string message = lastErrorMessage();
// Then do what you need here.
}

This might not be the best way, but should help you :)

Answer 5

I don't mind the Connect function returning a boolean and I'm not a big fan of output parameters. If the function did not return the connection state you'd probably have to write a IsConnected function/property (depending on your style) anyway to allow someone to check it, so it saves a step.

As far as expections go, let the exception be caught by the calling code, that'll force the caller to care. :)

Answer 6

I agree that failing to establish a connection (in most cases) should not be seen as an exceptional situation. However to force the user to provide arguments for error string etc. isn't nice either. Other possible solutions:

1. Use logging. Drawback is that messages are written to the log but not (without much effort) available to the caller.
2. Use boolean return and provide methods for querying the last error (like errno in C). IMHO not nice either.
3. Refactor your code, return an object of a connection class. Provide methods to query connection state.
4. return an instance of a class that gathers all relevant information, aka isSuccessfull(), getErrorString, etc.