10
\$\begingroup\$

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.

\$\endgroup\$
1
  • 7
    \$\begingroup\$ What about throwing an exception instead of returning false? You could also define a Result class which contains the return-value and the reason. \$\endgroup\$ – MrSmith42 Sep 4 '13 at 14:12
7
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ You could simplify the exception handling by declaring your own exceptions: public class ReplyParseException : Exception and sub class ReplyParseException appropriately. That way you can catch ReplyParseException or their sub classes to handle specific error conditions if the built in exceptions aren't up to the task. \$\endgroup\$ – Greg Burghardt Dec 18 '14 at 13:43
  • \$\begingroup\$ @GregBurghardt good point. I guess that is where I was heading at with the FileTooSmallException. I don't recall there being an exception class of that name already in .net?? \$\endgroup\$ – dreza Dec 18 '14 at 18:38
5
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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);
}
\$\endgroup\$
2
  • \$\begingroup\$ You do realize that the built-in exceptions are basically the same as the exception monad, right? Except they are built-in, so they have better syntax. \$\endgroup\$ – svick Sep 9 '13 at 14:01
  • 3
    \$\begingroup\$ I think the exception monad is not the same as the build in exceptions. I think a monad is a more composite approach, cause it can either has an error or an result. When you deal with mass operations, it can be handy to select out the error ones, without loosing the generic result type. I think exceptions are only classes, but they are throwable. The tuple monad is basically the same thing as the exception monad, but it doesn't have special semantics. \$\endgroup\$ – Manuel Grundner Sep 10 '13 at 11:54
2
\$\begingroup\$

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;
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Please don't do this. This is a class with two properties, implement them with proper naming and forget about inherit from Tuple. \$\endgroup\$ – Thomas Eyde Dec 6 '13 at 20:22
  • \$\begingroup\$ @ThomasEyde, yeah,ok. I agree. Whatever possessed me to commit blatant over-design and obfuscation :( ? \$\endgroup\$ – radarbob Dec 7 '13 at 19:03

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.