Cleaner way of returning true/false with error message

Question

What's a concise way of returning true or false along with an error message?

Here's a concrete example - I'm building a parser that takes in a file. ParseReplayData does some validation before proceeding with the actual parsing:

public class Parser
{
    private const string SUPPORTED_FILE_EXTENSION = "w3g";
    private const string ERROR_FILE_DOES_NOT_EXIST = "Replay file does not exist: {0}";
    private const string ERROR_FILE_EXTENSION_NOT_SUPPORTED = "File extension is not a w3g replay file: {0}";
    private const string ERROR_FILE_BYTE_LENGTH_TOO_SMALL = "Replay file's byte length is too small: {0}";
    private const int REPLAY_MINIMUM_BYTE_LENGTH = 288;
    private ReplayData _replayData;
    private readonly string _replayFilePath;

    public ReplayData ReplayData
    {
        get { return _replayData; }
    }

    public Parser(string replayFilePath)
    {
        _replayFilePath = replayFilePath;
    }

    public bool ParseReplayData(out string errorReason)
    {
        errorReason = String.Empty;
        if (!File.Exists(_replayFilePath))
        {
            errorReason = String.Format(ERROR_FILE_DOES_NOT_EXIST, _replayFilePath);
            return false;
        }
        if (Path.GetExtension(_replayFilePath) != SUPPORTED_FILE_EXTENSION)
        {
            errorReason = String.Format(ERROR_FILE_EXTENSION_NOT_SUPPORTED, _replayFilePath);
            return false;
        }

        byte[] replayFileBytes = File.ReadAllBytes(_replayFilePath);
        if (replayFileBytes.Length < REPLAY_MINIMUM_BYTE_LENGTH)
        {
            errorReason = String.Format(ERROR_FILE_BYTE_LENGTH_TOO_SMALL, _replayFilePath);
            return false;
        }
        _replayData = new ReplayData();
        ..... //Parse Replay 
        return true;
    }
} 

I can't shake the feeling that there's a lot of code smell in this class, and I'd appreciate any other suggestions you could give me.

Answer 1

An alternative as MrSmith42 suggested could be to use exceptions.

public class ReplayDataParser
{
    private const string SupportedFileExtension = "w3g";
    private const string ErrorFileDoesNotExist = "Replay file does not exist: {0}";
    private const string ErrorFileExtensionNotSupported = "File extension is not a w3g replay file: {0}";
    private const string ErrorFileByteLengthTooSmall = "Replay file's byte length is too small: {0}";
    private const int ReplayMinimumByteLength = 288;            

    private readonly string _replayFilePath;

    public ReplayDataParser(string replayFilePath)
    {
        _replayFilePath = replayFilePath;
    }

    public ReplayData Parse()
    {
        if (FileNotFound())
        {
            throw new FileNotFoundException(string.Format(ErrorFileDoesNotExist, _replayFilePath));
        }

        if (FileExtensionNotSupported())
        {
            throw new ArgumentException(string.Format(ErrorFileExtensionNotSupported, _replayFilePath));
        }

        if (FileNotTooSmall())
        {
            throw new FileTooSmallException(string.Format(ErrorFileByteLengthTooSmall, _replayFilePath));
        }

        return ParseFile();
    }

    private ReplayData ParseFile()
    {
        return new ReplayData();
    }

    private bool FileExtensionNotSupported()
    {
        return Path.GetExtension(_replayFilePath) != SupportedFileExtension;
    }

    private bool FileNotFound()
    {
        return !File.Exists(_replayFilePath);                
    }

    private bool FileNotTooSmall()
    {
        byte[] replayFileBytes = File.ReadAllBytes(_replayFilePath);
        return replayFileBytes.Length < ReplayMinimumByteLength;
    }
}

The downside from this from what I can see is if you use this parser in alot of places then you will need to handle the exceptions accordingly. However the upside is that if you wish to do different things based on the reason why something failed then you can catch individual exceptions as required.

Offered as an alternative for consideration in any event.

Answer 2

To me, whatever needs to Parse sounds like a crying need for a TryParse method/pattern.

Your Parse method should just throw a ParseException if it fails, and the TryParse method would return a Boolean indicating success or failure, along with an out parameter that returns your successfully parsed value, if any.

A good example of this pattern can be found in the BCL, with int.Parse(string):int and int.TryParse(string, out int result):bool.

The TryParse method never throws an exception.

---

Building on @dreza's answer, I would put the custom exceptions as an InnerException, within the outer ParseException.

Answer 3

You can use an Exceptional Monad to cleanup the code:

https://gist.github.com/bradphelan/6154972

public class Parser
{
    private const string SUPPORTED_FILE_EXTENSION = "w3g";
    private const string ERROR_FILE_DOES_NOT_EXIST = "Replay file does not exist: {0}";
    private const string ERROR_FILE_EXTENSION_NOT_SUPPORTED = "File extension is not a w3g replay file: {0}";
    private const string ERROR_FILE_BYTE_LENGTH_TOO_SMALL = "Replay file's byte length is too small: {0}";
    private const int REPLAY_MINIMUM_BYTE_LENGTH = 288;
    private ReplayData _replayData;
    private readonly string _replayFilePath;

    public ReplayData ReplayData
    {
        get { return _replayData; }
    }

    public Parser(string replayFilePath)
    {
        _replayFilePath = replayFilePath;
    }

    public Exceptional<ReplayData> ParseReplayData()
    {
        if (!File.Exists(_replayFilePath))
          return new FileNotFoundException(ERROR_FILE_DOES_NOT_EXIST).ToExceptional<ReplayData>();

        if (Path.GetExtension(_replayFilePath) != SUPPORTED_FILE_EXTENSION)
          return new NotSupportedException(ERROR_FILE_EXTENSION_NOT_SUPPORTED).ToExceptional<ReplayData>();

        byte[] replayFileBytes = File.ReadAllBytes(_replayFilePath);
        if (replayFileBytes.Length < REPLAY_MINIMUM_BYTE_LENGTH)
            return new InvalidOperationException(ERROR_FILE_BYTE_LENGTH_TOO_SMALL).ToExceptional<ReplayData>();



        _replayData = new ReplayData();
        ..... //Parse Replay 
        return new Exceptional(_replayData);
    }
} 

usage:

private void UseParser()
{
var parser = new Parser("path/to/file");

var result = parser.ParseReplayData();

if(result.HasException)
{
 Console.WriteLine(result.Exception.Message);
 return;
 }

 Console.WriteLine(result.Value);
}

Answer 4

Some "error" states may not be critical during data parsing. In these cases, you want to to be alerted of the error and decide whether to continue or stop parsing.

I recommend using events for this purpose.

public class ParseErrorArgs : EventArgs
{
    public string Reason {get; private set;}
    public bool Stop {get;set;}
    public ParseErrorArgs(string reason)
    {
        this.Reason = reason;
    }
}

// in your parser class
public event EventHandler<ParseErrorArgs> Error;

protected virtual void OnError(ParseErrorArgs e)
{
    if (Error != null) Error(this, e);
}

// then when you encountered an error
var args = new ParseErrorArgs("message");
this.OnError(args);
if (args.Stop) break;

Answer 5

returning true/false with an error message?

// I want more expressive and clearer code, "Tuple" is too generic.
public class ErrorInfo : Tuple<bool,string>

public ErrorInfo ParseReplayData() {}

client code:

var parseError= ParseReplayData();
errorObject.item1 ? doSomething() : DisplayErrorMessage(parseError.item2);

And/Or collect error objects for later processing

List<ErrorInfo> errorsCollection;
errorsCollection.Add(parseError);

Exceptions as Error Handling

If you do this make your own Exception class; if only to give it a name that conveys intent DataErrorException, for example.

I suggest the above because using exceptions this way is considered poor coding practice. It perverts the intent of exceptions. Further, exception generation & handling is relatively computationally expensive.