Collecting data from list and then removing

Question

I created this sample and wanted any advice on how to make this code; cleaner, more effective, just overall better! I am collecting the days data from the NegativeScheduleSentence model and ScheduleSentencePart model and removing the model from the concepts object. only to have to re-add them back in using the days object from both models. Any help would be great.

sample working https://dotnetfiddle.net/5jA7xh

if (concepts.NegativeScheduleSentencePartModel.Count() > 0 &&
    concepts.ScheduleSentencePartModel.Count() > 0)
{
    var conceptsNegativeScheduleCalendarDays = 
        concepts.NegativeScheduleSentencePartModel.Select(x => x.Days).ToList();
    var PositiveandNegativeDays = new List<int>();
    for (int i = 0; i < conceptsNegativeScheduleCalendarDays.Count; i++)
    {                   
        foreach (int day in conceptsNegativeScheduleCalendarDays[i])
        {
            Console.WriteLine(day);
            PositiveandNegativeDays.Add(day);
        }
    }

    var conceptscheduleCalendarDays = concepts.ScheduleSentencePartModel
        .Select(x => x.Days).ToList();
    for (int i = 0; i < conceptscheduleCalendarDays.Count; i++)
    {
        foreach (int day in conceptscheduleCalendarDays[i])
        {
            Console.WriteLine(day);
            PositiveandNegativeDays.Add(day);
        }
    }

    concepts.ScheduleSentencePartModel
        .RemoveRange(0, ScheduleSentencePartModel.Count);
    concepts.NegativeScheduleSentencePartModel
        .RemoveRange(0, conceptsNegativeScheduleCalendarDays.Count);

    var PositiveandNegativeScheduleSentencePartModel = 
        new List<ScheduleSentencePartModel>();
    ScheduleSentencePartModel.Add(
        new ScheduleSentencePartModel() { 
            Days = PositiveandNegativeDays, Isvalid = true });
    concepts.ScheduleSentencePartModel = ScheduleSentencePartModel;
}

Answer 1

• Type names should be written in PascalCase (class Concepts). Variables should be written in camelCase (negativeScheduleSentence).
• All the public fields should be properties (PascalCase).
• concepts.NegativeScheduleSentencePartModel and concepts.ScheduleSentencePartModel are lists (List<T>). Lists have a Count property. No need to call the LINQ Count() extension method. Write concepts.NegativeScheduleSentencePartModel.Count > 0.
• NegativeScheduleSentencePartModel and ScheduleSentencePartModel are identical. From a conceptual point of view, it can sometimes make sense to have two types, because they could evolve differently in future, but then they should have a common (abstract) base class (e.g., ScheduleSentencePartModelBase) or implement the same interface. This allows you to treat them the same way. E.g., you could have a method void PrintScheduleSentence(ScheduleSentencePartModelBase scheduleSentence) that can print both models. However, in your example code I see no reason to have two types.
• You declare and initialize an unused list PositiveandNegativeScheduleSentencePartModel. Remove it unless it is used in your real code.
• Instead of someList.RemoveRange(0, someList.Count); you can write someList.Clear();.
• Classes are reference types. This means that concepts.ScheduleSentencePartModel and the variable scheduleSentencePartModel (changed to camelCase) point to the same list object. It makes no sense to re-assign the variable (last line in the code above).

It is hard to understand what you are trying to achieve with this code, but if its purpose is simply to concatenate all the days from all the lists, this can be achieved much easier. The LINQ extension method SelectMany flattens nested collections.

It still makes sense to clear the lists, because you are potentially replacing many ScheduleSentencePartModels by one containing all the days of all the models (if this was your intention, after all it is what your loops do).

if (concepts.NegativeScheduleSentencePartModel.Count > 0 &&
    concepts.ScheduleSentencePartModel.Count > 0) {

    var positiveandNegativeDays = concepts.NegativeScheduleSentencePartModel
        .SelectMany(x => x.Days)
        .Concat(
            concepts.ScheduleSentencePartModel
            .SelectMany(x => x.Days)
        )
        .OrderBy(d => d) // Optionally sort the days.
        .ToList();

    concepts.NegativeScheduleSentencePartModel.Clear();
    concepts.ScheduleSentencePartModel.Clear();

    concepts.ScheduleSentencePartModel.Add(
        new ScheduleSentencePartModel {
            Days = positiveandNegativeDays, IsValid = true 
        });
}

Is the surrounding if-statement necessary? If the lists are empty the code would still work as expected. The only effect it has is to collect the data only when both lists have entries. If only one of the two lists has entries, nothing happens. Was this intended?

• A possible problem is that you select all the entries, even those with IsValid = false and replace them by an entry with with IsValid = true. Maybe you should exclude the invalid ones.

var positiveandNegativeDays = concepts.NegativeScheduleSentencePartModel
    .Where(x => x.IsValid)
    .SelectMany(x => x.Days)
    .Concat(
        concepts.ScheduleSentencePartModel
            .Where(x => x.IsValid)
            .SelectMany(x => x.Days)
        )
    .OrderBy(d => d)
    .ToList();