Simple foreach adding to a list

Question

I am having a simple foreach where I add objects to a list:

    List<SoonestDrawDateModel> soonestDrawDateModel = new List<SoonestDrawDateModel>();

    foreach (var item in drawDates)
    {
        SoonestDrawDateModel sdModel = new SoonestDrawDateModel();
        sdModel.DrawDay = item;
        sdModel.DrawDayId = item.DayId;
        sdModel.CutOffDayId = item.CutOffDayId;

        soonestDrawDateModel.Add(sdModel);

        sdModel = new SoonestDrawDateModel();
        sdModel.DrawDayId = item.DayId + 7;
        sdModel.CutOffDayId = item.CutOffDayId;
        sdModel.DrawDay = item;
        soonestDrawDateModel.Add(sdModel);
    }

    var realDrawDates = soonestDrawDateModel.OrderBy(x => x.DrawDayId);

Is there any way to do this with Linq with fewer lines?

Answer 1

What you're doing here, essentially, is creating two Model items for each item in your source list, and storing them in a list. Here's a somewhat contrived usage of the SelectMany LINQ function which flattens a hierarchical list:

drawDates
  .Select(dd => new SoonestDrawDateModel[2]  // create a 2-item array for each item.
                 { 
                   new SoonestDrawDateModel 
                    { 
                      DrawDay = item, 
                      DrawDayId = item.drawDayId,
                      CutOffDayId = item.CutOffDayId
                    },
                   new SoonestDrawDateModel 
                    { 
                      DrawDay = item, 
                      DrawDayId = item.drawDayId + 7,
                      CutOffDayId = item.CutOffDayId
                    } 
                }) // end of Select method.
 .SelectMany(items => items) // flatten collection of arrays into one collection.
 .OrderBy(x => x.DrawDayId); // order by.
 .ToList() //convert to a List<SoonestDrawDateModel>.

What we're doing here is creating a 2-item array or each Draw item, meaning we have an IEnumerable<SoonestDrawDateModel[]>, then use SelectMany to take all the members of each SoonestDrawDateModel[] and flatten them into one big IEnuemrable<SoonestDrawDateModel>, which we then sort and return.

You can simplify the syntax by extracting the main creation block into a method:

private SoonestDrawDateModel[] CreateDateModelPair(DrawItem item)
{
    return new SoonestDrawDateModel[2]
    { 
     new SoonestDrawDateModel 
     { 
        DrawDay = item, 
        DrawDayId = item.drawDayId,
        CutOffDayId = item.CutOffDayId
     },
     new SoonestDrawDateModel 
     { 
        DrawDay = item, 
        DrawDayId = item.drawDayId + 7,
        CutOffDayId = item.CutOffDayId
     } 
  }
}

then call it with this LINQ call that conveys the intent rather well:

drawDates
  .Select(CreateDateModelPair)
  .SelectMany (dateModels => dateModels)
  .OrderBy (dateModel => dateModel.DrawDayId)
  .ToList();

Or, as @anaximander suggests, an even terser (but less explicit) version that combines the Select and SelectMany calls. It saves a step, but it's less clear that we have two things going on - one to convert the DrawDate to two SoonestDrawDateModels, and another to flatten that list of lists.

drawDates
  .SelectMany (CreateDateModelPair)
  .OrderBy (dateModel => dateModel.DrawDayId)
  .ToList();

Answer 2

If you don't have access to changing the constructor, you could opt for the following syntax:

foreach (var item in drawDates)
{
    var sdModel = new SoonestDrawDateModel() {
         DrawDay = item,
         DrawDayId = item.DayId,
         CutOffDayId = item.CutOffDayId;
     };
    soonestDrawDateModel.Add(sdModel);

    # Alternatively, do it directly in the Add method
    soonestDrawDateModel.Add(new SoonestDrawDateModel() {
         DrawDayId = item.DayId + 7,
         CutOffDayId = item.CutOffDayId,
         DrawDay = item
    });
}

This is not Linq syntax, but it is still a rather neat way of adding newly created objects without (in the alternative method) using temporary variables. It also connects the initiliasation and default setting of public properties/variables neatly into a block of its own.

Answer 3

Try this:

List<SoonestDrawDateModel> soonestDrawDateModel = new List<SoonestDrawDateModel>();

var realDrawDates = drawDates.ForEach(x => 
{
    soonestDrawDateModel.Add(new SoonestDrawDateModel() { CutOffDayId = x.CutOffDayId, DrawDay = x, DrawDayId = x.DayId});
    soonestDrawDateModel.Add(new SoonestDrawDateModel() { CutOffDayId = x.CutOffDayId, DrawDay = x, DrawDayId = x.DayId + 7});
});

var realDrawDates = soonestDrawDateModel.OrderBy(y => y.DrawDayId);

Basically, this does the same as yours, but this iteration is more compacted.

EDIT: It seems I messed up while pasting the code, sorry for that, this should work now, try it and let me know

Answer 4

If I were to code-golf this question, I would probably go for something like

//use plural to indicate that this is actually a collection of models
//and not a single model
var soonestDrawDateModels = 
//for every date create model with and without offset
drawDates.Select(date => new SoonestDrawDateModel(date))
         .Concat(drawDates.Select(date => new SoonestDrawDateModel(date, 7)))
         .OrderBy(model => model.DrawDayId)
         .ToList();

I think this solution is not only short, but also pretty readable.

Answer 5

You can make a constructor that accepts the 3 parameters:

foreach (var item in drawDates)
{
    SoonestDrawDateModel sdModel = new SoonestDrawDateModel(item, item.DayId, item.CutOffDayId);
    soonestDrawDateModel.Add(sdModel);

    sdModel = new SoonestDrawDateModel(item, item.DayId + 7, item.CutOffDayId);
    soonestDrawDateModel.Add(sdModel);
}

in linq that lets you do

drawDates.map(item => new SoonestDrawDateModel(item, item.DayId, item.CutOffDayId))
       .concat(drawDates.map(item => new SoonestDrawDateModel(item, item.DayId + 7, item.CutOffDayId)).OrderBy(x => x.DrawDayId).ToList();

Answer 6

Just use the Concatenation method. That's really what you're doing. You can also use the List constructor that takes an IEnumerable.

var soonestDrawDateModel = new List<SoonestDrawDateModel>(
    drawDates.Select(s => new SoonestDrawDateModel()
        {
            DrawDay = s,
            DrawDayId = s.DayId,
            CutOffDayId = s.CutOffDayId,
        })
        .Concat(
            drawDates.Select(s => new SoonestDrawDateModel()
                {
                    DrawDay = s,
                    DrawDayId = s.DayId + 7,
                    CutOffDayId = s.CutOffDayId,
                })
        )
        .OrderBy(b => b.DrawDayId)
)

Answer 7

        soonestDrawDateModel.Add
(
new SoonestDrawDateModel
{
  DrawDay=item.Drawday,
DrawDayId=item.DayId,
CutOffId=item.CutOffId
}
);

Try with this kind of syntatic sugar code will look neat and clean .hope it will help you write better code.

Answer 8

Considering the LINQ generated from refactoring this code, I think cleaning this up might require more effort than just using LINQ.

For example, you're assigning the item directly to the model, but then you have other properties that are just mapped directly from properties on the item. I'd remove the item property completely or change the model to only contain an item.

I realize doing so immediately introduces difficulties: You have a scenario where you're assigning the model's DrawDayId to the item's DayId + 7 instead of directly mapping it. Okay, but I also noticed you don't reassign the DayId of the item itself. Even if that inconsistency is not problematic, it is an inconsistency that should be avoided. Having this inconsistency around means that object properties that are arguably redundant are not actually redundant because they might differ based on what workflow assigned them at what point in time.

Lastly, since it appears this model is just a bag of properties to bind to, I'd make the model's fields all read-only or private set, so that the model is immutable once it is created. If the model's fields are getting initialized or reassigned elsewhere, that's usually a sign of fragmented code that needs to be reworked.

Answer 9

Repeated Code

I can see the repeated code, which can be removed by introducing following method:

public static void AddToSoonestDrawDateModel(string day, int dayId, int cutOffDayId)
{
    var sdModel = new SoonestDrawDateModel
    {
        DrawDay = day,
        DrawDayId = dayId,
        CutOffDayId = cutOffDayId
    };

    soonestDrawDateModel.Add(sdModel);
}

After refactoring code should be like this:

var soonestDrawDateModel = new List<SoonestDrawDateModel>();

foreach (var item in drawDates)
{
    AddToSoonestDrawDateModel(item.Day, item.DayId, item.CutOffDayId);
}

var realDrawDates = soonestDrawDateModel.OrderBy(x => x.DrawDayId);

Obviously, it is not LINQ, but it is more neat, readable and less repetitive code.