Return to Answer

• 1

• 75.5k
• 18
• 194
• 467

In the object initializer syntax you're using here:

return new StatementLineResult()
{
 ErrorMessages = errorMessage,
 Data = BankLineData(delimitedRecord),
 IsValid = errorMessage.Count==0
};

The parentheses are redundant; object initializer calls the parameterless constructor by default, so you don't need to call it explicitly.

However there's a design smell here; using this syntax tells me your StatementLineResult class looks something like this:

public class StatementLineResult
{
 public Dictionary<string,string> ErrorMessages { get; set; }
 public SomeType Data { get; set; }
 public bool IsValid { get; set; }
}

Notice how everything could be set from the outside, even with invalid state? What if IsValid is set to false when there are items in ErrorMessages? Nonsensical isn't it!

The type is crying to be immutable. Make the properties get-only, and assign their values from the constructor. If the logic for IsValid is "has no error messages", then it's probably redundant. In any case, it should be get-only and return ErrorMessages.Count == 0, not an arbitrary bool value.

Don't expose a Dictionary (or a List, for that matter) like that. Expose an IEnumerable<T> - your client code needs only to iterate the values, and doesn't care about the Key and Value concepts. What's the type of T then?

How about a tiny little class?

public class FieldValidationError
{
 public FieldValidationError(string fieldName, string message)
 {
 _fieldName = fieldName;
 _message = message;
 }
 private readonly string _fieldName;
 public string FieldName { get { return _fieldName; } }
 private readonly string _message;
 public string Message { get { return _message; } }
}

Then instead of exposing a Dictionary<string,string>, you can expose a much more expressive IEnumerable<FieldValidationError>, and not worry about the possibility that client code could tamper with the results.

Random thoughts:

• Don't compare strings to null or "" - use string.IsNullOrEmpty instead... which you are using.. just not consistently.
• Don't throw System.Exception. Use the most appropriate existing exception type, or make one if none fits the bill appropriately. But don't throw the base exception type; it forces client code to catch (Exception), which makes it catch all kinds of nasties that you would probably rather see bubble up the call stack and crash your app. In this case, throwing an ArgumentException would probably be fine; deriving an InvalidFieldCountException from it could work too.

Spot the issue:

if (!int.TryParse(items[1], out sortCode) || items[1].Length > 6)
 errorMessage.Add("Sort Code", "Invalid sort code");
if (string.IsNullOrEmpty(items[2]) || items[1].Length > 34)
 errorMessage.Add("Account Number", "Invalid account number");
if (string.IsNullOrEmpty(items[3]) || items[1].Length > 35)
 errorMessage.Add("Account Alias", "Invalid account alias");
if (string.IsNullOrEmpty(items[4]) || items[1].Length > 35)
 errorMessage.Add("Account Name", "Invalid account name");
if(string.IsNullOrEmpty(items[5]) || items[5].Length != 3)
 errorMessage.Add("Account Currency", "Invalid account currency");

Looks like a copy-pasta bug. I think the part after the || operator is actually intended to use the same index as the part before.

This is clearly a bug:

if(_transactionTypes.First(item=>item.TransactionType==items[16]) == null)

First does not return null when there's no item: it throws an InvalidOperationException instead. You probably meant to use FirstOrDefault:

if(_transactionTypes.FirstOrDefault(item => item.TransactionType == items[16]) == null)

...but that's sub-optimal, too; if that's meant to say "if there's no _transactionTypes item for items[16]", then it can be expressed more concisely like this:

if(!_transactionTypes.Any(item => item.TransactionType == items[16]))