trjhtr

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));

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";
 

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.