Text parser code elegance

Question

I have a text parser that reads certain information from a given file with a specified format. The text file contains some measured properties of a product. The date model number/lot number is written on the file name.

class SomeTextFileParser
{
    private static int POINT_COUNT = 15;

    string filePath;
    string date;
    string modelName;
    string lotName;
    string fileDate;
    string fileModelName;
    string fileLotName;
    public string errMessage;
    public List<EqusData> dataList = new List<EqusData>();

    private bool ErrorReturn(string message)
    {
        this.errMessage += message + " ";
        return false;
    }

    public SomeTextFileParser(string path)
    {
        this.filePath = path;
        this.errMessage = "";
    }

    public bool Parse()
    {
        try
        {
            string[] lines;

            if (!File.Exists(this.filePath))
                return ErrorReturn("File does not exist.");

            lines = System.IO.File.ReadAllLines(this.filePath, Encoding.GetEncoding(949));

            if (lines.Length < 1)
                return ErrorReturn("File is empty.");

            if (!ParseFilePath())
                return ErrorReturn("");

            if (!ParseHeaderLine(lines[0]))
                return ErrorReturn("");

            if (!modelName.Equals(fileModelName) || !lotName.Equals(fileLotName))
                return ErrorReturn("File name does not correspond with the model/lot number in the content.");

            this.dataList.Clear();

            foreach (string line in lines)
            {
                string[] valueLineParts = line.Split(',');
                if (valueLineParts.Length != POINT_COUNT + 7)
                {
                    continue;
                }

                if (!ParseValueLine(line))
                {
                    this.dataList.Clear();
                    return ErrorReturn("");
                }
            }

            if(this.dataList.Count == 0)
                return ErrorReturn("No input data found.");

            return true;
        }
        catch (Exception e)
        {
            return ErrorReturn("exception:" + e.Message);
        }
    }

    private bool ParseFilePath()
    {
        string[] filePathParts = this.filePath.Split('\\');
        if (filePathParts.Length < 2) return ErrorReturn("Invalid file path.");

        string fileDay = filePathParts[filePathParts.Length - 2].Substring(0, 2);
        string fileName = filePathParts.Last();
        string[] fileNameParts = fileName.Split('_');

        if (fileNameParts.Length != 3) return ErrorReturn("Invalid file name format.");

        this.fileDate = "20" + fileNameParts[0].Substring(0, 4) + fileDay;
        this.fileModelName = fileNameParts[1];
        this.fileLotName = fileNameParts[2].Replace(".txt", "");

        if (this.fileDate.Length != 8 || this.fileModelName.Length < 1 || this.fileLotName.Length < 1) return ErrorReturn("Unable to determine Model/Lot number from file name.");

        this.date = this.fileDate;

        return true;
    }

    private bool ParseHeaderLine(string line)
    {
        int modelNameStartPoint, modelNameEndPoint, lotNameStartPoint;

        modelNameStartPoint = line.IndexOf("Model Name:");
        modelNameEndPoint = line.IndexOf(",");
        lotNameStartPoint = line.IndexOf("LOT Name:");

        if (modelNameStartPoint < 0 || modelNameEndPoint < 0 || lotNameStartPoint < 0) 
            return ErrorReturn("Unable to determine Model/Lot number from file name.");

        modelNameStartPoint += ("Model Name:").Length;
        lotNameStartPoint += ("LOT Name:").Length;

        this.modelName = line.Substring(modelNameStartPoint, modelNameEndPoint - modelNameStartPoint);
        this.lotName = line.Substring(lotNameStartPoint);

        return true;
    }


    private bool ParseValueLine(string line)
    {
        line = line.Replace(" ", "");

        string[] valueLineParts = line.Split(',');
        if (valueLineParts.Length != POINT_COUNT + 7) 
            return ErrorReturn("Invalid point count.");

        EqusData equsData = new EqusData();
        equsData.date = this.date;
        equsData.lotName = this.lotName;
        equsData.modelName = this.modelName;
        equsData.sequence = -1;

        if (!Int32.TryParse(valueLineParts[0], out equsData.sequence))
            return ErrorReturn("Invalid data sequence.");

        // Add value per point
        for(int i = 1; i <= POINT_COUNT; i++)
            equsData.valueList.Add(valueLineParts[i]);

        // Add average value to the list
        equsData.valueList.Add(valueLineParts[POINT_COUNT + 3]);

        equsData.isGood = valueLineParts[POINT_COUNT + 4].Equals("G");
        equsData.usl = valueLineParts[POINT_COUNT + 5];
        equsData.lsl = valueLineParts[POINT_COUNT + 6];

        this.dataList.Add(equsData);

        return true;
    }

Data Class

class EqusData
{
    public string date;
    public string modelName;
    public string lotName;
    public int sequence;
    public bool isGood;
    public string usl;
    public string lsl;
    public List<string> valueList = new List<string>();
}

This class is most likely to be used in the following manner:

SomeTextFileParser fileParser = new SomeTextFileParser("C:\SomeText.txt");
if (!fileParser.Parse())
{
    MessageBox.Show("Error: " + fileParser.errMessage);
    return;
}

//Do something with SomeTextFileParser.dataList here...

I'd like to be critiqued on how readable the above code is, but please let me know if my initial thoughts about the code is more or less right:

1. Shouldn't the errMessage be a public property instead of public variable, although in practice it doesn't make any distinction here?

2. Or even better, shouldn't the parse method follow the TryParse pattern and send the error message as out parameter?

   --> If this is the case, should the inner parse methods (i.e. ParseFilePath) also follow the same TryParse convention even though they are private methods only used in this class?

3. Although the inner parse methods use private variables in the class, shouldn't a data class be made containing those variables? For example, ParseFilePath sets fileDate, fileModelName and fileLotName variables to a value, but I have no way of knowing that from just looking at the Parse method. Would it be more clear if I returned an object containing those information from ParseFilePath, and pass that as a parameter to other methods requiring that information? This on the other hand, seems like an overkill.

I'd really appreciate any thoughts or comments. Be as harsh as you need to be.

Answer 1

Answers to specific questions

Property vs. Field

It can be considered A Matter Of Personal Preference (AMOPP) as much as anything else, but I would declare public data as properties rather than as fields. A property can be considered part of the class's contract; it can be included in an interface. A field is implementation. For example, one could have ErrorMessage as string property backed by a list of strings.

private readonly List<string> _errorMessages = new List<string>();
//...
public string ErrorMessage {
    get { return string.Join(Environment.NewLine, _errorMessages); }
}

Note: AMOPP, but one may as well go with ErrorMessage as ErrMessage.

TryParse()?

Unless you have a strong need to avoid throwing an exception from the method I would simply have Parse return IEnumerable and throw an exception on any sort of problem.

Most of the conditions that you have errors returning are exceptions (file not found, unreadable file name, unreadable data). We should be throwing exceptions.
Note: although the ErrMessage code allows for adding messages onto the existing one, that doesn't seem to happen. A single error message in an exception should suit.

Another possibility is to have a constructed return value, say, ParseResult that would include the output of the parsing and any error message.

public class ParseResult {

    public ParseResult(IEnumerable<EqusData> output) 
        : this(null, output) {}

    public ParseResult(string errorMessage ) 
        : this( errorMessage, null) { }

    private ParseResult(string errorMessage, IEnumerable<EqusData> output ) {
        ErrorMessage = errorMessage;

        Output = output;
    }

    public string ErrorMessage { get; private set; }

    public bool IsSuccess {
        get { return ErrorMessage != null; }
    }

    public IEnumerable<EqusData> Output { get; private set; }
}

As in the answer from @mjolka, I would modify the Parse() function to take in the path as input and return the result. There is no value in passing it into the ctor and then using it in parse. Also, this way, an instance of the parser can be used on different files.

ParseResult Parse(string path) {
    // ...
}

// throws exception
IEnumerable<EqusData> Parse(string path) {
    // ...
}

I wouldn't change it to a static method. Again, AMOPP, but if we make Parse() a static method, there are few of the mocking frameworks (and, AFAIK, none of the free ones) that support mocking static methods and this will hamper unit testing.

ParseFilePath - Class vs. Instance members

I would go with returning an class from ParseFileName and then passing that into a constructor the EqusData. It localizes changes if/when the file name format changes.

public class FileInfo {

    public FileInfo(DateTime date, string lotName, string modelName) {
        Date = date;
        LotName = lotName;
        ModelName = modelName;
    }

    public DateTime Date { get; private set; }
    public string LotName { get; private set; }
    public string ModelName { get; private set; }
}

public class EqusData {

    public EqusData(FileInfo info) {
        Values = new List<string>();
        Date = info.Date;
        LotName = info.LotName;
        ModelName = info.ModelName;
    }

    public DateTime Date { get; private set; }
    public string LotName { get; private set; }
    public string ModelName { get; private set; }
    public int Sequence { get; set; }
    public bool IsGood { get; set; }
    public string Usl { get; set; }
    public string Lsl { get; set; }
    public List<string> Values { get; private set; }

}

Now, if the format of the file name changes (say to add a Batch number) we need to change ParseFileName() and EqusData and its ctor but the rest of the code is untouched.

There's a few things is here that count as AMOPP rather than solid do this items.

• All the members of EqusData are Properties not fields - note: even if fields, the names should be capitalized
• Values, Date, LotName, ModelName are all readonly. They don't need to be settable from outside the class. If they don't need to be, then don't let them be.
• Date is stored as a DateTime not a string. Helps with locales (if needed) and is just generally cleaner. If it is a date, treat it as one, not as a string.

Other Points

In terms of readability there are too many returns.

Exceptions should be used to handle exceptional situations and most of the errors here seem to fall into that category. The top level function (Parse) has too many different pieces at too many different levels. By using exceptions and not having to worry about checking and propagating returns we can simplify the layout a lot and make it more readable.

public IEnumerable<EqusData> Parse(string path) {

    var fileInfo = ParseFilePath(path);

    var lines = ReadLinesFromFile(path);

    CheckHeaderLine(lines[0], fileInfo);

    return ReadValueLines(fileInfo, lines.Skip(1));

}

• Get the file info from the file name (path)
• Read all the lines from the file
• Check the header against the file info
• Read the data and return

Errors may happen at the lower levels but the exception mechanism handles it for us, we do not need to check returns and propagate.

Answer 2

Some notes:

• POINT_COUNT can be a const.

• Avoid magic numbers, e.g. in POINT_COUNT + 7.

• Don't catch Exception. Catch only the exceptions that you know how to handle.

• Don't call File.Exists -- instead, place a try around File.ReadAllLines and catch a FileNotFoundException.

• See if you can make any fields readonly. Immutable objects are, in my experience, much easier to deal with.

Your constructor doesn't really do anything useful. Why make a client construct a new object then call the Parse method?

Consider providing a constructor that returns a fully-initialised object, or throws an exception. If you don't like throwing exceptions in constructors, use a private constructor and provide a static method for creating new instances, returning null (or an object representing the error, or throwing an exception) if object initialisation failed.

For instance,

// Creates a fully-initialised SomeTextFileParser.
private SomeTextFileParser(string filePath, string date, string modelName, ...)
{
}

public static SomeTextFileParser ParseFile(string filePath)
{
    // Try to parse the file.
    // If parsing fails, then:
    return null;

    // or:
    throw new FileNameDoesNotCorrespondToModelLotNumberException();

    // If parsing succeeds, then:
    return new SomeTextFileParser(filePath, date, modelName, ...);
}

Then the client code would look like

try
{
    var parser = SomeTextFileParser.ParseFile(filePath);
    // Do something with parser.DataList.
}
catch (SomeTextFileParserException e)
{
    MessageBox.Show(e.Message);
}