What is best practice for returning value or error message from method in c#?

Question

I am trying to find the cleanest solution for returning value or error message from function / method in c#.

For now I have tried:

public float ValidateValue (float value)
{
    if (value == VALID_VALUE)
    {
        return value;
    }
    else 
    {
        throw new ArgumentException("Invalid value", "value"); 
    }
}

This solution seems to be good enough but in Clean Code Cheap Sheet I have found:

Using Exceptions for Control Flow – Don't do this

Using exceptions for control flow: has bad performance, is hard to understand and results in very hard handling of real exceptional cases.

Answer 1

What will you do in the case of invalid input?

If you are writing code at the UI level that is taking the input from the user then it makes most sense to do something like:

private bool IsValid(float value)
{
  return value == VALID_VALUE; // replace with real check.
}

Then in the calling code you would have:

public void ReactToInput()
{
  float value = HoweverYouGetTheFloatFromTheUser();
  if(!IsValid)
  {
    //Code to display error message.
  }
  else
  {
    //Code to do something useful.
    //
    //Code to display result.
  }
}

Because your job at this level is "take what the user gave me, return what they want as best I can" and at this level its best to have the possibility of the user doing something incorrect front and centre.

If you are writing code for other code to make use of, then it makes most sense to do something like:

private void CheckValid(float valid)
{
  if(value != VALID_VALUE) // replace with real check.
    throw new ArgumentException();
}

Then in the calling code you would have:

public float DoWork(float value)
{
  CheckValid(value)
  //Code to do something useful.
  //
  //Code to return result.
}

Here your job is to do what the method's task is cleanly and return a meaninful result (or void if there isn't one). If you can't do that job, because the input you were given is nonsense (or for any other reason) then you need to stop as soon as you can and deal with that problem. You could do this by returning an error/success code every time and having calling code checking it every time, but while this approach does indeed have some advantages, exceptions let us:

1. Write with a focus on the correct behaviour.
2. Pass up exceptions.

For an example of 1, compare:

private bool WithExceptions()
{
  return A() > B() && C() > D();
}

private bool WithExplicitChecks(out bool result)
{
  result = false;
  int a;
  int b;
  if(!A(out a))
    return false;
  if(!B(out b))
    return false;
  if(a <= b)
    return true;
  int c;
  int d;
  if(!C(out c))
    return false;
  if(!D(out d))
    return false;
  result = c > d;
  return true;
}

For an example of 2, consider:

private void A()
{
  if(_someField == null)
    throw new InvalidOperationException("field not ready");
  _someField.DoSomething();
}
private void B()
{
  A();
}
private void C()
{
  B();
}
private string D()
{
  try
  {
    C();
  }
  catch(InvalidOperationException)
  {
    Console.Error.WriteLine("Was not ready");
  }
}

Obviously a real case would have B() and C() do more, but we can see here that only A() has to worry about raising exceptions and only D() about dealing with them, B() and C() can both just concentrate on the main concern.*

The two approaches can be mixed. Consider:

private static string CheckValid(string path)
{
  if(path.Length == 0)
    return "You cannot enter an empty file path";
  switch(path[path.Length - 1])
  {
    case '\\':
    case '/':
      return "You cannot enter a directory path";
  }
  return null;
}
public static void Main(string[] args)
{
  Console.WriteLine("Enter a file path");
  var path = Console.ReadLine().Trim();
  var validationError = CheckValid(path);
  if(validationError != null)
    Console.Error.WriteLine(validationError);
  else
  {
    try
    {
      using(var reader = new StreamReader(path))
        Console.WriteLine(reader.ReadToEnd());
    }
    catch(FileNotFoundException)
    {
      Console.Error.WriteLine("File not found");
    }
    catch(UnauthorizedAccessException)
    {
      Console.Error.WriteLine("Access denied");
    }
    catch(IOException ioe)
    {
      Console.Error.WriteLine(string.Format("I/O Exception: {0}", ioe.Message));
    }
  }
  Console.Read();
}

This simple program takes a file path from the user, and opens the relevant file and outputs the contents as text. It takes both approaches to error-handling.

Because we can easily check for invalid input that is empty, or which ends with / or \, that is done with simple control-flow and we present an error message instead of doing something.

Other issues we can only know about by trying to open the file and failing, so in those cases we handle the exceptions. I combine both explicit checks for two types of problem along with one for a general class of problems, and act accordingly.

There is a third type of exception handling here; if an exception happens that I don't expect at all, the program fails with a exception message being dumped for debugging purposes. This is the case anywhere you don't catch all exceptions, but a very useful one; because I don't have a blanket catch or catch(Exception) I don't confuse exceptions I'm expecting to deal with (go me for handling them!) with exceptions that are there because I made a mistake in not realising they could happen (boo me! now I have to fix it).

This is a simple program that takes a file path from the user, and outputs the contents of the file. Note that it combines both approaches:

*Do though always consider that something started in a method may not be finished if an exception busts through it.

Answer 2

If you want to validate some input value, I would expect a bool to be returned indicating 'valid' or 'invalid', or no return value and an exception thrown when the value is invalid.

So I would suggest to use this:

public bool ValidateValue(float value)
{
    return value == VALID_VALUE;
}

Or this:

public void ValidateValue(float value)
{
    if (value != VALID_VALUE)
    {
        throw new ArgumentException("Invalid value", "value"); 
    }
}

So throwing an exception is not a problem, especially when there are multiple reasons to reject, and you want to distinguish the various reasons. Otherwise, just use a bool, like int.TryParse does for example.