C# MVVM pattern sanity check

Question

I am needing to get a sanity check to make sure I am setting up my model and viewmodel correctly. I believe what I have is correct even though I have seen some documentation that shows different ways this can be done. I posted two examples of my code below. Any hints or suggestions are always appreciated.

Model:

namespace StewLedger.Model
{
 public class AccountModel : IDisposable
{
private Connection conn = new Connection (ConnectionString.connectionString);
private bool disposed = false;
private SafeHandle handle = new SafeFileHandle(IntPtr.Zero, true);
private AccountVM accountVM = new AccountVM();
private AccountTable accountTable = new AccountTable();
public bool AddNewAccount(AccountVM account)
{
 if(conn.NonQueryCommand(accountTable.AddAccount(account.AccountType, account.AccountName, User._username, account.BankId, account.AccountNumber, account.StartingBalance, account.Balance)))
 {
 return true;
 }
 return false;
}
public Accounts LoadAccounts()
{
 List<string> o = conn.QueryCommand(accountTable.GetAccounts(User._username));
 Accounts accounts = new Accounts();
 int numRecords = 0;
 numRecords = o.Count / 7;
 int item = 0, item1 = 1, item2 = 2, item3 = 3, item4 = 4, item5 = 5;
 for (int i = 0; i < numRecords; i++)
 {
 AccountVM accountVM = new AccountVM
 {
 Id = Convert.ToInt16(o[item]),
 AccountName = o[item1],
 AccountType = o[item2],
 Balance = Convert.ToDouble(o[item4]),
 StartingBalance = Convert.ToDouble(o[item3]),
 AccountNumber = o[item5]
 };
 item += 7;
 item1 += 7;
 item2 += 7;
 item3 += 7;
 item4 += 7;
 item5 += 7;
 accounts.addAccount(accountVM);
 }
 return accounts;
}
public bool UpdateBalance(double balance, int id)
{
 AccountModel accountModel = new AccountModel();
 if (conn.NonQueryCommand(accountTable.UpdateBalance(User._username, balance, id)))
 {
 return true;
 }
 return false;
}
public void SelectAccount(DashBoardView dashBoard)
{
 AccountVM accountVM = (AccountVM)dashBoard.gridAccounts.SelectedItem;
 Transaction transaction = new Transaction();
 Accounts accounts = LoadAccounts();
 LedgerModel ledgerModel = new LedgerModel();
 if (accountVM != null)
 {
 transaction =ledgerModel.LoadLedger(accountVM.Id, accountVM.AccountName);
 for (int i = 0; i < accounts.Count; i++)
 {
 if (accounts[i].Id == accountVM.Id)
 {
 accountVM.Balance = accounts[i].StartingBalance;
 }
 }
 for (int i = 0; i < transaction.Count; i++)
 {
 transaction[i].Balance = ledgerModel.CalculateBalance(Convert.ToDouble(accountVM.Balance), transaction[i]);
 accountVM.Balance = transaction[i].Balance;
 }
 if (transaction != null)
 {
 for (int i = 0; i < transaction.Count; i++)
 {
 if (transaction[i].TransType == "Withdrawal")
 {
 transaction[i].Color = new SolidColorBrush(Colors.Red);
 }
 else if (transaction[i].TransType == "Deposit")
 {
 transaction[i].Color = new SolidColorBrush(Colors.SteelBlue);
 }
 else
 {
 transaction[i].Color = new SolidColorBrush(Colors.Green);
 }
 }
 dashBoard.gridLedger.ItemsSource = transaction;
 }
 }
}
public void DeleteAccount(DashBoardView dashBoard)
{
 AccountTable accountTable = new AccountTable();
 LedgerTable ledgerTable = new LedgerTable();
 AccountVM accountVM = (AccountVM)dashBoard.gridAccounts.SelectedItem;
 using (Connection conn = new Connection(ConnectionString.connectionString))
 {
 TableInfo tableInfo = new TableInfo
 {
 Id = accountVM.Id,
 Owner = User._username,
 TableName = "Accounts",
 T = TableInfo.Table.ACCT
 };
 conn.NonQueryCommand(accountTable.RemoveRow(tableInfo));
 if (conn.TableExist(User._username + accountVM.AccountName + "Ledger") == true)
 {
 conn.NonQueryCommand(ledgerTable.DropTable(User._username, accountVM.AccountName));
 }
 dashBoard.gridAccounts.ItemsSource = LoadAccounts();
 };
}
public void Dispose()
{
 Dispose(false);
 GC.SuppressFinalize(this);
}
protected virtual void Dispose(bool disposing)
{
 if (disposed)
 {
 return;
 }
 //Free managed resources
 if (disposing)
 {
 handle.Dispose();
 conn.Dispose();
 }
 disposed = true;
 }
}
}

ViewModel:

namespace StewLedger.ViewModel
{
public class AccountVM : Notifier
{
private string _accountType;
private string _accountName;
private string _accountNumber;
private double _balance, _startingBalance;
private int _Id;
private int _bankId;
public int Id { get => _Id; set { _Id = value; OnPropertyChanged("Id"); } }
public string AccountType { get => _accountType; set { _accountType = value; OnPropertyChanged("AccountType"); } }
public string AccountName { get => _accountName; set { _accountName = value; OnPropertyChanged("AccountName"); } }
public double Balance { get => _balance; set { _balance = value; OnPropertyChanged("Balance"); } }
public int BankId { get => _bankId; set { _bankId = value; OnPropertyChanged("BankId"); } }
public string AccountNumber { get => _accountNumber; set { _accountNumber = value; OnPropertyChanged("AccountNumber"); } }
public double StartingBalance { get => _startingBalance; set { _startingBalance = value; OnPropertyChanged("StartingBalance"); } }
}
public class AccountTypes : ObservableCollection<string>
{
public AccountTypes()
{
 this.Add("Checking");
 this.Add("Savings");
 //Add credit and loans later will require seperate setup
}
}
public class Accounts : ObservableCollection<AccountVM>
{
public Accounts()
{
}
public void addAccount(AccountVM vM)
{
 this.Add(vM);
}
public ObservableCollection<AccountVM> GetAccounts()
{
 return this;
}
public void deleteAllAccounts()
{
 this.Clear();
 }
}
}

• Adriano Repetti: Exact requirements for VM depend on UI framework you're using but a reference to VM inside the model is surely a smell. Actually your model is doing VM things and manipulating directly the view. After that we can talk about micro design.
• M. Stewart: This Model pretty much access and manipulates the database. There is a database connection class, tables classes, etc. My understanding of the ViewModel is that it pretty much holds data items that can be observed by the view and the model is any code that does something with those data items.
• M. Stewart: Using C# and WPF. The MVVM is rolled be me, I am not using Prism or anything like that.
• Pieter Witvoet: How is this model being used? That is, where is the code that calls those LoadAccounts, UpdateBalance and SelectAccount methods?
• M. Stewart: It depends, for instance addNewAccound adds a row to the database and LoadAccounts returns a list of accounts from the database . They are located where they are needed. Methods like Delete and SelectAccount are methods for button events. After doing more research I think I see where I fundamentally went way off of the rails.

Answer 1

MVVM

MVVM is normally structured as following:

 View // user interface
 ↑ ↓
Viewmodel // handles user interaction, makes data ready for display, 'glue' layer
 ↑ ↓
 Model // actual business logic

Views display data taken from view-models, and send user input back to view-models (in WPF this happens via data and command bindings). View-models don't reference views directly, but notify them about data updates via events (INotifyPropertyChanged).

View-models operate on models (often in reaction to user input) and make data (extracted from models) ready for display and interaction. Models don't know about view-models, but (if necessary) can raise events that view-models can react to.

This makes the model reusable in different contexts (console application, service, server-side, etc.) and you can write tests for it without having to fiddle with UI automation.

Your architecture

In your case, the model directly operates on views and view-models, and those view-model classes are used as if they're part of the model layer. This makes the model difficult to reuse and to test, and there's an increased risk that UI changes will break something or require additional model changes.

Here's roughly how I would structure your code (various things omitted for clarity):

DashBoardView (DataContext -> DashBoardViewModel)
 - accounts list-view (ItemsSource -> Accounts, SelectedItem -> SelectedAccount)
 - add-account button (Command -> AddAccountCommand)
 - determines how to display accounts,
 color-selection could be done with ValueConverters or data triggers
--------------------
DashBoardViewModel
 - Accounts property, a list of AccountViewModel objects
 - SelectedAccount, an AccountViewModel
 - AddAccountCommand, an ICommand that calls AddAccount on the model and refreshes Accounts
 - holds a reference to an AccountModel
AccountViewModel
 - wraps or represents an Account object
 - takes care of input validation
 - may contain additional properties to make editing or display easier
--------------------
AccountModel
 - AddAccount method, adds an account to the database
 - GetAccounts method, returns a collection of Account objects
 - perhaps an account-added event, if multiple VM's need to be informed
Account
 - holds account information
 - could be immutable, depending on how it is meant to be used

Model class

Here are some more observations:

• Why does QueryCommand return a 'flat' list of values? Why not a list of records, or a DataTable? Hard-coding the column count is brittle.
• UpdateBalance creates an AccountModel object that isn't used.
• SelectAccount and DeleteAccount should not operate on a view. Keeping track of item selection and passing data to the model layer is a task for view-models.
• Transaction is a confusing name. It sounds like a database transaction, but it's actually a collection, and it's exposed directly to the view?
• Display colors are a responsibility of the view, not of the model.
• TransType should probably be an enum. Enums clearly indicate which values you can expect (a string could contain any text), which also helps 'documenting' the code.
• SelectAccount uses for loops, where foreach loops would be easier and more readable.
• Why does DeleteAccount create its own Connection? All other methods use the conn field.
• Where does User._username come from? The leading underscore makes it look like a private field, so that's odd. Either way, I assume it's a static field or property. Static things are essentially global, which means they can be accessed (and possibly changed) from anywhere. It also makes dependencies harder to spot. All that tends to make code more difficult to maintain. If AccountModel depends on a User, it should be passed in as a constructor or method argument (dependency injection).
• Dispose doesn't need to call GC.SuppressFinalize because your class doesn't have a finalizer (finalizers are rarely needed). There's no need for the full Dispose(bool disposing) pattern here.
• Why do SelectAccount and DeleteAccount call LoadAccounts? That seems like a lot of unnecessary work for a single change.

View-model classes

• Personally I would group backing fields and properties together.
• Use nameof(PropertyName) instead of "PropertyName" to make things more refactor-proof, or use the [CallerMemberName] attribute in your OnPropertyChanged method so you don't need to specify the property name at all.
• Why inherit from ObservableCollection? Why not just use an ObservableCollection<AccountVM> directly? The extra methods don't provide any additional functionality (especially GetAccounts - what's the idea behind that?).

And something that applies to both:

• Inconsistent naming: some private fields are prefixed with an underscore, others are not. Most method names use PascalCase, but some use camelCase. Try sticking to the standard C# naming conventions, and be consistent - it'll make code a little easier to read.

Answer 2

From top to bottom review we will...

AccountModel

private SafeHandle handle = new SafeFileHandle(IntPtr.Zero, true);
private AccountVM accountVM = new AccountVM(); 

These variables aren't used and can be removed.

AddNewAccount() and some other methods as well

Whenever you see a code snippet like

if (someCondition)
{
 return true;
}
return false; 

you can simply return the codition itself like

public bool AddNewAccount(AccountVM account)
{
 return (conn.NonQueryCommand(accountTable.AddAccount(account.AccountType, account.AccountName, User._username, account.BankId, account.AccountNumber, account.StartingBalance, account.Balance)));
} 

LoadAccounts()

Well o isn't really a descriptive name, is it? You should always name things as descriptive as possible. Assume you will come back to this code in 6 months because you have to fix a bug or add some functionality. You will spend a lot of time to understand the code again if you name things poorly.

The whole stuff about numRecords, item, item1, item2, ... could be rewritten like so

public Accounts LoadAccounts()
{
 List<string> o = conn.QueryCommand(accountTable.GetAccounts(User._username));
 Accounts accounts = new Accounts();
 for (int i = 0; i < o.Count; i += 7)
 {
 AccountVM accountVM = new AccountVM
 {
 Id = Convert.ToInt16(o[i + 0]),
 AccountName = o[i + 1],
 AccountType = o[i + 2],
 Balance = Convert.ToDouble(o[i + 3]),
 StartingBalance = Convert.ToDouble(o[i + 4]),
 AccountNumber = o[i + 5]
 };
 accounts.addAccount(accountVM);
 }
 return accounts;
} 

and now we see as well that the call of conn.QueryCommand(accountTable.GetAccounts(User._username)); returns more than needed.

UpdateBalance()

See

AddNewAccount() and some other methods as well

SelectAccount()

Using a guard check if (accountVM == null) { return; } will remove one level of indentation and the creation of

Transaction transaction = new Transaction();
Accounts accounts = LoadAccounts();
LedgerModel ledgerModel = new LedgerModel();

isn't needed anymore for the case that the condition is true.

The loop

for (int i = 0; i < accounts.Count; i++)
{
 if (accounts[i].Id == accountVM.Id)
 {
 accountVM.Balance = accounts[i].StartingBalance;
 }
} 

looks a little strange, but I just assume there is at maximum one account with an Id equal to accountVM.Id. If Accounts would be a class which implements e.g IEnumerable<T> one could use FirstOrDefault(a => a.Id == accountVM.Id) which would make it more obvious what is happening here.
Edit: I just noticed that Accounts is an ObservableCollection<AccountVM> hence using FirstOrDefault() should be the way to go.

At first glance it looks like the value of accountVM.Balance is overwritten each time.

The condition if (transaction != null) will never ever be false. You can safely remove it.

The coloring of the transactions should be moved inside a separate method.

Rewritten it would look like so

public void SelectAccount(DashBoardView dashBoard)
{
 AccountVM accountVM = (AccountVM)dashBoard.gridAccounts.SelectedItem;
 if (accountVM == null) { return; }
 Accounts accounts = LoadAccounts();
 for (int i = 0; i < accounts.Count; i++)
 {
 if (accounts[i].Id == accountVM.Id)
 {
 accountVM.Balance = accounts[i].StartingBalance;
 }
 }
 LedgerModel ledgerModel = new LedgerModel();
 Transaction transaction = ledgerModel.LoadLedger(accountVM.Id, accountVM.AccountName);
 for (int i = 0; i < transaction.Count; i++)
 {
 transaction[i].Balance = ledgerModel.CalculateBalance(Convert.ToDouble(accountVM.Balance), transaction[i]);
 accountVM.Balance = transaction[i].Balance;
 }
 ColorTransaction(transaction);
 dashBoard.gridLedger.ItemsSource = transaction;
} 
private void ColorTransaction(Transaction transaction) 
{
 for (int i = 0; i < transaction.Count; i++)
 {
 if (transaction[i].TransType == "Withdrawal")
 {
 transaction[i].Color = new SolidColorBrush(Colors.Red);
 }
 else if (transaction[i].TransType == "Deposit")
 {
 transaction[i].Color = new SolidColorBrush(Colors.SteelBlue);
 }
 else
 {
 transaction[i].Color = new SolidColorBrush(Colors.Green);
 }
 }
} 

DeleteAccount()

In this method you don't bother that AccountVM accountVM may be null. Either it is possible then you should check it, or it isn't possible then you won't need the check in SelectAccount() as well.

• M. Stewart: I checked Pieter's comment bellow, but honestly both of these post were extremely helpful!