0
\$\begingroup\$

I'm working on a compiler. During the final stage I will get a collection of Lines.

This is the method that takes the Lines and gives name to Labels that don't have it yet.

Lines can have Labels. When a Line doesn't have a Label, its Label property is set to null. Apart from this, Labels can be unnamed (Label.Name is null)

This is the method. It looks ugly and I'm using a nested method. Please, review it and try to improve it to be elegant. Performance isn't a requirment (at least, for now):

private static void GiveNameToUnnamedLabels(IEnumerable<Line> finalCode)
{
    int count = 0;

    void GiveName(Model.Label label)
    {
        label.Name = $"dyn_label{++count}";
    }

    var unnamed = finalCode.Where(x => x.Label != null && x.Label.Name == null).ToList();
    unnamed.ForEach(x => GiveName(x.Label));
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ I'm not really a fan of slapping a .ToList() on LINQ queries just to use the .ForEach() method of the List<T> class. I would suggest removing it and using a normal foreach loop. \$\endgroup\$ Mar 23, 2018 at 0:53

2 Answers 2

4
\$\begingroup\$

A few notes about your code:

  • Both the inner and outer method are fairly small, and the name of the outer method is already aptly descriptive, so I see no need for a named inner method here. An anonymous method would suffice.
  • List<T>.ForEach is a bit of an odd duck, in my opinion. It looks like functional programming (similar to Linq), but it's actually used to produce side-effects. Combining the two is giving off conflicting signals. A for or foreach loop, on the other hand, is clearly not functional-style, so side-effects would not be surprising.
  • I agree with keeping anonymous method parameter names short, but x isn't very descriptive. line is a little longer but much more descriptive.

Also, I find it more readable to put chained Linq calls on a line of their own:

var unnamed = finalCode
    .Where(x => x.Label != null && x.Label.Name == null)
    .ToList();

Having that said, I would go for a plain old foreach loop. It may not be as fancy, but it's simple and to the point, which should make it easier to understand:

int count = 0;
foreach (var line in finalCode)
{
    if (line.Label != null && line.Label.Name == null)
    {
        line.Label.Name = $"dyn_label{++count}";
    }
}
\$\endgroup\$
0
\$\begingroup\$

I wouldn't make a lot of changes.

I don't know which ForEach you are using. There are third-party libraries implementing extensions to the standard LINQ extension methods. If you are using one of them, there is no need for .ToList();. The ForEach can directly work on the IEnumerable<T>. Otherwise keep .ToList().

I would let the query return labels, not lines. This enables you to pass GiveName as a delegate to ForEach without an extra lambda.

I also tweaked the names a little bit and used the new C# 6.0 expression-bodied methods and inlined the temp unnamed.

private static void NameUnnamedLabels(IEnumerable<Line> finalCode)
{
    int labelNo = 0;

    void GiveName(Model.Label label) => label.Name = $"dyn_label{++labelNo}";

    finalCode
        .Select(ln => ln.Label)
        .Where(lbl => lbl != null && lbl.Name == null)
        .ToList()
        .ForEach(GiveName);
}

By projecting (Select) the line on the label before Where, this also slightly simplifies the latter.

\$\endgroup\$
5
  • 2
    \$\begingroup\$ Your first statement is incorrect. .ForEach() is not a supplied extension method on IEnumerable<T>. \$\endgroup\$ Mar 23, 2018 at 0:55
  • \$\begingroup\$ The signature of ForEach is probably public static void ForEach<T>(this IEnumerable<T> source, Action<T> action). So yes, it works on IEnumerable<T>. This does not mean that it is implemented in the base class library. \$\endgroup\$ Mar 23, 2018 at 0:58
  • 4
    \$\begingroup\$ That makes no sense whatsoever. .ForEach() is a method on List<T> and no extension method with that signature exists in the standard .NET LINQ libraries. If you're writing it, supply it, otherwise it's useless to the OP. \$\endgroup\$ Mar 23, 2018 at 1:00
  • 1
    \$\begingroup\$ There is a ForEach operator in MoreLinq. Don't know if the OP is using it. I was not aware of the one in List<T>. \$\endgroup\$ Mar 23, 2018 at 1:06
  • 2
    \$\begingroup\$ Ok, then you should state that as a requirement for your solution to work. \$\endgroup\$ Mar 23, 2018 at 1:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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