debugcsharpMinor
Cleaner way of returning true/false with error message
Viewed 0 times
errorwithmessagewaytruefalsereturningcleaner
Problem
What's a concise way of returning
Here's a concrete example - I'm building a parser that takes in a file.
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.
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.
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.
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.