Combobox with multiple itemssources MVVM

Question

I'm trying to make a list of either customers or suppliers based on a radio button. I have a solution that works but I want to know if it is a correct solution to work with the MVVM-model. Next to that I want to know if I make correct use of the collecions. If there would be a more efficient way of coding for this part, I'd very happy to hear it.

I have one interface (customers and suppliers) which both implement an interface (IStakeholder). In my UI I have a two radio buttons and a combobox. Based on which radio button is used, another list should be displayed in the combobox. In my InputViewModel I use three list, one general for the combobox (CustomersSuppliers), one for customers (CustomerList) and one for Suppliers (SupplierList). The code is found below. To generate the INotifyProperty, I use Fody Weaver.

C# - ViewModel

public class InputViewModel : BaseViewModel
 {
 // Combobox list
 public IEnumerable<Customer> CustomerList { get; set; }
 /// <summary>
 /// List with suppliers to choose from
 /// </summary>
 public IEnumerable<Supplier> SupplierList { get; set; }
 /// <summary>
 /// List with either customers or suppliers
 /// Choice depends on radiobox in the View
 /// </summary>
 public ObservableCollection<IStakeholder> CustomersSuppliers { get; set; }
 /// <summary>
 /// Binds to the Customer Radio Button
 /// </summary>
 public bool ButtonCustomerIsChecked
 {
 get => _buttonCustomerIsChecked;
 set
 {
 _buttonCustomerIsChecked = value;
 if(value)
 CustomersSuppliers = new ObservableCollection<IStakeholder>(CustomerList);
 }
 }
 /// <summary>
 /// Binds to the Supplier Radio Button
 /// </summary>
 public bool ButtonSupplierIsChecked
 {
 get => _buttonSupplierIsChecked;
 set
 {
 _buttonSupplierIsChecked = value;
 if(value)
 CustomersSuppliers = new ObservableCollection<IStakeholder>(SupplierList);
 }
 }

C# - InputDesignModel (Dummy data)

public class InputDesignModel : InputViewModel
 {
 #region Singleton
 public static InputDesignModel Instance = new InputDesignModel();
 #endregion
 #region Constructor
 public InputDesignModel()
 {
 CustomersSuppliers = new ObservableCollection<IStakeholder>();
 CustomerList = new ObservableCollection<Customer>
 {
 new Customer{Id=1, Name="Bart"},
 new Customer{Id=1, Name="Nick"},
 new Customer{Id=1, Name="Erwin"},
 new Customer{Id=1, Name="Erwin"},
 };
 SupplierList = new ObservableCollection<Supplier>
 {
 new Supplier{Id=1, Name="Rita"},
 new Supplier{Id=1, Name="Sascha"},
 new Supplier{Id=1, Name="Didier"},
 };
 }
 #endregion
 }

WPF - UI

<Grid>
 <Grid.ColumnDefinitions>
 <ColumnDefinition/>
 <ColumnDefinition/>
 </Grid.ColumnDefinitions>
 <RadioButton Content="Klant"
 Grid.Column="0"
 Foreground="White"
 Margin="10"
 IsChecked="{Binding ButtonCustomerIsChecked}"
 GroupName="CustomerSupplier"
 Name="RadioButtonCustomer"
 />
 <RadioButton Content="Leverancier"
 Grid.Column="1"
 Foreground="White"
 Margin="10"
 IsChecked="{Binding ButtonSupplierIsChecked}"
 GroupName="CustomerSupplier"
 Name="RadioButtonSupplier"
 />
</Grid>
<ComboBox ItemsSource="{Binding CustomersSuppliers}"
 DisplayMemberPath="Name"
 SelectedItem="{Binding Path=SelectedCustomerSupplier}"
 Style="{StaticResource InputComboBox}"
 />

Model data

Istakeholder interface

public class Customer : IStakeholder
 {
 /// <summary>
 /// The ID of the customer
 /// </summary>
 public int Id { get; set; }
 /// <summary>
 /// The name of the customer
 /// </summary>
 public string Name { get; set; }
 /// <summary>
 /// The phone number
 /// </summary>
 public string PhNr { get; set; }
 /// <summary>
 /// The VAT number
 /// can be null
 /// </summary>
 public string VatNr { get; set; }
 /// <summary>
 /// The country where the customer is located
 /// </summary>
 public CountryCatEnum CountryCat { get; set; }
 /// <summary>
 /// A list of payments made by the customer
 /// </summary>
 public List<IPayments> Payments { get; set; }
 }

Supplier class (customer is basicly the same)

public class Supplier : IStakeholder
 {
 /// <summary>
 /// The name of the supplier
 /// </summary>
 /// <returns></returns>
 public String Name { get; set; }
 /// <summary>
 /// The ID of the supplier
 /// </summary>
 public int Id { get; set; }
 /// <summary>
 /// The telephone number of the supplier
 /// </summary>
 public string PhNr { get; set; }
 /// <summary>
 /// The country 
 /// </summary>
 public CountryCatEnum CountryCat { get; set; }
 /// <summary>
 /// The VAT number of the supplier
 /// </summary>
 public string VatNr { get; set; }
 /// <summary>
 /// The standard bank account number to pay to
 /// </summary>
 public string BankAcc { get; set; }
 /// <summary>
 /// List of payments made to this supplier
 /// </summary>
 public List<IPayments> Payments { get; set; }
 }

Answer 1

The InputDesignModel class looks very weird. First these #region shouldn't be there - it's certainly not auto-generated code, and its entire complete listing easily fits any screen, even with the members collapsed. There's no need for #region, at least not in code that isn't in the process of being refactored - so, definitely not in production code. Remove.

But it's the "singleton" implementation that's jumping at me. Its #region says it's a singleton (why on Earth would you need a region to wrap a single member anyway), but then there's this public, parameterless constructor that's right there for anyone to use: this isn't a singleton class, it's just a public static field that clearly serves no purpose. If its presence is somehow justified in XAML bindings somewhere, then there should be a comment to that effect, but there isn't, so the field strikes me as useless, if not dangerously misleading.

The view model shouldn't need to know about checkboxes and radiobuttons and whatnot - that's strictly a presentation concern. I can see something like this being somewhat useful:

// ReSharper disable once UnusedMember.Global; property is used in XAML bindings.

However it's kind of implied already, by the simple fact that this is a view model class should tell the reader that the properties are used in XAML bindings. But this?

// Combobox list
public IEnumerable<Customer> CustomerList { get; set; }

The name, CustomerList, is also a lie: this isn't a List<Customer> - it's just a bunch of customers that can be enumerated - whether it's a List<Custoemr>, a Stack<Customer> a HashSet<Customer>, or whatever other possible IEnumerable<Customer> type, we don't care: the property should be named Customers, period.

Then there's that comment. Such comments have no business in the view model: whether the view currently represents these items as ComboBox items is irrelevant. Besides, tomorrow the view might represent them in a custom ListView, or any other ContentControl - it's completely not the view model's responsibility to care what kind of XAML element is binding to it. Remove that comment.

Same reasonning applies here:

/// <summary>
/// List with either customers or suppliers
/// Choice depends on radiobox in the View
/// </summary>
public ObservableCollection<IStakeholder> CustomersSuppliers { get; set; }

In MVVM the view model doesn't even know that there's a view - and there might not even be one (unit tests?)! The contents of that observable collection doesn't depend on a radiobox in the view, it depends on what the two setters underneath it are doing: that there's a radiobox in a view that happens to bind to these two properties is none of its concern.

Ok.

Now you're thinking "what's wrong with this guy, why is he nit-picking on comments?!" - these comments, the names you give your properties (e.g. ButtonSupplierIsChecked), they ultimately convey your understanding of the responsibilities of each component of the MVVM pattern: when I see "is the supplier checkbox checked" logic in a view model, I immediately start wondering why that logic isn't implemented in the view - because checkboxes and UI state belongs in the view, not in the view model.

Now now.. so you have two implementations of an IStakeHolder interface, which apparently expose at least an Id and a Name property. This then:

public interface IStakeHolder
{
 int Id { get; }
 string Name { get; }
}

IMO the interface shouldn't expose setters. I can't tell if it does, but anyway IMO these values should be immutable when accessed from the IStakeHolder interface. Doesn't keep you from using auto-properties in the implementation, and have such code work:

new Supplier{Id=1, Name="Rita"}

You could add an enum member, to specify the "kind" of stakeholder:

public enum StakeHolderType
{
 Supplier,
 Customer,
 //...
}

So:

new Supplier{Id=1, Name="Rita", Type=StakeHolderType.Supplier}

Of course it doesn't make much sense to be able to make a new Supplier with a StakeHolderType.Customer, so you could implement the interface in some StakeHolderBase abstract class that takes the enum value as a constructor parameter, and so the supplier constructor could do:

public Supplier(int id, string name) : base(StakeHolderType.Supplier)
{
 Id = id;
 Name = name;
}

And then this would still work identically:

new Supplier{Id=1, Name="Rita"}

Except now, you're equipped to turn that source-swapping logic into a simple filter - and that filter could quite possibly be implemented entirely in XAML, removing all these really-belongs-on-the-view view model properties and leaving only the CustomersSuppliers property on the view model - and you could even rename it to a much cleaner and precise StakeHolders property.

• Bart Pinnock: thank you for your very thorough review. I'm just starting as a programmer and your comments help a lot. I could use some clarification though. Can you describe a bit more precise how I could get rid of the button...IsChecked while still maintaining the ability to show two lists in one combobox based on a selection?
• Mathieu Guindon: @BartPinnock you would need to research about, say, toggling filters in XAML - it's very much beyond the scope of this review though; I'm just planting a seed here, a concept. The filtering can be done all-XAML, without involving UI state on the view model. I'm sure Stack Overflow has a ton of examples.