Parsing pipe delimited lines

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])) 

Will I get into trouble if I post the same solution to both questions in two places - here and there? :-)

There is no one proper way of doing this but I have a suggestion for you that uses a custom attribute.

I named it ColumnInfoAttribute:

class ColumnInfoAttribute : Attribute { public int Index { get; set; } public int MaxLength { get; set; } } 

It allows you to specify the index and max length of a field so you can apply it all properties that should receive a value:

class LineItem { [ColumnInfo(Index = 0, MaxLength = 1)] public string S0 { get; set; } [ColumnInfo(Index = 1, MaxLength = 6)] public string S1 { get; set; } [ColumnInfo(Index = 2, MaxLength = 34)] public string S2 { get; set; } [ColumnInfo(Index = 3, MaxLength = 34)] public string S3 { get; set; } [ColumnInfo(Index = 4, MaxLength = 34)] public string S4 { get; set; } [ColumnInfo(Index = 5, MaxLength = 34)] public string S5 { get; set; } [ColumnInfo(Index = 6, MaxLength = 34)] public DateTime S6 { get; set; } public static LineItem Parse(string line) { var propertyDictionary = typeof(LineItem) .GetProperties(BindingFlags.Instance | BindingFlags.Public) // create an anonymous object to hold the property and the ColumnInfo .Select(p => new { Property = p, ColumnInfo = p.GetCustomAttribute<ColumnInfoAttribute>() }) // get only those where the ColumnInfo is not null (in case there are other properties) .Where(ci => ci.ColumnInfo != null) // create a dictionary with the Index as a key .ToDictionary(ci => ci.ColumnInfo.Index); var result = new LineItem(); var values = line.Split('|'); for (var i = 0; i < values.Length; i++) { // validate the length of the value var isValidLength = values[i].Length > propertyDictionary[i].ColumnInfo.MaxLength; if (!isValidLength) { // todo: throw some appropriate exception or do other error handling } // set the corresponding property var converterdValue = Convert.ChangeType( values[i], propertyDictionary[i].Property.PropertyType); propertyDictionary[i].Property.SetValue(result, converterdValue); } return result; } } 

The same class has also a Parse method that via reflection gets all properties with the ColumnInfo attribute and creates a dictionary. The key of the dictionary is the Index.

Now you can for-loop over all values and use the i to get a ColumnInfo. You then check if the length of the field is valid and if so you use the property.SetValue to assign a value to the property. You don't have to worry much about the types. The Convert.ChangeType is able to handle quite a few of them.

Usage:

var line = "D|111111|87654321|Bar|BCreace|GBP|24/08/2010"; var lineItem = LineItem.Parse(line); 

It's easily extensible and very generic. If you have more such cases you can put this code in a base class and add the attributes to the derived classes.