Parsing pipe delimited lines

Question

I am parsing the following line of code via a specific format. Line is:

S|111111|87654321|Bar UK|BCreace UK|GBP|24/08/2010|

The Format is:

Index    Field Length    
S0 -     1               
S1 -     6
S2 -     34
....
...
S6 -     10

I am validating using many if statements. Could anyone suggest a better approach?

private static StatementLineResult Validate(string delimitedRecord)
{
    if (delimitedRecord == null)
        throw new ArgumentNullException("delimitedRecord");

    var items = delimitedRecord.Split('|');
    if(items.Length != 19)
        throw new Exception("Improper line format");

    var errorMessage = new Dictionary<string, string>();

    var bankIdentifierCodes = new List<string> {"ABCDGB2L", "EFGHGB2L"};

    if (items[0].Length != 1 || items[0] != "S")
        errorMessage.Add("Record Identifier","Invalid Record Identifier");

    var sortCode = 0;
    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");

    if (!string.IsNullOrEmpty(items[6]) && items[6].Length > 20)
        errorMessage.Add("Account Type", "Invalid account type");

    if(string.IsNullOrEmpty(items[7]) || items[7].Length != 8 || !bankIdentifierCodes.Contains(items[7],StringComparer.OrdinalIgnoreCase))
        errorMessage.Add("Bank Identifier Code", "Invalid bank identifier code");

    if (!string.IsNullOrEmpty(items[8]) && items[8].Length > 35)
        errorMessage.Add("Bank Name", "Invalid bank name");

    if (!string.IsNullOrEmpty(items[9]) && items[9].Length > 27)
        errorMessage.Add("Bank Branch Name", "Invalid bank branch name");

    DateTime transactionDate;
    if (!DateTime.TryParse(items[10], out transactionDate))
        errorMessage.Add("Transaction Date", "Invalid date");

    if (!string.IsNullOrEmpty(items[11]) && items[11].Length > 25)
        errorMessage.Add("Narrative Line 1", "Invalid narrative line 1");
    if (!string.IsNullOrEmpty(items[12]) && items[12].Length > 25)
        errorMessage.Add("Narrative Line 2", "Invalid narrative line 2");
    if (!string.IsNullOrEmpty(items[13]) && items[13].Length > 25)
        errorMessage.Add("Narrative Line 13", "Invalid narrative line 13");
    if (!string.IsNullOrEmpty(items[14]) && items[14].Length > 25)
        errorMessage.Add("Narrative Line 14", "Invalid narrative line 14");
    if (!string.IsNullOrEmpty(items[15]) && items[15].Length > 25)
        errorMessage.Add("Narrative Line 15", "Invalid narrative line 15");

    if(_transactionTypes.First(item=>item.TransactionType==items[16]) == null)
        errorMessage.Add("Transaction Type", "Invalid transaction type");

    decimal debitValue;
    if(items[17] != "" && !Decimal.TryParse(items[17], out debitValue))
        errorMessage.Add("Debit Value", "Invalid debit amount");

    decimal creditValue;
    if (items[18] != "" && !Decimal.TryParse(items[18], out creditValue))
        errorMessage.Add("Credit Value", "Invalid credit amount");

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

Answer 1

You are using the following if quite a lot:

if (string.IsNullOrEmpty(items[index]) || items[index].Length > value)
    errorMessage.Add("Field Name", "Invalid field value");

now, there is not much you can do about this, but I do notice that you are making some mistakes in copy-pasting the index values, which could be avoided by making the conditional statement into a separate method:

private boolean FieldHasCorrectLength(string field, int length){
    return string.IsNullOrEmpty(field) || field.Length > value;
}

And then call it like this:

if (FieldHasCorrectLength(items[index], value))
    errorMessage.Add("Field Name", "Invalid field value");

You can then combine this with the recommendation from t3chb0t to add an Attribute annotation to the class you're parsing the line to. Another advantage of this is that it's easier to add a secondary validation check, because adding extra validations doesn't mean struggling with whether you need to use || or &&, which can get confusing in your case because you sometimes use && and sometime || for what is essentially the same comparison.

Answer 2

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

Answer 3

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.