4
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – user2296177 Nov 20 '15 at 19:36
5
\$\begingroup\$

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();
\$\endgroup\$
  • 3
    \$\begingroup\$ You can do the .SelectMany() immediately; no need to .Select() it and then .SelectMany() the result. \$\endgroup\$ – anaximander Nov 20 '15 at 12:55
  • \$\begingroup\$ @anaximander You're right, though my way is more explicit, I think. I'll add your way too. \$\endgroup\$ – Avner Shahar-Kashtan Nov 20 '15 at 12:55
  • \$\begingroup\$ 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. \$\endgroup\$ – moarboilerplate Nov 20 '15 at 16:30
  • \$\begingroup\$ I too see no need for the additional Select call but other than that the explicit method is the way to go. \$\endgroup\$ – Esben Skov Pedersen Nov 21 '15 at 16:07
  • 1
    \$\begingroup\$ @EsbenSkovPedersen I thought of that, but I think adding yet another variant would confuse the answer. :) \$\endgroup\$ – Avner Shahar-Kashtan Nov 22 '15 at 10:21
3
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ @JohnMathilda can you give me feedback if the code worked for you? It did for me but maybe in your implementation doesn't work much as expected, thanks! \$\endgroup\$ – Oscar Guillamon Nov 23 '15 at 7:33
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ It is quite clear and better than what I suggested since I don't use LINQ the way you did, if I used it at all. \$\endgroup\$ – Oscar Guillamon Nov 23 '15 at 7:34
0
\$\begingroup\$

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();
\$\endgroup\$
  • \$\begingroup\$ Why bother passing item.DayId and item.CutOffDayId when you're already passing item? \$\endgroup\$ – BCdotWEB Nov 20 '15 at 11:05
  • \$\begingroup\$ @BCdotWEB Because as the second item initialization shows, you don't always initialize the day id directly from the item. \$\endgroup\$ – Avner Shahar-Kashtan Nov 20 '15 at 13:00
  • \$\begingroup\$ @AvnerShahar-Kashtan But that can be solved by introducing an offSet parameter which then can be used in the constructor. \$\endgroup\$ – BCdotWEB Nov 20 '15 at 13:24
  • \$\begingroup\$ @BCdotWEB It can be, sure. Feel free to propose it. :) \$\endgroup\$ – Avner Shahar-Kashtan Nov 20 '15 at 13:27
0
\$\begingroup\$

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)
)
\$\endgroup\$
0
\$\begingroup\$
        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.

\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$
-2
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ This approach is having side effects because it will change the item. \$\endgroup\$ – Heslacher Nov 20 '15 at 12:10
  • \$\begingroup\$ This seems incorrect, you're changing the original item's DayId. \$\endgroup\$ – Kroltan Nov 20 '15 at 12:52
  • \$\begingroup\$ @Heslacher: Appreciate your keen observation. I've updated the code. \$\endgroup\$ – SiD Nov 20 '15 at 20:47

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.