c# list object with view model child list

Question

I am trying to build this view model that has a ChildViewModel and this was the only way I was able to get it working. Any advice on how to make this code; cleaner, more effective, just overall better! It’s just the adding to view model that will need to be reviewed the other section is to create the mock up.

    static void Main(string[] args)
    {
        try
        {
         //Setup Database object
        List<Subscription> ListOfSubscription = new List<Subscription>();
        ListOfSubscription.Add(new Subscription() { SubscriptionId = 1, ParentProductId = 4, ChildProductId = 4 , ParentProductName = "Product 1", ChildProductName = "Product 1", GroupId = 362 });
        ListOfSubscription.Add(new Subscription() { SubscriptionId = 2, ParentProductId = 114, ChildProductId = 1, ParentProductName = "Product 2", ChildProductName = "Product 3", GroupId = 1 });
        ListOfSubscription.Add(new Subscription() { SubscriptionId = 3, ParentProductId = 114, ChildProductId = 2, ParentProductName = "Product 2", ChildProductName = "Product 4",GroupId = 1 });
            //Review Section of the Code
            var groupedListOfSubscription = ListOfSubscription.GroupBy(u => u.GroupId).ToList();

            List<SubscriptionViewModel> SubscriptionViewModel = new List<SubscriptionViewModel>();

            foreach (var record in groupedListOfSubscription)
            {
                
                int groupId = record.Key;
                var SelectListofSubscription = ListOfSubscription.Where(w=> w.GroupId == groupId).ToList();
                foreach (var subscription in SelectListofSubscription)
                {
                    SubscriptionViewModel svm = new SubscriptionViewModel();
                    if (subscription.ParentProductId == subscription.ChildProductId)
                    {
                        svm.ParentProductId = subscription.ParentProductId;
                        svm.SubscriptionIds = subscription.SubscriptionId.ToString();
                        svm.GroupId = subscription.GroupId;
                        svm.ParentProductName = subscription.ParentProductName;
                        SubscriptionViewModel.Add(svm);
                    }
                    else
                    {
                        svm.ParentProductId = subscription.ParentProductId;
                        svm.GroupId = subscription.GroupId;
                        svm.ParentProductName = subscription.ParentProductName;
                        var SelectChildListofSubscription = ListOfSubscription.Where(w => w.GroupId == groupId).ToList();
                        StringBuilder builderSubscriptionIds = new StringBuilder();
                        List<SubscriptionChildViewModel> _scvm = new List<SubscriptionChildViewModel>();
                        foreach (var child in SelectChildListofSubscription)
                        {
                            builderSubscriptionIds.Append(child.ChildProductName);
                            builderSubscriptionIds.Append(",");                                
                            _scvm.Add(new SubscriptionChildViewModel()
                            {
                                ChildProductId = child.ChildProductId,
                                ChildProductName = child.ChildProductName
                            });
                        }
                        svm.ListOfSubscriptionChild = _scvm;
                        svm.SubscriptionIds = builderSubscriptionIds.ToString();
                        SubscriptionViewModel.Add(svm);
                    }
                }
            }
        }
        catch (Exception ex)
        {
            ex.ToString();
        }
    }
    class Subscription
    {
        public int SubscriptionId { get; set; }
        public int ParentProductId { get; set; }
        public string ParentProductName { get; set; }
        public string ChildProductName { get; set; }
        public int ChildProductId { get; set; }
        public int GroupId { get; set; }
        public DateTime? EndDate { get; set; }
    }
    class SubscriptionViewModel
    {
        public int SubscriptionId { get; set; }
        public int ParentProductId { get; set; }
        public string ParentProductName{ get; set; }
        public string SubscriptionIds { get; set; }
        public int GroupId { get; set; }
        public List<SubscriptionChildViewModel> ListOfSubscriptionChild { get; set; }
    }
    class SubscriptionChildViewModel
    {
        public string ChildProductName { get; set; }
        public int ChildProductId { get; set; }
    }

Output

<Subscription>
      <Parent>Product 1</Parent>
      <ProductId>4</ProductId>
      <SubscriptionIds Ids="1" />
</Subscription>
<Subscription>
    <Parent>Product 2</Parent>
      <ProductId>114</ProductId>
      <SubscriptionIds Ids="2,3" />
      <Children>
        <Child Name="Product 3">
          <ChildProductId>1</ChildProductId>
        </Child>
        <Child Name="Product 4">
          <ChildProductId>2</ChildProductId>
        </Child>
      </Children>
</Subscription>

Answer 1

The code pattern var x = new List<X>() followed by a foreach adding to that list is always a telltale that LINQ will probably make things easier and more comprehensible.

Your code is not exception to that rule. I'll show you how this can be done quite concisely using LINQ and then explain a few things:

var groupedSubscriptions = subscriptions.GroupBy(u => u.GroupId);

var result = groupedSubscriptions.Select(grp1 => new
{
    GroupId = grp1.Key,
    Subscriptions = grp1.GroupBy(subscr => new 
    { 
        subscr.ParentProductId, 
        subscr.ParentProductName 
    })
    .Select(grp2 => new SubscriptionViewModel
    {
        GroupId = grp1.Key,
        ParentProductId = grp2.Key.ParentProductId,
        ParentProductName = grp2.Key.ParentProductName,
        SubscriptionIds = string.Join(",", grp2.Select(y => y.SubscriptionId)),
        ListOfSubscriptionChild = grp2
            .Where(subsc => subsc.ChildProductId != grp2.Key.ParentProductId)
            .Select(subsc => new SubscriptionChildViewModel
            {
                ChildProductId = subsc.ChildProductId,
                ChildProductName = subsc.ChildProductName
            })
            .ToList()
    })
});

In Linqpad, the final result, result.SelectMany((s => s.Subscriptions) shows this:

Remarks:

• I used prevailing naming conventions (subscriptions is your ListOfSubscription ).
• You only grouped by GroupId and omitted a second required grouping level, ParentProductId. So I added a nested grouping by ParentProductId which I accompanied by ParentProductName to have a Key that can readily be used for the view model properties. And both properties should be always be coupled, right?
• As you see, populating ListOfSubscriptionChild is just a subquery on the nested grouping. No need to loop through SubscriptionViewModels to populate their child lists.

A note on architecture:

In a remark you say "The database is flat", which probably means it presents the source data shaped like your ListOfSubscription. I probably don't have to explain to you that this is not well normalized, like not at all. Especially the redundant product names are a nuisance an a possible source of errors and ambiguity. If you have even the slightest chance of improving this database model, go for it. The database should be able to readily produce the data more or less in the form of the end result.

Answer 2

Database design vs Object Oriented Class Design

The code has a look and feel of "database think" not Object Oriented think. Often the database schema naturally overlaps the code classes but that should never be an explicit class design criteria. This code is not object oriented in the least. Reasons why:

• all classes have no methods - just like database tables.
• parent objects are instantiated with child properties. This implies that a parent object cannot exist unless it already has children.
• child properties are passed to parents but the child object does not exist.
• no constructors. Constructor parameters force client code to supply the minimal required properties for a valid object. Constructor overloads can make more complex instantiations easier.
• classX properties exist also as ClassY instance properties.
• The client code, Main(), does all functionality to and for all other classes - just like SQL does for tables.

---

Good OO classes hide state and expose functionality

• Without constructors client code cannot guarantee bug free objects without knowing about every property and how each is used anywhere and everywhere after creation.
• Without constructors the created object cannot guarantee to the client code that it is in a valid, bug free state.
• A sure candidate for a method: anywhere client code applies conditional logic directly on another class' property.
• The View classes should contain whole data objects, not disconnected object properties. The View will have its own methods/properties for exposing the encapsulated data objects as and how desired.

---

Are You My Mother?

In the children's book the baby bird asks each animal it meets, "are you my mother?". If this book was about OO, the title would be aPuppy.IsParent(babyBird). In contrast, our version has Main()` strapping down the helpless tiny bird and puppy, alien probing their DNA.

---

.NET Collections

There is lots of magic built into .NET collection classes. You can leverage Add(), Contains(), Find(), Sort() and more if you design the "collected" class right. For a Subscriptions list for example, in Subscription class override Equals and CompareTo(). You really must read the MSDN documentation.

The volume of collection classes MSDN documentation is stunning, but just focus on "equals" and/or "compareTo". You do not have to implement or override the brazillion interfaces and virtual methods.

The grouping and filtering LINQ can then be put into Subscriptions class which is where it belongs, with methods like SortThisWay(...)

 class Subscription : IEquatable<Subscription> {
     public bool Equals( Subscription thatOne ) {
        if ( thatOne == null ) return false;
        if ( this.Id == thatOne.Id ) return true;
        return false;
     }
 }

 class Subscriptions : List<Subscription> {
    public bool Add( thisSubscription ) {
       if ( thisSubscription == null ) return false;

       // uses Subscription.Equals in the background.
       if ( this.Contains( thisSubscription ) return false;

       this.Add( thisSubscription );
       return true;
    }

    public Subscription FindById( thisSubscription ) {
        this.Find( x => x.Equals( thisSubscription )
    }

 }

---

Edit

I am not sure what to do with that answer its very high level. The database is flat and I am trying to add it to a view model.

---

not this:

foreach (var subscription in SelectListofSubscription)
            {
                SubscriptionViewModel svm = new SubscriptionViewModel();
                if (subscription.ParentProductId == subscription.ChildProductId)
                {
                    svm.ParentProductId = subscription.ParentProductId;
                    svm.SubscriptionIds = subscription.SubscriptionId.ToString();
                    svm.GroupId = subscription.GroupId;
                    svm.ParentProductName = subscription.ParentProductName;
                    SubscriptionViewModel.Add(svm);
                }
                else
                {
                    svm.ParentProductId = subscription.ParentProductId;
                    svm.GroupId = subscription.GroupId;
                    svm.ParentProductName = subscription.ParentProductName;
                    var SelectChildListofSubscription = ListOfSubscription.Where(w => w.GroupId == groupId).ToList();
                    StringBuilder builderSubscriptionIds = new StringBuilder();
                    List<SubscriptionChildViewModel> _scvm = new List<SubscriptionChildViewModel>();
                    foreach (var child in SelectChildListofSubscription)
                    {
                        builderSubscriptionIds.Append(child.ChildProductName);
                        builderSubscriptionIds.Append(",");                                
                        _scvm.Add(new SubscriptionChildViewModel()
                        {
                            ChildProductId = child.ChildProductId,
                            ChildProductName = child.ChildProductName
                        });
                    }
                    svm.ListOfSubscriptionChild = _scvm;
                    svm.SubscriptionIds = builderSubscriptionIds.ToString();
                    SubscriptionViewModel.Add(svm);
                }
            }

this

Subscriptions mySubscriptions = new Subscriptions();
// assume all objects and collection are created

SubscrptionsVMs my SubscriptionsVMs = new SubscriptionsVMs();
// assume all objects are created.


mySubscriptionVMs.Add(mySubscriptions);

// that's all folks!

------- The above courtesy of the following -------

.

• The view model object has a subscription object
• view model properties pass through the subscription properties. They are transformed as needed for the VM. There could be several "forms" of a property, as needed.
• I don't know if Subscription children are "products" or "subscriptions", or what.
• much of the original nested code goes away because the view models hold the entire object instead of "cutting out" a sub-set of a subscriptions object.
• if a given VM does not use a certain Subscription property, then just ignore it. Don't create a matching pass-through property.
• GroupId - note how the original object, a Subscription, returns the value untouched. It is up to the view model to transform it as desired. And note how that logic is distributed between the view model and view model collection.

.

public class SubscriptionsVMs : IEnumerable<List<SubscriptionVM>> {
    protected List<SubscriptionVM> theScripts {get; set;}

    public string GroupIds() {
        Stringbuilder groupIds = new Stringbuilder();
        
        forEach(var script in theScripts) {
           groupIds.Append(script.Id + ",");
        }

       return groupIds.ToString();
    }

    // for client code to "forEach" SubscriptionsVMs collection we need to implement IEnumerable<T> - this one method, that's it.
    // List<T> already implements an Enumerator.
    public IEnumerator<List<SubscriptionVM>> GetEnumerator() {
       return theScripts.GetEnumerator();
    }

    public bool Add(SubscriptionVM thisGuy) {
       if( thisGuy == null ) return false;
       
        theScripts.Add(thisGuy);
        return true;
    } 

    public bool Add(SubscriptionVMs these) {
       if( these == null ) return false;
       
        theScripts.Add(these);
        return true;
    }  


    public bool Add(Subscriptions theseGuys) {
       if( theseGuys == null ) return false;

       forEach(Subscription guy in theseGuys) {
          if( guy == null ) continue;
          Add(new SubscriptionVM( guy ))
       }

       return true;
    }
}


public class SubscriptionVM {
    protected Subscript theSubscription {get; set;}

    public SubscriptionVM(Subscription thisGuy ) {
       if( thisGuy = null ) { // throw ArgumentNullException }

       theSubscription = thisGuy;
    }


    // I assume the basic VM wants it as a string
    public string GroupId { return theSubscription.GroupId.ToString(); }

    public in ParentProductId { return theSubscription.ParentProductId; }
    // and so on

    public SubscriptionVM ( Subscription aSubscript ) {
        if ( aSubscript = null ) { 
         // throw ArguementNullException
         }

         this.theSubscription = aSubscript
    }
} //class SubscriptionVM


public class Subscriptions {
    // constructor not shown
    protected List<Subscription> theSubscriptions {get; set;}

    public List<int> GroupIds() {
        List<int> groupies = new List<int>();

        forEach (var subscript in theSubscriptions) {
            groupies.Append(subscript.GroupId);
        }

        return groupies;
     }

    // for client code to "forEach" Subscriptions collection we need to implement IEnumerable<T> - this one method, that's it.
    // List<T> already implements an Enumerator.
    public IEnumerator<List<Subscription>> GetEnumerator() {
       return theSubscriptions.GetEnumerator();
    }     
} // class Subscriptions


public class Subscription : IEquatable<Subscription> {
    // constructor not shown

    public Subscriptions Children {get; protected set;}

    // my guess as to what comparing these Ids means.
    // Testing Children collection for empty would be good
    public bool HasChildren() {
       return this.ParentProductId == this.ChildProductId ? false : true;
    }
}

end Edit