10
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ This looks like C# (though you should mention that in the tags). Can you use LINQ? \$\endgroup\$ – sepp2k Jan 30 '11 at 16:24
  • \$\begingroup\$ @sepp2k yes it is c# and i could use linq \$\endgroup\$ – RoflcoptrException Jan 30 '11 at 16:26
13
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Would that work if returning a new list was not an option? \$\endgroup\$ – Sean Lynch Jan 30 '11 at 20:48
  • \$\begingroup\$ @Sean: It would if reassigning lines is an option (i.e. you could reassign lines to the result of Where + ToList). If that is also not an option, then no, I'm afraid it won't work for you. Though I'd have to ask why it isn't an option. I'd suspect that any such restriction would be the result of a design flaw (though of course I would not presume to say for sure without knowing the design). \$\endgroup\$ – sepp2k Jan 30 '11 at 22:51
  • \$\begingroup\$ Your right, it generally would be the result of a design flaw. Unfortunately with third party controls it might not be my design. But assuming that reassigning lines is a possibility it is definitely the best option. \$\endgroup\$ – Sean Lynch Jan 31 '11 at 2:02
1
\$\begingroup\$
  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);
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.