Loops for removing unnecessary lines

Question

I have the following method:

private void removeUnnecessaryLines(List<ScatterViewItem> list)
{
    List<Line> remove = new List<Line>();
    foreach (Line line in lines)
    {
        SourceFile destination = (line.Tag as Call).getCallee();

        foreach (ScatterViewItem svi in list)
        {
            SourceFile test = svi.Tag as SourceFile;
            if (test.Equals(destination))
            {
                remove.Add(line);
            }
        }
    }

    foreach (Line l in remove)
    {
        lines.Remove(l);
        Dependencies.Children.Remove(l);
    }
}

As you can see, there is a lot of iteration and casting. Is there a simple way to improve that?

Answer 1

The most obvious improvement would be to use LINQ's Where method to filter the lines list instead of building up a list of to-be-deleted items and then deleting them. In addition to being easier to read and understand it will have the benefit of its runtime not being quadratic in the length of lines.

You can also use the List<T>.RemoveAll method which also takes a predicate like Where (though the predicate specifies the items to be removed, so it's in a way the opposite of Where), but modifies the list in-place, like your solution did.

Answer 2

1. I would not insist on replacing lines.Remove with lines = lines.Where(...). I believe this replacement is subjective since you will still need to remove the same lines from Dependencies.Children collection. Instead I would modify foreach statement.
2. If you do not checking test == null then instead of test = svi.Tag as SourceFile, I would use test = (SourceFile)svi.Tag. Resharper says the same.
3. Second nested foreach is definitely a Contains statement
4. Rename remove -> linesToRemove since remove is not a noun
5. IEnumerable<> for method parameter, not List<>
6. Better name for list parameter? It doesn't clear what is that parameter used for from method signature
7. Why not UpperCamelCase?

Result:

private static void RemoveUnnecessaryLines(IEnumerable<ScatterViewItem> list)
{
    var linesToRemove = lines
        .Select(line => ((Call)line.Tag).getCallee())
        .Where(destination => list.Any(svi => Equals(svi.Tag, destination)))
        .ToList();

    foreach (Line l in linesToRemove)
    {
        lines.Remove(l);
        Dependencies.Children.Remove(l);
    }
}