0
\$\begingroup\$

There seems to be a point of contention on my project team regarding what method should be used when trying to display something about an object that requires "lookup-logic". Namely, should their be a (lazy-loaded) property on the class being bound to (in my case a model object), or is it acceptable to use a converter to perform the lookup?

Some things to keep in mind, the examples below are just that: examples. I work in a classified environment and thus am unable to access the internet on any machine with access to the actual project code, and my post has to be written entirely from a mobile device. As such, my examples classes and relationships are entirely fabricated for this example and omit a large amount of code I do not feel is relevant for the question I am trying to ask.

Edit

Let me try to rephrase the question... If I want to bind the results of a method (specifically one that takes a model obect as a parameter), is it better to wrap the method with a (possibly lazy-loaded) property on the model object itself so that I can bind directly to it, Or is it better to bind directly to the model object and use a converter to execute the desired look up function (essentially using the converter as a data source).


Let's say I have an Individual class and that each Individual has two parents (I know this assumption doesn't always hold true in the real world, but let's pretend that it does for the sake of the example). In this case, my Individual class may look something like:

public class Individual
{
    public string FirstName {get; set;}
    public string LastName {get; set; }
    public Individual Mother {get; set;}        
    public Individual Father {get; set;}
    ...
}

Let's assume also that I have some way of looking up Individual by Parent:

public class IndividualRepository : IRepository<Individual> 
{
    public List<Individual> GetIndividualsByParent(Individual parent);
}

Now, I want to bind a DataGrid's ItemsSource property to that Individual's Children.


Option 1

Create a (lazy-loaded) Children property on Individual

public class Individual
{
    public string FirstName {get; set;}
    public string LastName {get; set; }
    public Individual Mother {get; set;}        
    public Individual Father {get; set;}
    ...

    private List<Individual> _children;
    public List<Individual> Children 
    {
        get
        {
            if (_children == null)
            {
                var repository = new IndividualRepository();
                _children = repository.GetIndividualsByParent(this); 
            }
            return _children;
        }
    }
}

I personally don't like this approach. I don't believe it is scalable. What if I later needed a property for my Individual's grandchildren or grandparent? I'd have to keep adding properties.

It also doesn't work in cases where I might need to pass additional parameters. Imagine if at some other point I needed only male children, or children under 21.


Option 2

Use IValueConverter(s)

Instead of having a Children property on Individual, I have an implementation of the IValueConverter interface that looks up an Individual's children. Instead of having a specific implementation for the IValueConverter that only looks up an Individual's Children, I've chosen to create a DelegateConverter where the converter method is not defined in the converter class but rather in the code-behind or viewmodel:

public class DelegateConverter:IValueConverter 
{
    public Func<object,object> Method {get;set;}
    // Or if using a converter-parameter...
    public Func<object,object,object> MethodWithParameter {get;set;}

    object IValueConverter.Convert(object value, Type targetType, object parameter, ....)
    {
       if (MethodWithParameter != null)
       {
          return MethodWithParameter(value, parameter);
       }
       else
       {
          return Method(value);
       }
    } 
}

XAML:

    <Grid>
    <Grid.Resources>
        <Converter:DelegateConverter x:Key="GetChildrenConverter" Method="LookupChildrenForParent" />
    </Grid.Resources>
    </Grid>

xaml.cs:

private object LookupChildrenForParent(object parameter)
{
    //Do lookup here directly, or call a method in the ViewModel,
    //Really doesn't mater....
    var rep = new IndividualRepository();
    return rep.GetIndividualsByParent(parameter as Individual);
}

I really like this approach. I can easily pass addition parameters via the converter parameter. Also, because the conversion method is actually located in the code-behind or ViewModel it has access to whatever additional properties are in the ViewModel that it may need in order to perform the lookup.

My team member, however, thinks that goes against the Intent of converters since it's not really converting anything. It's performing a lookup (or delegating the lookup to a method in the code-bind and/or ViewModel). He is not particularly familiar with IValueConvertersand wants to make sure that we are not "going against the grain" or otherwise violating some industry best-practice. Unfortunately most examples of IValueConverters online are limited to very very simple logic (i.e. converting a bool to Visibility).


Is either of these approaches "incorrect" or "bad design" and is there a better option?

\$\endgroup\$

closed as off-topic by Vogel612, forsvarir, mdfst13, ferada, Mast Feb 15 '17 at 12:54

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ So a method that saves values is not scalable but your converter that looks up every time is scalable? \$\endgroup\$ – paparazzo Feb 14 '17 at 9:15
  • \$\begingroup\$ @Paparazzi The issue I am asking about is not performance related. I don't have access to any project source code on any machine with an internet connection (perks of working in a classified environment). Therefore my examples are only meant to illustrate the concepts we are working with. Obviously I would want to keep an in-memory copy of the data once it's been looked up, but I don't feel that the Business class (the Individual Model in my example) is the appropriate location for this. My thoughts are that the ViewModel should be responsible for this work. \$\endgroup\$ – NuclearProgrammer Feb 14 '17 at 12:24
  • 1
    \$\begingroup\$ But you are not keeping it in memory. My thought is you are wrong. The ViewModel should get data from the Model. Get children is business logic \$\endgroup\$ – paparazzo Feb 14 '17 at 12:31
  • \$\begingroup\$ Perhaps I am miscommunicating the layers of my app. What I am calling my "business classes" are my models. My Viemodels I am considering to completely separate. I want my ViewModels to have the logic for looking up additional information about my models (by executing a LINQ query using the model as a parameter) but My team member feels that the model itself should just have a property that can be bound to directly without any involvement of the ViewModel (or converters). \$\endgroup\$ – NuclearProgrammer Feb 14 '17 at 13:12
2
\$\begingroup\$

Let's go through your question and your choice step by step.


Option 1

Create a (lazy-loaded) Children property on Individual I personally don't like this approach. I don't believe it is scalable. What if I later needed a property for my Individual's grandchildren or grandparent? I'd have to keep adding properties.

The above concerns you are having, have nothing to do with the class Individual. Why would you need a property for your individuals grandchildren or grandparent: Those are all there already. You need to use the Individual class wisely and you can represent all of those relationships with the Individual class. For example, you can add one more property to your Individual class like below:

public class Individual
{
    private List<Individual> _children;
    public List<Individual> Children 
    {
        get
        {
            if (_children == null)
            {
                var repository = new IndividualRepository();
                _children = repository.GetChildren(this); 
            }
            return _children;
        }
    }

    private List<Individual> _parents;
    public List<Individual> Parents 
    {
        get
        {
            if (_parents == null)
            {
                var repository = new IndividualRepository();
                _parents = repository.GetParents(this); 
            }
            return _children;
        }
    }
}

That class will be good enough for getting children, grand-children, grand-grand-children and same for parents. Think of this as a directory structure, think recursively. Also, note I have changed the method names in the repo:

GetIndividualsByParent  // Changed to --> GetChildren

That is more clear because you will use the method like this:

// no point in saying ByParent because if you add another method for age
// you will not say ByParentByAge and if you added another method...
// you get the point 
repository.GetChildren(this); 

It also doesn't work in cases where I might need to pass additional parameters. Imagine if at some other point I needed only male children, or children under 21.

You have a few of options here:

  1. If you do not care about memory, then load all the children and let the caller filter only male or children under 21.
  2. Create a method which takes a predicate Func<TSource, bool> predicate and send it to the repository.

Option 2

Use IValueConverter(s)

Instead of having a Children property on Individual, I have an implementation of the IValueConverter interface that looks up an Individual's children.

Here is the documentation for IValueConverter from MSDN:

If you want to associate a value converter with a binding, create a class that implements the IValueConverter interface and then implement the Convert and ConvertBack methods. Converters can change data from one type to another, translate data based on cultural information, or modify other aspects of the presentation.

The key part in the above is: Converters can change data from one type to another. You are not changing from one type to another but you are taking advantage of when this is triggered by WPF and you figured to use it for getting children. This is not good and I call this a hack. This is when you have taken something which every developer who is used to WPF, is well aware of it functionality but only to be surprised that you are using it for something completely different. I knew a developer once who used SqlDataAdapter.Fill method but in the command he had an update query. It worked and his argument was that it works so what is the issue. Interfaces and patterns are there for a reason so developers can quickly understand what you are doing. So it is wise to stick to them and not abuse them.

Instead of having a specific implementation for the IValueConverter that only looks up an Individual's Children, I've chosen to create a DelegateConverter where the converter method is not defined in the converter class but rather in the code-behind or viewmodel:

Does not matter where it is defined. It is not a good idea.

My team member, however, thinks that goes against the Intent of converters since it's not really converting anything. It's performing a lookup (or delegating the lookup to a method in the code-bind and/or ViewModel). He is not particularly familiar with IValueConvertersand wants to make sure that we are not "going against the grain" or otherwise violating some industry best-practice.

Your team member has every right to be concerned and he/she may not be familiar with IValueConverter but a quick google has led them to the right path. Yes, it is against the intent of converters and yes it is against the industry standard.


Conclusion

A ViewModel's sole purpose is to be there for the UI and serve the UI. Therefore, having many properties such as Children, Parents, Males, Females and on and on should not be a problem. Just make sure the thing which is populating those properties is not duplicating code and it is using the same code because at the end whether they are children, parents or males or females, they are all individuals.

\$\endgroup\$
  • \$\begingroup\$ I think some of the problem I have with this is coming from how our team uses ViewModels. Essentially, each "transaction" has a single ViewModel... so one ViewModel can represent many, many models. Our ViewModel for example may have a Collection of Individuals I want to list, And I want to list each Individuals children, grandchildren, etc. It sounds like this is not the correct way to use ViewModels. From they way you and others describe it, I should have a "IndividualViewModel" that has children (and grand children, etc.) Is this correct? \$\endgroup\$ – NuclearProgrammer Feb 14 '17 at 18:57
  • \$\begingroup\$ It is not that it is incorrect but it is a different way of doing it. Both are fine. In some circumstances if there is a parent child relationship then a recursive viewmodel makes sense. But in other cases a viewmodel with collection properties makes sense. Most devs will shy away from the recursive one because they do not like recursion. \$\endgroup\$ – CodingYoshi Feb 14 '17 at 19:28
  • \$\begingroup\$ Choosing a recursive viewmodel vs a flat model depends on what you are displaying in the ui. So the decision depends on the requirement. My suggestion to you is to put a small application together which mimics your architecture and then ask about that. \$\endgroup\$ – CodingYoshi Feb 14 '17 at 19:31
1
\$\begingroup\$

Indexing

I am not sure if there is any reason why you cannot introduce key/indexing into your classes, but I would imagine it would help optimize your queries greatly.

public class Individual
{
    public int Id { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public Individual Mother { get; set; }
    public int MotherId { get; set; }
    public Individual Father { get; set; }
    public int FatherId { get; set; }
}

Option 1 vs Option 2

There is some code smell behind your Option 2 approach to me, but I can't quite put a finger on it.

I'm not exactly sure what you're trying to achieve with your lazy-loading, but I would imagine most of your concerns with issues about parameters and code constraints in your Option 1 would be solved by using a virtual modifier property to allow for overrides in the future.

public virtual IEnumerable<Individual> Children { get; set; }

Note that I put forward a suggested change for an IEnumerable type, so you can run additional filters or queries without the performance hit of enumerating multiple times.

var childrenUnderTwentyOne = individual.Children.Where(child => child.Age < 21);
var grandChildren = individual.Children.Select(child => child.Children);

Entity Framework

Have you considered using EF to assist you? If you want to roll your own solution, maybe you can sift through the open source version that came with ASP.NET Core here.

\$\endgroup\$
  • \$\begingroup\$ The example I provided is a very simple example that omits much of the complexity of our classes. Do to the classified nature of my development environment, I'm unable to post any actual project code. Our actual code base does in-fact use indexing and EF. \$\endgroup\$ – NuclearProgrammer Feb 14 '17 at 12:28
  • \$\begingroup\$ That said, the question I am trying to get an answer for is related to WPF Binding. Specifically, If I want to bind to the results of a method call that takes a model object as a parameter (or an ID anyway), Is it better to wrap that method in a (lazy-loaded) property on that model object that can be bound to directly, or is it better to bind directly to the model object and use a converter to execute the desired function passing in whatever parameters are necessary (presumably the model object or its ID). \$\endgroup\$ – NuclearProgrammer Feb 14 '17 at 12:35
  • \$\begingroup\$ @NuclearProgrammer May I suggest you put the code-behind for Option 1 as well? Regrettably, it was not very clear to me. I'm getting the feeling that you're fishing for a good reason not to go with the converter rather than follow convention. :) \$\endgroup\$ – Svek Feb 14 '17 at 12:43
  • \$\begingroup\$ @NuclearProgrammer here is a link anyway, hopefully a re-read might get you to be willing to apply EF to solve it msdn.microsoft.com/en-us/library/… \$\endgroup\$ – Svek Feb 14 '17 at 12:46
  • \$\begingroup\$ actually I would much prefer the use of the converter, but my team member (who has more seniority than me) doesn't like the "feel" of it, mostly because in this situation we're using a converter to retrieve additional information about an entity rather than converting it to a different type (the actual lookup uses a EF/LINQ query). My team member told me to find justification for this approach by going online. Unfortunately I've found no online resource that discusses using converters in this manor (much less arguing for or against the approach). This is why I posted the question here. \$\endgroup\$ – NuclearProgrammer Feb 14 '17 at 13:02

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