Read table row with multiple options

Question

I developing an algorithm to read a row from a table (specifically, I'm reading from an Excel file).

I have three conditions about when to terminate the Read operation:

• When an empty cell is found (recognized by an ElementNotFoundException
• When a cell with errors is found (which corresponds to a null value)
• Both the two previous conditions

This is the current algorithm:

public IEnumerable<string> ReadRow(string spreadsheet, int column, int row, ReadOptions readOptions = ReadOptions.ReadUntilEmpty)
{
    var values = new List<string>();

    if (readOptions.HasFlag(ReadOptions.ReadUntilEmpty) && readOptions.HasFlag(ReadOptions.ReadUntilCellError))
    {
        try
        {
            for (var currentColumn = column; ; currentColumn++)
            {
                var value = ReadCell(spreadsheet, currentColumn, row);

                // Cell has errors, so we exit from the loop
                if (value == null) break;

                values.Add(value);
            }
        }
        catch (ElementNotFoundException)
        {
            // The row is terminated
            // so we exit the loop withouth loggin any error
        }
    }
    else if (readOptions.HasFlag(ReadOptions.ReadUntilCellError))
    {
        for (var currentColumn = column; ; currentColumn++)
        {
            try
            {
                var value = ReadCell(spreadsheet, currentColumn, row);

                // Cell has errors, so we exit from the loop
                if (value == null) break;

                values.Add(value);
            }
            catch (ElementNotFoundException)
            {
                // The ReadOptions is not setted on ReadUntilEmpty
                // so we just ignore empty cells
            }
        }
    }
    else if (readOptions.HasFlag(ReadOptions.ReadUntilEmpty))
    {
        try
        {
            for (var currentColumn = column; ; currentColumn++)
            {
                var value = ReadCell(spreadsheet, currentColumn, row);

                values.Add(value);
            }
        }
        catch (ElementNotFoundException)
        {
            // The row is terminated
            // so we exit the loop withouth loggin any error
        }
    }

    return values;
}

And just for reference, the enumeration used:

[Flags]
public enum ReadOptions
{ 
    ReadUntilEmpty,
    ReadUntilCellError
}

How can I improve it?

Also, any idea on how I can reuse this code to read a column instead a row?

Answer 1

• You always terminate the reading of the row if the element isn't found. This means you can have just one Try-Catch block that wraps your If statement.
• Instead of checking for && in your first statement, use || (or instead of and). You don't really care if ReadUntilEmpty is set at that point; only ReadUntilCellError matters. This eliminates an entire block from the If statement.

• You could go one step further and only check the flag just before, or with, the null value check.

  public IEnumerable<string> ReadRow(string spreadsheet, int column, int row, ReadOptions readOptions = ReadOptions.ReadUntilEmpty)
  {
      var values = new List<string>();
  
      try
      {
          for (var currentColumn = column; ; currentColumn++)
          {
              var value = ReadCell(spreadsheet, currentColumn, row);
  
              if readOptions.HasFlag(ReadOptions.ReadUntilCellError))
              {
                  // Cell has errors, so we exit from the loop
                  if (value == null) break;
              }
  
              values.Add(value);
          }
      }
      catch (ElementNotFoundException)
      {
          // The row is terminated
          // so we exit the loop withouth loggin any 
      }
  
      return values;
  }
  

Ultimately I realized there's never actually a need to check the ReadUntilEmpty flag, so you could probably just drop the Enum and change ReadUntilError to a Boolean with a default value of false.

Abstracting this to work on columns instead of rows is easy. The logic doesn't change at all. You just call it with the row number passed into the column argument and the column number passed into the row argument. So, really, naming the hard part and I'm drawing a blank on any actually useful names, but for the sake of giving an example...

public IEnumerable<string> ReadRow(string spreadsheet, int iteratorIndex, int secondaryIndex, ReadOptions readOptions = ReadOptions.ReadUntilEmpty)
{
    var values = new List<string>();

    try
    {
        for (var index = iteratorIndex; ; index++)
        {
            var value = ReadCell(spreadsheet, index, secondaryIndex);

            if readOptions.HasFlag(ReadOptions.ReadUntilCellError))
            {
                // Cell has errors, so we exit from the loop
                if (value == null) break;
            }

            values.Add(value);
        }
    }
    catch (ElementNotFoundException)
    {
        // The row is terminated
        // so we exit the loop withouth loggin any 
    }

    return values;
}

One final note: I'm not entirely comfortable with the fact that the catch block doesn't actually contain any code. It feels a bit hacky.

Answer 2

As @ckuhn203 hinted at the end of his answer, the empty catch block smells.

The problem is that you're catching an exception that should be avoidable: reading an empty cell isn't something that should be exceptional in Excel after all.

I think if ReadCell returned a struct that represented a cell's value, you wouldn't need to catch that exception.

Something like this:

public struct CellValueInfo
{
    private readonly bool _isEmpty;
    public bool IsEmpty { get { return _isEmpty; } }

    private readonly bool _isError;
    public bool IsError { get { return _isError; } }

    private readonly object _value;
    public object Value { get { return _value; } }

    public CellValueInfo(object value, bool isEmpty, bool isError)
    {
        _value = value;
        _isEmpty = isEmpty;
        _isError = isError;
    }
}

Granted, this could be greatly improved, but it's a starting point: an immutable structure that contains the information you need - the idea is to store the information that lets you determine whether a cell has a valid value, an error, or is empty.

Don't rely on exceptions when you can avoid them, especially with Excel interop.

Answer 3

This is the current version, developed based also on @ckuhn203 answer:

[Flags]
public enum ReadOptions
{
    ReadUntilEmpty = 1,
    ReadUntilCellError = 2
}

var values = new List<string>(); 
for (var currentColumn = column; currentColumn < CellLocation.MaxColumnIndex; currentColumn++)
{
    string value;

    if (!TryReadCell(spreadsheet, currentColumn, row, out value))
    {
        // An empty cell has been found
        // so we exit the loop if requested by the options
        if (readOptions.HasFlag(ReadOptions.ReadUntilEmpty)) break;
    }

    // Cell has errors, so we exit from the loop if requested by the options
    if (value == null && readOptions.HasFlag(ReadOptions.ReadUntilCellError)) break;

    values.Add(value);
}
return values;

Basically, I have:

• Fixed the enumeration (the values for the various flags were missing)
• Replaced the ReadCell method with a TryReadCell, that returns a boolean value instead throw up a new exception (improving also the performance in this way)
• Moved the if statements inside the loop