HiveBrain v1.2.0
Get Started
← Back to all entries
debugcsharpMinor

Cleaner way of returning true/false with error message

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
errorwithmessagewaytruefalsereturningcleaner

Problem

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.

Solution

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.

Code Snippets

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;
    }
}

Context

StackExchange Code Review Q#30774, answer score: 7

Revisions (0)

No revisions yet.