Skip to main content
added 1103 characters in body
Source Link
RubberDuck
  • 30.9k
  • 6
  • 71
  • 174
  • 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 ReadUntilCellErrorReadUntilEmpty is set at that point; only ReadUntilEmptyReadUntilCellError 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 ReadUntilErrorReadUntilEmpty flag, so you could probably just drop the enumEnum and change ReadUntilEmptyReadUntilError 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.

  • 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 ReadUntilCellError is set at that point; only ReadUntilEmpty 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 ReadUntilError flag, so you could probably just drop the enum and change ReadUntilEmpty to a Boolean with a default value of false.

  • 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.

edited body
Source Link
RubberDuck
  • 30.9k
  • 6
  • 71
  • 174
  • 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 ReadUntilCellError is set at that point; only ReadUntilEmpty 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 ReadUntilError flag, so you could probably just drop the enemenum and change ReadUntilEmpty to a Boolean with a default value of false.

  • 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 ReadUntilCellError is set at that point; only ReadUntilEmpty 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 ReadUntilError flag, so you could probably just drop the enem and change ReadUntilEmpty to a Boolean with a default value of false.

  • 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 ReadUntilCellError is set at that point; only ReadUntilEmpty 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 ReadUntilError flag, so you could probably just drop the enum and change ReadUntilEmpty to a Boolean with a default value of false.

added 1086 characters in body
Source Link
RubberDuck
  • 30.9k
  • 6
  • 71
  • 174
  • 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.

    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 ReadUntilCellError is set at that point; only ReadUntilEmpty matters. This eliminates an entire block from the If statement.

    Instead of checking for && in your first statement, use || (or instead of and). You don't really care if ReadUntilCellError is set at that point; only ReadUntilEmpty 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.

    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 ReadUntilError flag, so you could probably just drop the enem and change ReadUntilEmpty to a Boolean with a default value of false.

  • 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 ReadUntilCellError is set at that point; only ReadUntilEmpty 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.
  • 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 ReadUntilCellError is set at that point; only ReadUntilEmpty 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 ReadUntilError flag, so you could probably just drop the enem and change ReadUntilEmpty to a Boolean with a default value of false.

Source Link
RubberDuck
  • 30.9k
  • 6
  • 71
  • 174
Loading