6
\$\begingroup\$

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>
\$\endgroup\$
6
  • 4
    \$\begingroup\$ In my humble opinion, ListOfSubscription is naming in the style of Hungarian notation at its worst. I would give the subscriptions name (in the plural). \$\endgroup\$ Oct 1 '20 at 14:30
  • 1
    \$\begingroup\$ //review this section of code. My previous answer is 100% applicable. This section, its length, complexity, harder than it needs to be to read and understand, the try covering all of it because functionality is shotgunned all over that block, ... all are symptoms of the foundational design. Code Review rules ask for complete, working code postings so accurate assessments can be made. No code can live in a vacuum. \$\endgroup\$
    – radarbob
    Oct 2 '20 at 18:15
  • \$\begingroup\$ 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. the code is working just wanted to see what improvements can be made? \$\endgroup\$
    – Jefferson
    Oct 2 '20 at 18:43
  • \$\begingroup\$ I added to my answer. \$\endgroup\$
    – radarbob
    Oct 2 '20 at 22:22
  • \$\begingroup\$ Is the code really correct? If I run it I get three SubscriptionViewModels with SubscriptionId = 0 and the latter two are identical (having Product 3,Product 4,). It would help to see your expected output. \$\endgroup\$ Oct 7 '20 at 6:58
2
+50
\$\begingroup\$

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:

result

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.

\$\endgroup\$
8
  • \$\begingroup\$ Can I flattened this and just return the SubscriptionViewModel var flattenedresult = result.SelectMany(grp => grp.AsEnumerable()).ToList(); \$\endgroup\$
    – Jefferson
    Oct 9 '20 at 14:48
  • \$\begingroup\$ I get this AnonymousType and not the SubscriptionViewModel \$\endgroup\$
    – Jefferson
    Oct 9 '20 at 15:07
  • 1
    \$\begingroup\$ Yes, that's why you need the syntax result.SelectMany(s => s.Subscriptions). s is an anonymous type here. (Or grp in your code). \$\endgroup\$ Oct 9 '20 at 15:09
  • \$\begingroup\$ what if I wanted to check OriginalSubscriptionId is null? Would I add it to grp1.GroupBy? They are different numbers so I dont think it would work . \$\endgroup\$
    – Jefferson
    Oct 29 '20 at 13:27
  • \$\begingroup\$ would something like this work (grp2.Select(y => y.OriginalSubscriptionId).Count() > 0) ? null :string.Join(",", grp2.Select(y => y.OriginalSubscriptionId)) \$\endgroup\$
    – Jefferson
    Oct 29 '20 at 13:37
6
\$\begingroup\$

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

\$\endgroup\$
8
  • 1
    \$\begingroup\$ Do not inherit from List<T> \$\endgroup\$
    – Johnbot
    Oct 2 '20 at 10:37
  • \$\begingroup\$ @Johnbot Maybe something was lost in translation but to quote that answer ""Is inheriting from List<T> always unacceptable?" Unacceptable to who? Me? No." \$\endgroup\$
    – Peilonrayz
    Oct 2 '20 at 12:25
  • \$\begingroup\$ @Johnbot, yeah, I was lazy. In real life I always use composition creating custom collections. I want a very DSL api. And leaving all those List<T>methods exposed also makes it easy too easy, and tempting(!), for client code to screw things up. \$\endgroup\$
    – radarbob
    Oct 2 '20 at 17:50
  • \$\begingroup\$ @radarbob I tried to create your view model but getting error related to Subscript? https://dotnetfiddle.net/XamjC0 \$\endgroup\$
    – Jefferson
    Oct 5 '20 at 12:51
  • \$\begingroup\$ it's a typo. the type should be "Subscription" NOTE: protected Subscript theSubscription {get; set;} - I declared it, misspelled it. The intent is to instantiate it in a constructor. \$\endgroup\$
    – radarbob
    Oct 5 '20 at 19:42

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.