Cleanest way to skip record in foreach based on condition

Question

I have a nested foreach loop, and I would like to know which is the best way to skip a record based on an if condition in c#.

Below is my solution, if there are any improvements or suggestions please do let me know.

foreach (var ospMap in sourceSpecificMaps)
{
    foreach (var idMapSensorId in ospMap.SensorIds)
    {
        try
        {
            if (string.IsNullOrEmpty(idMapSensorId.SourceId))
            {
                throw new Exception($"SourceId couldn't be found in the { idMapSensorId.SensorId } sensor. The sensor is being skiped.");
            }
            _ospIdMapDictionary[GenCacheId(sourceId, idMapSensorId.SensorId)] = ospMap;
       }
       catch (Exception)
       {
            // We continue through the loop
            continue;
       }  
    }
}

Answer 1

Using exceptions like this is both slow (exceptions are very slow) and terrible practice. Just use the continue if you want to skip.

foreach (var ospMap in sourceSpecificMaps)
{
  foreach (var idMapSensorId in ospMap.SensorIds)
  {
    if (string.IsNullOrEmpty(idMapSensorId.SourceId))
    {
      continue; // TODO: Log the follwoing ?  SourceId couldn't be found in the { idMapSensorId.SensorId } sensor. The sensor is being skiped
    }
    _ospIdMapDictionary[GenCacheId(sourceId, idMapSensorId.SensorId)] = ospMap;
  }

}

Answer 2

You are using exceptions to control logic flow, which is generally a bad idea. Unless you are actually going to do something with that exception, get rid of it and just put the continue inside that if statement.

Answer 3

I think you can remove your try catch if you don't need it for a logical process,

Then you will have code like this :

foreach (var ospMap in sourceSpecificMaps)
{
    foreach (var idMapSensorId in ospMap.SensorIds)
    {

            if (string.IsNullOrEmpty(idMapSensorId.SourceId))
            {
                 continue; // SourceId couldn't be found in the { idMapSensorId.SensorId } sensor. The sensor is being skiped
            }
            _ospIdMapDictionary[GenCacheId(sourceId, idMapSensorId.SensorId)] = ospMap;

    }
}

Answer 4

with linq you could do something like this:

var list = outerList.SelectMany(x => x.TheInnerList).Where(n => !string.IsNullOrEmpty(n.Id));

I think to iterate over those elements based on a initial condition is a cleanest way to do the job

Answer 5

What you want is something like this:

foreach (var ospMap in sourceSpecificMaps)
{
    foreach (var idMapSensorId in ospMap.SensorIds)
    {
        if (string.IsNullOrEmpty(idMapSensorId.SourceId))
        {
            // SourceId couldn't be found in the sensor. The sensor is being skipped.
            continue;
        }
        _ospIdMapDictionary[GenCacheId(sourceId, idMapSensorId.SensorId)] = ospMap; 
    }
}

As everyone above has mentioned, unless you are throwing an exception and the only way to properly handle the error condition caused by the exception is to catch is somewhere outside the loop, don't use exceptions for control flow. They are, compared to a simple conditional test, extremely slow and resource intensive. Especially in a loop, if you get a ton of empty sourceIds this approach could seriously affect your apps performance.

And in your example, again as others have said, you are not actually 'handling' the exception. You are just ignoring it and skipping the remaining body of the loop after the 'if' statement. The exact same behavior results from the code above.