Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
    
I'd just like to point out (minor nitpick) that in pretty much every single answer. people order the properties being modified in the same order. I found your arbitrary modification order a bit confusing. – user2296177 10 hours ago

7 Answers 7

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.

share|improve this answer
    
FWIW they're called Object Initializers – RobH 18 hours ago

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();
share|improve this answer
1  
You can do the .SelectMany() immediately; no need to .Select() it and then .SelectMany() the result. – anaximander 17 hours ago
    
@anaximander You're right, though my way is more explicit, I think. I'll add your way too. – Avner Shahar-Kashtan 17 hours ago
    
This approach, projecting inputs to outputs using an item creation method paired with Select, is the way to go. You can then write unit tests for your mappings too. – moarboilerplate 13 hours ago

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

share|improve this answer
    
I tried, but syntax is incorrect. Once I use ToList() on drawDates, OrderBy() in the end doesn't work, since there are no methods available. – John Mathilda 18 hours ago
    
Let me check it and I'll edit – Oscar Guillamon 18 hours ago
    
@JohnMathilda I corrected the error I had, please try the code again. – Oscar Guillamon 17 hours ago

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();
share|improve this answer
    
Why bother passing item.DayId and item.CutOffDayId when you're already passing item? – BCdotWEB 18 hours ago
    
@BCdotWEB Because as the second item initialization shows, you don't always initialize the day id directly from the item. – Avner Shahar-Kashtan 17 hours ago
    
@AvnerShahar-Kashtan But that can be solved by introducing an offSet parameter which then can be used in the constructor. – BCdotWEB 16 hours ago
    
@BCdotWEB It can be, sure. Feel free to propose it. :) – Avner Shahar-Kashtan 16 hours ago

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.

share|improve this answer

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)
)
share|improve this answer

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.

share|improve this answer
2  
This approach is having side effects because it will change the item. – Heslacher 17 hours ago
    
This seems incorrect, you're changing the original item's DayId. – Kroltan 17 hours ago
    
@Heslacher: Appreciate your keen observation. I've updated the code. – SiD 9 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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